Repository: clusternet/clusternet
Pull request #343 (2022-05-24 08:42:39)

fix bug about rollback sync condition judge
<!--  Thanks for sending a pull request!

-->

#### What type of PR is this?
Bug fix.
#### What this PR does / why we need it:

1. meta.resourceVersion does not exist in description.raw  so every time  we call ```ResourceNeedResync(curObj, resource) ``` it will return 'true' because of "delete action". 
2.  ```ResourceNeedResync```, normally this function return true, when add、remove、replace actions  happen. However when come to rollback, some fileds may polulated by child cluster controllers, so  'add action' should be ignored.  Add a parameter ``ignoreAdd``. When ```ApplyDescription``` triggerd from ```handleResource``` ignoreAdd is true.

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #

#### Special notes for your reviewer:



Patch:
diff --git a/pkg/agent/deployer/generic/generic.go b/pkg/agent/deployer/generic/generic.go
index 1421a3a..df5730d 100644
--- a/pkg/agent/deployer/generic/generic.go
+++ b/pkg/agent/deployer/generic/generic.go
@@ -132,7 +132,7 @@ func (deployer *Deployer) handleDescription(desc *appsapi.Description) error {
 	if !utils.DeployableByAgent(deployer.syncMode, deployer.appPusherEnabled) {
 		klog.V(5).Infof("Description %s is not deployable by agent, skipping syncing", klog.KObj(desc))
 		return utils.ApplyDescription(context.TODO(), deployer.clusternetClient, deployer.dynamicClient,
-			deployer.discoveryRESTMapper, desc, deployer.recorder, true, deployer.ResourceCallbackHandler)
+			deployer.discoveryRESTMapper, desc, deployer.recorder, true, deployer.ResourceCallbackHandler, false)
 	}
 
 	if desc.DeletionTimestamp != nil {
@@ -141,7 +141,7 @@ func (deployer *Deployer) handleDescription(desc *appsapi.Description) error {
 	}
 
 	return utils.ApplyDescription(context.TODO(), deployer.clusternetClient, deployer.dynamicClient,
-		deployer.discoveryRESTMapper, desc, deployer.recorder, false, deployer.ResourceCallbackHandler)
+		deployer.discoveryRESTMapper, desc, deployer.recorder, false, deployer.ResourceCallbackHandler, false)
 }
 
 func (deployer *Deployer) ResourceCallbackHandler(resource *unstructured.Unstructured) error {
@@ -200,7 +200,7 @@ func (deployer *Deployer) handleResource(ownedByValue string) error {
 	}
 
 	err = utils.ApplyDescription(context.TODO(), deployer.clusternetClient, deployer.dynamicClient,
-		deployer.discoveryRESTMapper, desc, deployer.recorder, false, nil)
+		deployer.discoveryRESTMapper, desc, deployer.recorder, false, nil, true)
 	if err == nil {
 		klog.V(4).Infof("successfully rollback Description %s", ownedByValue)
 	}
diff --git a/pkg/hub/deployer/generic/generic.go b/pkg/hub/deployer/generic/generic.go
index 4db7870..7dbd2cb 100644
--- a/pkg/hub/deployer/generic/generic.go
+++ b/pkg/hub/deployer/generic/generic.go
@@ -154,7 +154,7 @@ func (deployer *Deployer) handleDescription(desc *appsapi.Description) error {
 	}
 
 	return utils.ApplyDescription(context.TODO(), deployer.clusternetClient, dynamicClient,
-		discoveryRESTMapper, desc, deployer.recorder, false, nil)
+		discoveryRESTMapper, desc, deployer.recorder, false, nil, false)
 }
 
 func (deployer *Deployer) getDynamicClient(desc *appsapi.Description) (dynamic.Interface, meta.RESTMapper, error) {
diff --git a/pkg/known/constant.go b/pkg/known/constant.go
index d07b18c..4a0d769 100644
--- a/pkg/known/constant.go
+++ b/pkg/known/constant.go
@@ -72,11 +72,14 @@ const (
 
 // fields should be ignored when compared
 const (
-	MetaGeneration    = "/metadata/generation"
-	CreationTimestamp = "/metadata/creationTimestamp"
-	ManagedFields     = "/metadata/managedFields"
-	MetaUID           = "/metadata/uid"
-	MetaSelflink      = "/metadata/selfLink"
+	MetaGeneration      = "/metadata/generation"
+	CreationTimestamp   = "/metadata/creationTimestamp"
+	ManagedFields       = "/metadata/managedFields"
+	MetaUID             = "/metadata/uid"
+	MetaSelflink        = "/metadata/selfLink"
+	MetaResourceVersion = "/metadata/resourceVersion"
+
+	SectionStatus = "/status"
 )
 
 const (
diff --git a/pkg/utils/deployer.go b/pkg/utils/deployer.go
index 5d0241a..ee3b6b9 100644
--- a/pkg/utils/deployer.go
+++ b/pkg/utils/deployer.go
@@ -305,7 +305,7 @@ type ResourceCallbackHandler func(resource *unstructured.Unstructured) error
 
 func ApplyDescription(ctx context.Context, clusternetClient *clusternetclientset.Clientset, dynamicClient dynamic.Interface,
 	discoveryRESTMapper meta.RESTMapper, desc *appsapi.Description, recorder record.EventRecorder, dryApply bool,
-	callbackHandler ResourceCallbackHandler) error {
+	callbackHandler ResourceCallbackHandler, ignoreAdd bool) error {
 	var allErrs []error
 	wg := sync.WaitGroup{}
 	objectsToBeDeployed := desc.Spec.Raw
@@ -333,7 +333,7 @@ func ApplyDescription(ctx context.Context, clusternetClient *clusternetclientset
 
 			// dryApply means do not apply resources, just add sub resource watcher.
 			if !dryApply {
-				retryErr := ApplyResourceWithRetry(ctx, dynamicClient, discoveryRESTMapper, resource)
+				retryErr := ApplyResourceWithRetry(ctx, dynamicClient, discoveryRESTMapper, resource, ignoreAdd)
 				if retryErr != nil {
 					errCh <- retryErr
 					return
@@ -450,7 +450,8 @@ func OffloadDescription(ctx context.Context, clusternetClient *clusternetclients
 	return err
 }
 
-func ApplyResourceWithRetry(ctx context.Context, dynamicClient dynamic.Interface, restMapper meta.RESTMapper, resource *unstructured.Unstructured) error {
+func ApplyResourceWithRetry(ctx context.Context, dynamicClient dynamic.Interface, restMapper meta.RESTMapper,
+	resource *unstructured.Unstructured, ignoreAdd bool) error {
 	// set UID as empty
 	resource.SetUID("")
 
@@ -479,8 +480,7 @@ func ApplyResourceWithRetry(ctx context.Context, dynamicClient dynamic.Interface
 		} else {
 			lastError = nil
 		}
-
-		if ResourceNeedResync(curObj, resource) {
+		if ResourceNeedResync(resource, curObj, ignoreAdd) {
 			// try to update resource
 			_, lastError = dynamicClient.Resource(restMapping.Resource).Namespace(resource.GetNamespace()).
 				Update(context.TODO(), resource, metav1.UpdateOptions{})
@@ -634,14 +634,36 @@ func fieldsToBeIgnored() []string {
 		known.ManagedFields,
 		known.MetaUID,
 		known.MetaSelflink,
+		known.MetaResourceVersion,
+	}
+}
+
+func sectionToBeIgnored() []string {
+	return []string{
+		known.SectionStatus,
 	}
 }
 
+// shouldPatchBeIgnored used to decide if this patch operation should be ignored.
+func shouldPatchBeIgnored(operation jsonpatch.JsonPatchOperation) bool {
+	// some fields need to be ignore like meta.selfLink, meta.resourceVersion.
+	if ContainsString(fieldsToBeIgnored(), operation.Path) {
+		return true
+	}
+	// some sections like status section need to be ignored.
+	if ContainsPrefix(sectionToBeIgnored(), operation.Path) {
+		return true
+	}
+
+	return false
+}
+
 // ResourceNeedResync will compare fields and decide whether to sync back the current object.
 //
 // current is deployed resource, modified is changed resource.
+// ignoreAdd is true if you want to ignore add action.
 // The function will return the bool value to indicate whether to sync back the current object.
-func ResourceNeedResync(current pkgruntime.Object, modified pkgruntime.Object) bool {
+func ResourceNeedResync(current pkgruntime.Object, modified pkgruntime.Object, ignoreAdd bool) bool {
 	currentBytes, err := json.Marshal(current)
 	if err != nil {
 		klog.ErrorDepth(5, fmt.Sprintf("Error marshal json: %v", err))
@@ -661,13 +683,17 @@ func ResourceNeedResync(current pkgruntime.Object, modified pkgruntime.Object) b
 	}
 	for _, operation := range patch {
 		// filter ignored paths
-		if ContainsString(fieldsToBeIgnored(), operation.Path) {
+		if shouldPatchBeIgnored(operation) {
 			continue
 		}
 
 		switch operation.Operation {
 		case "add":
-			continue
+			if ignoreAdd {
+				continue
+			} else {
+				return true
+			}
 		case "remove", "replace":
 			return true
 		default:
diff --git a/pkg/utils/slice.go b/pkg/utils/slice.go
index addcdb9..dd839ca 100644
--- a/pkg/utils/slice.go
+++ b/pkg/utils/slice.go
@@ -18,6 +18,7 @@ package utils
 
 import (
 	"sort"
+	"strings"
 )
 
 // Copied from k8s.io/kubernetes/pkg/utils/slice/slice.go
@@ -51,6 +52,16 @@ func ContainsString(slice []string, s string) bool {
 	return false
 }
 
+// ContainsPrefix checks if a given slice of strings start with the provided string.
+func ContainsPrefix(slice []string, s string) bool {
+	for _, item := range slice {
+		if strings.HasPrefix(s, item) {
+			return true
+		}
+	}
+	return false
+}
+
 // RemoveString returns a newly created []string that contains all items from slice that
 // are not equal to s.
 func RemoveString(slice []string, s string) []string {


Test patch:
diff --git a/pkg/utils/deployer_test.go b/pkg/utils/deployer_test.go
index 6f30410..618c252 100644
--- a/pkg/utils/deployer_test.go
+++ b/pkg/utils/deployer_test.go
@@ -23,7 +23,6 @@ import (
 )
 
 func TestResourceNeedResync(t *testing.T) {
-
 	barX := &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"spec": map[string]interface{}{
@@ -103,34 +102,120 @@ func TestResourceNeedResync(t *testing.T) {
 			},
 		},
 	}
+	barU := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"spec": map[string]interface{}{
+				"externalTrafficPolicy": "Cluster",
+				"ipFamilies": []string{
+					"IPv4",
+				},
+				"ipFamilyPolicy": "SingleStack",
+				"ports": []interface{}{
+					map[string]interface{}{
+						"name":       "tcp-80-80",
+						"port":       int64(80),
+						"protocol":   "TCP",
+						"targetPort": int64(80),
+					},
+					map[string]interface{}{
+						"name":       "tcp-443-443",
+						"port":       int64(443),
+						"protocol":   "TCP",
+						"targetPort": int64(443),
+					},
+				},
+			},
+			"status": map[string]interface{}{
+				"availableReplicas":  2,
+				"observedGeneration": 1,
+			},
+		},
+	}
+	barV := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"spec": map[string]interface{}{
+				"externalTrafficPolicy": "Cluster",
+				"ipFamilies": []string{
+					"IPv4",
+				},
+				"ipFamilyPolicy": "SingleStack",
+				"ports": []interface{}{
+					map[string]interface{}{
+						"name":       "tcp-80-80",
+						"port":       int64(80),
+						"protocol":   "TCP",
+						"targetPort": int64(80),
+					},
+					map[string]interface{}{
+						"name":       "tcp-443-443",
+						"port":       int64(443),
+						"protocol":   "TCP",
+						"targetPort": int64(443),
+					},
+				},
+			},
+			"status": map[string]interface{}{
+				"availableReplicas":  2,
+				"observedGeneration": 1000,
+			},
+		},
+	}
 
 	tests := []struct {
-		label    string // Test name
-		x        *unstructured.Unstructured
-		y        *unstructured.Unstructured
-		wantSync bool   // Whether the inputs are equal
-		reason   string // The reason for the expected outcome
+		label     string // Test name
+		x         *unstructured.Unstructured
+		y         *unstructured.Unstructured
+		wantSync  bool   // Whether the inputs are equal
+		reason    string // The reason for the expected outcome
+		ignoreAdd bool   // whether or not ignore add action.
 	}{
 		{
-			label:    "fields-populated",
-			x:        barX,
-			y:        barY,
-			wantSync: false,
-			reason:   "won't re-sync because fields are auto populated",
+			label:     "fields-populated",
+			x:         barX,
+			y:         barY,
+			wantSync:  false,
+			reason:    "won't re-sync because fields are auto populated",
+			ignoreAdd: true,
+		},
+		{
+			label:     "fields-removed",
+			x:         barY,
+			y:         barX,
+			wantSync:  true,
+			reason:    "should re-sync because fields are removed",
+			ignoreAdd: false,
+		},
+		{
+			label:     "fields-changed",
+			x:         barX,
+			y:         barZ,
+			wantSync:  true,
+			reason:    "should re-sync because fields get changed",
+			ignoreAdd: false,
+		},
+		{
+			label:     "fields-added",
+			x:         barX,
+			y:         barY,
+			wantSync:  true,
+			reason:    "should re-sync because add action are not ignored",
+			ignoreAdd: false,
 		},
 		{
-			label:    "fields-removed",
-			x:        barY,
-			y:        barX,
-			wantSync: true,
-			reason:   "should re-sync because fields are removed",
+			label:     "section-ignored",
+			x:         barX,
+			y:         barU,
+			wantSync:  false,
+			reason:    "won't re-sync because status section is ignored",
+			ignoreAdd: true,
 		},
 		{
-			label:    "fields-changed",
-			x:        barX,
-			y:        barZ,
-			wantSync: true,
-			reason:   "should re-sync because fields get changed",
+			label:     "section-ignored",
+			x:         barU,
+			y:         barV,
+			wantSync:  false,
+			reason:    "won't re-sync because status section is ignored",
+			ignoreAdd: true,
 		},
 	}
 
@@ -138,7 +223,7 @@ func TestResourceNeedResync(t *testing.T) {
 		t.Run(tt.label, func(t *testing.T) {
 			var gotEqual bool
 			func() {
-				gotEqual = ResourceNeedResync(tt.x, tt.y)
+				gotEqual = ResourceNeedResync(tt.x, tt.y, tt.ignoreAdd)
 			}()
 			switch {
 			case tt.reason == "":