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.
This commit is contained in:
Daniel Bennett
2024-07-11 09:59:24 -05:00
committed by GitHub
parent f3de47e63d
commit 372dfd3816
4 changed files with 31 additions and 16 deletions

View File

@@ -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)
}

View File

@@ -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

View File

@@ -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

View File

@@ -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) {