From ebbd19decab14f7fe136858f7db3361b8699f673 Mon Sep 17 00:00:00 2001 From: Jingfang Liu Date: Mon, 7 Mar 2022 14:15:25 -0800 Subject: [PATCH] fix: Use empty object status list when the StatusPolicyNone is used This reduces the request size when StatusPolicyNone is used --- cmd/main.go | 2 +- pkg/apply/common_test.go | 2 +- pkg/inventory/inventory-client.go | 13 ++++- pkg/inventory/inventory-client_test.go | 66 ++++++++++++++++++-------- pkg/inventory/inventorycm.go | 16 +++---- pkg/inventory/inventorycm_test.go | 17 +++---- test/e2e/e2e_test.go | 4 +- 7 files changed, 73 insertions(+), 47 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 36ffc94a..c35dc068 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -56,7 +56,7 @@ func main() { initCmd := initcmd.NewCmdInit(f, ioStreams) updateHelp(names, initCmd) loader := manifestreader.NewManifestLoader(f) - invFactory := inventory.ClusterClientFactory{} + invFactory := inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone} applyCmd := apply.Command(f, invFactory, loader, ioStreams) updateHelp(names, applyCmd) previewCmd := preview.Command(f, invFactory, loader, ioStreams) diff --git a/pkg/apply/common_test.go b/pkg/apply/common_test.go index 6e7a94b6..24e80b7e 100644 --- a/pkg/apply/common_test.go +++ b/pkg/apply/common_test.go @@ -123,7 +123,7 @@ func newTestInventory( ) inventory.Client { // Use an Client with a fakeInfoHelper to allow generating Info // objects that use the FakeRESTClient as the UnstructuredClient. - invClient, err := inventory.ClusterClientFactory{}.NewClient(tf) + invClient, err := inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyAll}.NewClient(tf) require.NoError(t, err) return invClient } diff --git a/pkg/inventory/inventory-client.go b/pkg/inventory/inventory-client.go index 84690701..aa8fabaf 100644 --- a/pkg/inventory/inventory-client.go +++ b/pkg/inventory/inventory-client.go @@ -105,7 +105,10 @@ func (cic *ClusterClient) Merge(localInv Info, objs object.ObjMetadataSet, dryRu } if clusterInv == nil { // Wrap inventory object and store the inventory in it. - status := getObjStatus(nil, objs) + var status []actuation.ObjectStatus + if cic.statusPolicy == StatusPolicyAll { + status = getObjStatus(nil, objs) + } inv := cic.InventoryFactoryFunc(invObj) if err := inv.Store(objs, status); err != nil { return nil, err @@ -132,7 +135,10 @@ func (cic *ClusterClient) Merge(localInv Info, objs object.ObjMetadataSet, dryRu } pruneIds = clusterObjs.Diff(objs) unionObjs := clusterObjs.Union(objs) - status := getObjStatus(pruneIds, unionObjs) + var status []actuation.ObjectStatus + if cic.statusPolicy == StatusPolicyAll { + status = getObjStatus(pruneIds, unionObjs) + } klog.V(4).Infof("num objects to prune: %d", len(pruneIds)) klog.V(4).Infof("num merged objects to store in inventory: %d", len(unionObjs)) wrappedInv := cic.InventoryFactoryFunc(clusterInv) @@ -203,6 +209,9 @@ func (cic *ClusterClient) Replace(localInv Info, objs object.ObjMetadataSet, sta // replaceInventory stores the passed objects into the passed inventory object. func (cic *ClusterClient) replaceInventory(inv *unstructured.Unstructured, objs object.ObjMetadataSet, status []actuation.ObjectStatus) (*unstructured.Unstructured, error) { + if cic.statusPolicy == StatusPolicyNone { + status = nil + } wrappedInv := cic.InventoryFactoryFunc(inv) if err := wrappedInv.Store(objs, status); err != nil { return nil, err diff --git a/pkg/inventory/inventory-client_test.go b/pkg/inventory/inventory-client_test.go index f5cc5338..6732694a 100644 --- a/pkg/inventory/inventory-client_test.go +++ b/pkg/inventory/inventory-client_test.go @@ -37,6 +37,12 @@ func podData(name string) map[string]string { } } +func podDataNoStatus(name string) map[string]string { + return map[string]string{ + fmt.Sprintf("test-inventory-namespace_%s__Pod", name): "", + } +} + func TestGetClusterInventoryInfo(t *testing.T) { tests := map[string]struct { statusPolicy StatusPolicy @@ -125,18 +131,20 @@ func TestMerge(t *testing.T) { isError bool }{ "Nil local inventory object is error": { - localInv: nil, - localObjs: object.ObjMetadataSet{}, - clusterObjs: object.ObjMetadataSet{}, - pruneObjs: object.ObjMetadataSet{}, - isError: true, + localInv: nil, + localObjs: object.ObjMetadataSet{}, + clusterObjs: object.ObjMetadataSet{}, + pruneObjs: object.ObjMetadataSet{}, + isError: true, + statusPolicy: StatusPolicyAll, }, "Cluster and local inventories empty: no prune objects; no change": { - localInv: copyInventory(), - localObjs: object.ObjMetadataSet{}, - clusterObjs: object.ObjMetadataSet{}, - pruneObjs: object.ObjMetadataSet{}, - isError: false, + localInv: copyInventory(), + localObjs: object.ObjMetadataSet{}, + clusterObjs: object.ObjMetadataSet{}, + pruneObjs: object.ObjMetadataSet{}, + isError: false, + statusPolicy: StatusPolicyAll, }, "Cluster and local inventories same: no prune objects; no change": { localInv: copyInventory(), @@ -146,8 +154,9 @@ func TestMerge(t *testing.T) { clusterObjs: object.ObjMetadataSet{ ignoreErrInfoToObjMeta(pod1Info), }, - pruneObjs: object.ObjMetadataSet{}, - isError: false, + pruneObjs: object.ObjMetadataSet{}, + isError: false, + statusPolicy: StatusPolicyAll, }, "Cluster two obj, local one: prune obj": { localInv: copyInventory(), @@ -161,7 +170,8 @@ func TestMerge(t *testing.T) { pruneObjs: object.ObjMetadataSet{ ignoreErrInfoToObjMeta(pod3Info), }, - isError: false, + statusPolicy: StatusPolicyAll, + isError: false, }, "Cluster multiple objs, local multiple different objs: prune objs": { localInv: copyInventory(), @@ -176,7 +186,8 @@ func TestMerge(t *testing.T) { ignoreErrInfoToObjMeta(pod1Info), ignoreErrInfoToObjMeta(pod3Info), }, - isError: false, + statusPolicy: StatusPolicyAll, + isError: false, }, } @@ -309,8 +320,9 @@ func TestReplace(t *testing.T) { clusterObjs: object.ObjMetadataSet{ ignoreErrInfoToObjMeta(pod1Info), }, - objStatus: []actuation.ObjectStatus{podStatus(pod1Info)}, - data: podData("pod-1"), + objStatus: []actuation.ObjectStatus{podStatus(pod1Info)}, + data: podData("pod-1"), + statusPolicy: StatusPolicyAll, }, "Cluster two obj, local one": { localObjs: object.ObjMetadataSet{ @@ -320,8 +332,9 @@ func TestReplace(t *testing.T) { ignoreErrInfoToObjMeta(pod1Info), ignoreErrInfoToObjMeta(pod3Info), }, - objStatus: []actuation.ObjectStatus{podStatus(pod1Info), podStatus(pod3Info)}, - data: podData("pod-1"), + objStatus: []actuation.ObjectStatus{podStatus(pod1Info), podStatus(pod3Info)}, + data: podData("pod-1"), + statusPolicy: StatusPolicyAll, }, "Cluster multiple objs, local multiple different objs": { localObjs: object.ObjMetadataSet{ @@ -331,8 +344,21 @@ func TestReplace(t *testing.T) { ignoreErrInfoToObjMeta(pod1Info), ignoreErrInfoToObjMeta(pod2Info), ignoreErrInfoToObjMeta(pod3Info)}, - objStatus: []actuation.ObjectStatus{podStatus(pod2Info), podStatus(pod1Info), podStatus(pod3Info)}, - data: podData("pod-2"), + objStatus: []actuation.ObjectStatus{podStatus(pod2Info), podStatus(pod1Info), podStatus(pod3Info)}, + data: podData("pod-2"), + statusPolicy: StatusPolicyAll, + }, + "Cluster multiple objs, local multiple different objs with StatusPolicyNone": { + localObjs: object.ObjMetadataSet{ + ignoreErrInfoToObjMeta(pod2Info), + }, + clusterObjs: object.ObjMetadataSet{ + ignoreErrInfoToObjMeta(pod1Info), + ignoreErrInfoToObjMeta(pod2Info), + ignoreErrInfoToObjMeta(pod3Info)}, + objStatus: []actuation.ObjectStatus{podStatus(pod2Info), podStatus(pod1Info), podStatus(pod3Info)}, + data: podDataNoStatus("pod-2"), + statusPolicy: StatusPolicyNone, }, } diff --git a/pkg/inventory/inventorycm.go b/pkg/inventory/inventorycm.go index 8c841673..63a496cf 100644 --- a/pkg/inventory/inventorycm.go +++ b/pkg/inventory/inventorycm.go @@ -107,14 +107,11 @@ func (icm *ConfigMap) Store(objMetas object.ObjMetadataSet, status []actuation.O // or an error if one occurs. func (icm *ConfigMap) GetObject() (*unstructured.Unstructured, error) { // Create the objMap of all the resources, and compute the hash. - objMap, err := buildObjMap(icm.objMetas, icm.objStatus) - if err != nil { - return nil, err - } + objMap := buildObjMap(icm.objMetas, icm.objStatus) // Create the inventory object by copying the template. invCopy := icm.inv.DeepCopy() // Adds the inventory map to the ConfigMap "data" section. - err = unstructured.SetNestedStringMap(invCopy.UnstructuredContent(), + err := unstructured.SetNestedStringMap(invCopy.UnstructuredContent(), objMap, "data") if err != nil { return nil, err @@ -122,7 +119,7 @@ func (icm *ConfigMap) GetObject() (*unstructured.Unstructured, error) { return invCopy, nil } -func buildObjMap(objMetas object.ObjMetadataSet, objStatus []actuation.ObjectStatus) (map[string]string, error) { +func buildObjMap(objMetas object.ObjMetadataSet, objStatus []actuation.ObjectStatus) map[string]string { objMap := map[string]string{} objStatusMap := map[object.ObjMetadata]actuation.ObjectStatus{} for _, status := range objStatus { @@ -132,12 +129,11 @@ func buildObjMap(objMetas object.ObjMetadataSet, objStatus []actuation.ObjectSta if status, found := objStatusMap[objMetadata]; found { objMap[objMetadata.String()] = stringFrom(status) } else { - // This should never happen since the objStatus have captured all the - // object status - return nil, fmt.Errorf("object status not found for %s", objMetadata) + // It's possible that the passed in status doesn't any object status + objMap[objMetadata.String()] = "" } } - return objMap, nil + return objMap } func stringFrom(status actuation.ObjectStatus) string { diff --git a/pkg/inventory/inventorycm_test.go b/pkg/inventory/inventorycm_test.go index ce36d8a2..413975cb 100644 --- a/pkg/inventory/inventorycm_test.go +++ b/pkg/inventory/inventorycm_test.go @@ -54,21 +54,16 @@ func TestBuildObjMap(t *testing.T) { }, "empty object status list": { objSet: object.ObjMetadataSet{ObjMetadataFromObjectReference(obj1), ObjMetadataFromObjectReference(obj2)}, - hasError: true, + hasError: false, + expected: map[string]string{ + "ns_na_group1_Kind": "", + "ns_na_group2_Kind": "", + }, }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { - actual, err := buildObjMap(tc.objSet, tc.objStatus) - if tc.hasError { - if err == nil { - t.Fatalf("expected erroe, but not happened") - } - return - } - if err != nil { - t.Fatal(err) - } + actual := buildObjMap(tc.objSet, tc.objStatus) if diff := cmp.Diff(actual, tc.expected); diff != "" { t.Errorf(diff) } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index f3092766..15318259 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -345,11 +345,11 @@ func deleteNamespace(ctx context.Context, c client.Client, namespace *v1.Namespa } func newDefaultInvApplier() *apply.Applier { - return newApplierFromInvFactory(inventory.ClusterClientFactory{}) + return newApplierFromInvFactory(inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyAll}) } func newDefaultInvDestroyer() *apply.Destroyer { - return newDestroyerFromInvFactory(inventory.ClusterClientFactory{}) + return newDestroyerFromInvFactory(inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyAll}) } func defaultInvNotExistsFunc(ctx context.Context, c client.Client, name, namespace, id string) {