fix cluster labels update conflict. #287

morvencao · 2022-11-22T15:01:27Z

fix cluster labels update conflict:

E1122 13:49:38.188870       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-4/work-manager", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-4": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.423603       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-3/governance-policy-framework", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-3": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.438568       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-1/config-policy-controller", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-1": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.451226       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-3/config-policy-controller", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-3": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.460923       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-1/config-policy-controller", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-1": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.477055       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-1/governance-policy-framework", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-1": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.488734       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-3", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-3": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.503787       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-1/governance-policy-framework", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-1": the object has been modified; please apply your changes to the latest version and try again
E1122 13:49:45.511774       1 base_controller.go:270] "AddOnFeatureDiscoveryController" controller failed to sync "ocm-controlplane-1-mc-3", err: Operation cannot be fulfilled on managedclusters.cluster.open-cluster-management.io "ocm-controlplane-1-mc-3": the object has been modified; please apply your changes to the latest version and try again

Signed-off-by: morvencao [email protected]

openshift-ci · 2022-11-22T15:01:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: morvencao
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval by writing /assign @qiujian16 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zhiweiyin318 · 2022-11-22T15:09:34Z

pkg/hub/addon/discovery_controller.go

 		return nil
 	}

-	// otherwise, update cluster
-	_, err = c.clusterClient.ClusterV1().ManagedClusters().Update(ctx, cluster, metav1.UpdateOptions{})
+	oldData, err := json.Marshal(clusterv1.ManagedCluster{


maybe we could only build the patch like

registration/pkg/hub/managedcluster/controller.go

Line 100 in 18e7982

patch := fmt.Sprintf("{\"metadata\": {\"finalizers\": %s}}", string(finalizerBytes))

and the unit tests need to be updated too

morvencao · 2022-11-22T15:14:59Z

/hold

skeeey · 2022-11-23T01:43:26Z

pkg/hub/addon/discovery_controller.go

-	// no work if the cluster labels have no change
-	if !modified {
-		return nil
-	}


How about keeping the modified check, after the modified check, we create patchBytes and apply the patch, thus we can avoid to path the same thing

added label equality check, if they are equal, no need to patch.

skeeey · 2022-11-23T01:43:28Z

pkg/hub/addon/discovery_controller.go

-		return nil
-	}
+	// patch the cluster labels
+	_, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "")


nit: we can omit the empty string "" at the end of Patch function

nice catch! done.

skeeey · 2022-11-23T01:43:34Z

pkg/hub/addon/discovery_controller.go

-		return nil
-	}
+	// patch the cluster labels
+	_, err = c.clusterClient.ClusterV1().ManagedClusters().Patch(ctx, cluster.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "")


nit: we can omit the empty string "" at the end of Patch function

skeeey · 2022-11-23T01:43:50Z

pkg/hub/addon/discovery_controller.go

-	resourcemerge.MergeMap(&modified, &cluster.Labels, addOnLabels)
-
-	// no work if the cluster labels have no change
-	if !modified {


How about keeping the modified check, after the modified check, we create patchBytes and apply the patch, thus we can avoid to path the same thing

added label equality check, if they are equal, no need to patch.

morvencao · 2022-11-23T03:32:18Z

is the integration test failure relevant?

morvencao · 2022-11-23T03:34:15Z

/hold cancel

skeeey · 2022-11-23T04:03:57Z

it seems not, restart the test to have a try

Signed-off-by: morvencao <[email protected]>

qiujian16 · 2022-11-23T09:45:50Z

pkg/hub/addon/discovery_controller.go

 	addOn, err := c.addOnLister.ManagedClusterAddOns(clusterName).Get(addOnName)
 	switch {
 	case errors.IsNotFound(err):
 		// addon is deleted
-		key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName)
-		labels[key] = ""
+		key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)


nit I think you can build key apriori instead of in each case

qiujian16 · 2022-11-23T09:46:23Z

pkg/hub/addon/discovery_controller.go

-		key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName)
-		labels[key] = ""
+		key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
+		delete(labels, key)


should this be labels[key] = nil?

it will not work since labels are known type(map[string]string), set value to nil, it will complain;

pkg/hub/addon/discovery_controller.go:144:17: cannot use nil as string value in assignment pkg/hub/addon/discovery_controller.go:148:17: cannot use nil as string value in assignment pkg/hub/addon/discovery_controller.go:219:23: cannot use nil as string value in assignment pkg/hub/addon/discovery_controller.go:234:23: cannot use nil as string value in assignment

qiujian16 · 2022-11-23T09:46:35Z

pkg/hub/addon/discovery_controller.go

-		key := fmt.Sprintf("%s%s-", addOnFeaturePrefix, addOnName)
-		labels[key] = ""
+		key := fmt.Sprintf("%s%s", addOnFeaturePrefix, addOnName)
+		delete(labels, key)


should this be labels[key] = nil?

qiujian16 · 2022-11-23T09:53:07Z

pkg/hub/taint/controller.go

+			newTaints = nil
+		}
+		// build cluster taints patch
+		patchBytes, err := json.Marshal(map[string]interface{}{


it is better to add uid/resourceversion is precond.

added uid/resourceversion as precond

qiujian16 · 2022-11-23T09:54:08Z

pkg/hub/addon/discovery_controller.go

@@ -110,48 +112,68 @@ func (c *addOnFeatureDiscoveryController) sync(ctx context.Context, syncCtx fact
 func (c *addOnFeatureDiscoveryController) syncAddOn(ctx context.Context, clusterName, addOnName string) error {
 	klog.V(4).Infof("Reconciling addOn %q", addOnName)

-	labels := map[string]string{}


I do not remember why we need syncAddOn, seems to me syncCluster is sufficient...

From integration testing, the syncAddOn is covered, so it is still being used?

qiujian16 · 2022-11-23T09:54:53Z

pkg/hub/addon/discovery_controller.go

 		// addon is deleting
 		if !addOn.DeletionTimestamp.IsZero() {
+			delete(addOnLabels, key)


set to nil if the key exists

qiujian16 · 2022-11-23T09:55:04Z

pkg/hub/addon/discovery_controller.go

-		if _, ok := addOnLabels[key]; !ok {
-			addOnLabels[fmt.Sprintf("%s-", key)] = ""
+		if _, ok := newAddonLabels[key]; !ok {
+			delete(addOnLabels, key)


set to nil if the key exists

Signed-off-by: morvencao <[email protected]>

morvencao · 2022-11-25T08:02:24Z

@qiujian16 Another look? all comments addressed except the one set to nil for label key, because labels is known type(map[string]string), set value to nil, it will complain:

pkg/hub/addon/discovery_controller.go:144:17: cannot use nil as string value in assignment
pkg/hub/addon/discovery_controller.go:148:17: cannot use nil as string value in assignment
pkg/hub/addon/discovery_controller.go:219:23: cannot use nil as string value in assignment
pkg/hub/addon/discovery_controller.go:234:23: cannot use nil as string value in assignment

qiujian16 · 2022-11-26T01:42:48Z

you cannot set nil in map[string]string, but you can do it in map[string]interface{}. The patch you generate should be based on a map[string]interface{}.

morvencao · 2022-11-26T11:54:07Z

Any benefit to use map[string]interface{} instead of map[string]string?
In labels update case, the labels field is known type, and the JSON encoding can also generate patch bytes based on map[string]string type. We can convert map[string]string to map[string]interface{}, but that will also make it more complicated, especially we need to check whether labels are actually updated before launch patch request.
I tried to use map[string]interface{}, it is more complicated and the UT cases are also need to updated accordingly.

qiujian16 · 2022-11-28T02:02:46Z

you can delete a key with map[string]interface{}

openshift-ci bot requested review from deads2k and skeeey November 22, 2022 15:01

zhiweiyin318 reviewed Nov 22, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Nov 22, 2022

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch 2 times, most recently from 058c319 to af19cfd Compare November 22, 2022 16:08

skeeey reviewed Nov 23, 2022

View reviewed changes

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch 4 times, most recently from 1ed4b3c to 3c8be77 Compare November 23, 2022 03:22

openshift-ci bot removed the do-not-merge/hold label Nov 23, 2022

fix cluster labels update conflict.

004c7cb

Signed-off-by: morvencao <[email protected]>

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch from 3c8be77 to 004c7cb Compare November 23, 2022 06:08

fix update cluster taints conflict.

3028100

Signed-off-by: morvencao <[email protected]>

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch 2 times, most recently from 9c6ee0c to a49c139 Compare November 23, 2022 07:30

fix cluster labels patch format.

54a22eb

Signed-off-by: morvencao <[email protected]>

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch from a49c139 to 54a22eb Compare November 23, 2022 08:37

qiujian16 reviewed Nov 23, 2022

View reviewed changes

address comments.

5dbc6af

Signed-off-by: morvencao <[email protected]>

morvencao force-pushed the br_fix_cluster_labels_update_confliction branch from d42d27e to 5dbc6af Compare November 23, 2022 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix cluster labels update conflict. #287

fix cluster labels update conflict. #287

morvencao commented Nov 22, 2022

openshift-ci bot commented Nov 22, 2022

zhiweiyin318 Nov 22, 2022

zhiweiyin318 Nov 22, 2022

morvencao Nov 23, 2022

morvencao commented Nov 22, 2022

skeeey Nov 23, 2022

morvencao Nov 23, 2022

skeeey Nov 23, 2022

morvencao Nov 23, 2022

skeeey Nov 23, 2022

morvencao Nov 23, 2022

skeeey Nov 23, 2022

morvencao Nov 23, 2022

morvencao commented Nov 23, 2022

morvencao commented Nov 23, 2022

skeeey commented Nov 23, 2022

qiujian16 Nov 23, 2022

morvencao Nov 23, 2022

qiujian16 Nov 23, 2022

morvencao Nov 23, 2022 •

edited

Loading

qiujian16 Nov 23, 2022

qiujian16 Nov 23, 2022

morvencao Nov 23, 2022

qiujian16 Nov 23, 2022

morvencao Nov 23, 2022

qiujian16 Nov 23, 2022

qiujian16 Nov 23, 2022

morvencao commented Nov 25, 2022 •

edited

Loading

qiujian16 commented Nov 26, 2022

morvencao commented Nov 26, 2022 •

edited

Loading

qiujian16 commented Nov 28, 2022

fix cluster labels update conflict. #287

Are you sure you want to change the base?

fix cluster labels update conflict. #287

Conversation

morvencao commented Nov 22, 2022

openshift-ci bot commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morvencao commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morvencao commented Nov 23, 2022

morvencao commented Nov 23, 2022

skeeey commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morvencao Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morvencao commented Nov 25, 2022 • edited Loading

qiujian16 commented Nov 26, 2022

morvencao commented Nov 26, 2022 • edited Loading

qiujian16 commented Nov 28, 2022

morvencao Nov 23, 2022 •

edited

Loading

morvencao commented Nov 25, 2022 •

edited

Loading

morvencao commented Nov 26, 2022 •

edited

Loading