From 372dfd38162c60ca9bec518ad775a91455f3384a Mon Sep 17 00:00:00 2001
From: Daniel Bennett <dbennett@hashicorp.com>
Date: Thu, 11 Jul 2024 09:59:24 -0500
Subject: [PATCH] msgpack: omit empty fields on NetworkResource (#23540)

the ratio of optimized/unoptimized log size in TestPlanNormalize
has been increased several times as people have added to various
structs and coincidentally bumped into the magic limit.

we encountered another such case when adding to NetworkResource,
but here we omitempty on the struct instead of bumping the limit
in the test.

this has the added benefit of reducing the serialized struct size!
which was the original intent behind this test in the first place :P
the actual value of the ratio is now 0.628... but here the
test value is only dropped down to 0.66 to leave some wiggle room.
---
 nomad/plan_normalization_test.go | 12 ++++++++++--
 nomad/structs/diff.go            |  2 +-
 nomad/structs/structs.go         |  3 +++
 nomad/structs/structs_test.go    | 30 +++++++++++++++++-------------
 4 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/nomad/plan_normalization_test.go b/nomad/plan_normalization_test.go
index b167d452b..0f34abc9e 100644
--- a/nomad/plan_normalization_test.go
+++ b/nomad/plan_normalization_test.go
@@ -12,7 +12,7 @@ import (
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
-	"github.com/stretchr/testify/assert"
+	"github.com/shoenig/test/must"
 )
 
 // This test compares the size of the normalized + OmitEmpty raft plan log entry
@@ -20,6 +20,13 @@ import (
 //
 // Whenever this test is changed, care should be taken to ensure the older msgpack size
 // is recalculated when new fields are introduced in ApplyPlanResultsRequest
+//
+// If you make an unrelated change that unexpectedly fails this test,
+// consider adding omitempty to the struct you are modifying, e.g.:
+//
+//	 type NetworkResource struct {
+//		// msgpack omit empty fields during serialization
+//		_struct bool `codec:",omitempty"` // nolint: structcheck
 func TestPlanNormalize(t *testing.T) {
 	ci.Parallel(t)
 
@@ -68,5 +75,6 @@ func TestPlanNormalize(t *testing.T) {
 	}
 
 	optimizedLogSize := buf.Len()
-	assert.Less(t, float64(optimizedLogSize)/float64(unoptimizedLogSize), 0.67)
+	ratio := float64(optimizedLogSize) / float64(unoptimizedLogSize)
+	must.Less(t, 0.66, ratio)
 }
diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go
index e2a6ecec8..5b75d8cb9 100644
--- a/nomad/structs/diff.go
+++ b/nomad/structs/diff.go
@@ -2628,7 +2628,7 @@ func (r *Resources) Diff(other *Resources, contextual bool) *ObjectDiff {
 func (n *NetworkResource) Diff(other *NetworkResource, contextual bool) *ObjectDiff {
 	diff := &ObjectDiff{Type: DiffTypeNone, Name: "Network"}
 	var oldPrimitiveFlat, newPrimitiveFlat map[string]string
-	filter := []string{"Device", "CIDR", "IP", "MBits"}
+	filter := []string{"_struct", "Device", "CIDR", "IP", "MBits"}
 
 	if reflect.DeepEqual(n, other) {
 		return nil
diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
index 44bb64f33..e0315654d 100644
--- a/nomad/structs/structs.go
+++ b/nomad/structs/structs.go
@@ -2860,6 +2860,9 @@ func (d *DNSConfig) IsZero() bool {
 // NetworkResource is used to represent available network
 // resources
 type NetworkResource struct {
+	// msgpack omit empty fields during serialization
+	_struct bool `codec:",omitempty"` // nolint: structcheck
+
 	Mode          string     // Mode of the network
 	Device        string     // Name of the device
 	CIDR          string     // CIDR block of addresses
diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go
index ecf10ec78..0c81555e7 100644
--- a/nomad/structs/structs_test.go
+++ b/nomad/structs/structs_test.go
@@ -5188,22 +5188,26 @@ func TestPlan_AppendPreemptedAllocAppendsAllocWithUpdatedAttrs(t *testing.T) {
 	assert.Equal(t, expectedAlloc, appendedAlloc)
 }
 
-func TestAllocation_MsgPackTags(t *testing.T) {
+func TestMsgPackTags(t *testing.T) {
 	ci.Parallel(t)
-	planType := reflect.TypeOf(Allocation{})
 
-	msgPackTags, _ := planType.FieldByName("_struct")
+	cases := []struct {
+		name   string
+		typeOf reflect.Type
+	}{
+		{"Allocation", reflect.TypeOf(Allocation{})},
+		{"Evaluation", reflect.TypeOf(Evaluation{})},
+		{"NetworkResource", reflect.TypeOf(NetworkResource{})},
+		{"Plan", reflect.TypeOf(Plan{})},
+	}
 
-	assert.Equal(t, msgPackTags.Tag, reflect.StructTag(`codec:",omitempty"`))
-}
-
-func TestEvaluation_MsgPackTags(t *testing.T) {
-	ci.Parallel(t)
-	planType := reflect.TypeOf(Evaluation{})
-
-	msgPackTags, _ := planType.FieldByName("_struct")
-
-	assert.Equal(t, msgPackTags.Tag, reflect.StructTag(`codec:",omitempty"`))
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			field, ok := tc.typeOf.FieldByName("_struct")
+			must.True(t, ok, must.Sprint("_struct field not found"))
+			must.Eq(t, `codec:",omitempty"`, field.Tag, must.Sprint("bad _struct tag"))
+		})
+	}
 }
 
 func TestAllocation_Terminated(t *testing.T) {