✨ Enable sync labels for clustermanager operator and registration controller #1021

jeffw17 · 2025-06-02T21:23:24Z

Summary

Previously, there was a feature implemented where the klusterlet resources have labels passed down to resources created indirectly by the controllers. This feature was not implemented in the clustermanager side. For example, the operator goes ahead and create the various deployments on the open-cluster-management-hub namespace and those deployments will now have the same labels as the operator. As such, the changes in this PR implements this feature. The feature in this PR however only applies to the cluster-manager operator and the registration-controller. In a future date, we will implement the same feature for the other controllers (i.e. placement-controller, work-controller, etc.). To enable this feature, in the helm charts, you can set enableSyncLabels to true or, in clusteradm commands, you can add the --enable-sync-labels=true flag.

Related issue(s)

#957

Fixes # 957

openshift-ci · 2025-06-02T21:23:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jeffw17
Once this PR has been reviewed and has the lgtm label, please assign jnpacker for approval. For more information see the 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

codecov · 2025-06-02T21:29:02Z

Codecov Report

Attention: Patch coverage is 44.94382% with 49 lines in your changes missing coverage. Please review.

Project coverage is 64.02%. Comparing base (8c49474) to head (aa86a23).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/operator/helpers/helpers.go	40.00%	20 Missing and 4 partials ⚠️
pkg/registration/hub/manager.go	27.27%	7 Missing and 1 partial ⚠️
...stermanagercontroller/clustermanager_controller.go	0.00%	4 Missing and 1 partial ⚠️
...rmanagercontroller/clustermanager_crd_reconcile.go	40.00%	2 Missing and 1 partial ⚠️
pkg/registration/helpers/helpers.go	0.00%	3 Missing ⚠️
pkg/cmd/hub/operator.go	0.00%	2 Missing ⚠️
pkg/registration/hub/clusterrole/controller.go	66.66%	2 Missing ⚠️
pkg/registration/hub/managedcluster/controller.go	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
- Coverage   64.25%   64.02%   -0.24%     
==========================================
  Files         195      196       +1     
  Lines       19118    19362     +244     
==========================================
+ Hits        12285    12397     +112     
- Misses       5823     5933     +110     
- Partials     1010     1032      +22

Flag	Coverage Δ
unit	`64.02% <44.94%> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

qiujian16 · 2025-06-03T01:56:02Z

pkg/registration/helpers/helpers.go

@@ -176,3 +179,21 @@ func IsCSRSupported(nativeClient kubernetes.Interface) (bool, bool, error) {
 	}
 	return v1CSRSupported, v1beta1CSRSupported, nil
 }
+
+func ParseLabels(labels string) map[string]string {


you could use this https://github.com/kubernetes/kubectl/blob/master/pkg/generate/generate.go#L178

do we still need this func?

pkg/registration/hub/manager.go

pkg/operator/helpers/helpers.go

...r/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go

qiujian16 · 2025-06-03T02:22:52Z

...tors/clustermanager/controllers/clustermanagercontroller/clustermanager_runtime_reconcile.go

@@ -136,6 +136,11 @@ func (c *runtimeReconcile) reconcile(ctx context.Context, cm *operatorapiv1.Clus
 		}
 	}

+	if cm.Labels != nil {


why do we need this again, I think config has been set on the controller already.

We want to sync the labels continuously. In case the labels are changed on the clustermanager, the new labels should be reflected.

hrm I do not think it will happen in reconcile, the cm is passed from the controller and it has been updated already, this will not be changed in the reconcile loop.

pkg/registration/hub/managedcluster/controller.go

qiujian16 · 2025-06-03T02:28:44Z

pkg/registration/hub/manager.go

@@ -89,6 +90,8 @@ func (m *HubManagerOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.StringSliceVar(&m.AutoApprovedARNPatterns, "auto-approved-arn-patterns", m.AutoApprovedARNPatterns,
 		"A list of AWS EKS ARN patterns such that an EKS cluster will be auto approved if its ARN matches with any of the patterns")
 	fs.StringSliceVar(&m.AwsResourceTags, "aws-resource-tags", m.AwsResourceTags, "A list of tags to apply to AWS resources created through the OCM controllers")
+	fs.StringVar(&m.Labels, "labels", m.Labels,


It is better to set it as fs.StringSliceVar, and convert it to map in complete.

If we use StringSliceVar which returns m.Labels as a []string then we won't be able to leverage generate.ParseLabels() as it expects a string to convert to map.

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

…ion test Signed-off-by: Jeffrey Wong <[email protected]>

…ble and also add labels to rbac manifests if present.

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

Signed-off-by: Jeffrey Wong <[email protected]>

qiujian16 · 2025-06-04T06:44:54Z

go.mod

-	k8s.io/client-go v0.32.4
-	k8s.io/component-base v0.32.4
+	k8s.io/client-go v0.33.1
+	k8s.io/component-base v0.33.1


please do not upgrade the library

Signed-off-by: Jeffrey Wong <[email protected]>

qiujian16 · 2025-06-05T13:41:36Z

test/integration/registration/integration_suite_test.go

-	eventuallyInterval = 1  // seconds
+	eventuallyTimeout     = 30 // seconds
+	eventuallyInterval    = 1  // seconds
+	testCustomLabel       = "custom-label"


this should not be set as global variable. It will be really hard to maintain. You should consider stop the existing hub, customize the option and restart the hub in the specific test. Example like this https://github.com/open-cluster-management-io/ocm/blob/main/test/integration/registration/spokecluster_aws_joining_test.go#L32-L47

qiujian16 · 2025-06-05T13:45:05Z

test/integration/operator/integration_suite_test.go

@@ -129,6 +129,11 @@ var _ = ginkgo.BeforeSuite(func() {
 	_, err = operatorClient.OperatorV1().ClusterManagers().Create(context.Background(), &operatorapiv1.ClusterManager{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: clusterManagerName,
+			Labels: map[string]string{
+				"app":      "clustermanager",


same here, consider a test that update the clustermanager, and check that the label is updated on the resource instead of setting it globally.

…abels during the test Signed-off-by: suvaanshkumar <[email protected]>

openshift-ci bot requested review from haowells and ldpliu June 2, 2025 21:23

qiujian16 reviewed Jun 3, 2025

View reviewed changes

amrcoder and others added 23 commits June 3, 2025 17:39

Adding enableSyncLabels in hub operator, clustermanagercontroller

f2cc475

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

Make changes to deployment yamls to accept labels and create integrat…

feffe1e

…ion test Signed-off-by: Jeffrey Wong <[email protected]>

Adding changes for registration controller

18b25d6

feat: make changes in clusterrole controller to pass labels if availa…

b021e32

…ble and also add labels to rbac manifests if present.

adding test for adding labels and parsing labels

42315f0

Adding unit test for MC controller

e424812

adding integration tests and adding labels

8e4ac12

adding assertions to check labels in work role and registration role

03d571d

Adding labels to missing resources

926ee57

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

Testing enable sync labels

20cd51b

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

making all labels as string

0f84115

adding enablesynclabels to chart config

54470bf

setting default value of enable sync labels to false

5a7b375

Adding labels in string format

4ee3abe

fixing test cases

d7d595b

trying to fix integration tests

38a79d6

Signed-off-by: Jeffrey Wong <[email protected]>

remove print statements

155eb0a

Signed-off-by: Jeffrey Wong <[email protected]>

feat: adding debug statements for testing.

2cf9e06

fix integration test

0555b9e

Signed-off-by: Jeffrey Wong <[email protected]>

remove print statements

f107be8

Signed-off-by: Jeffrey Wong <[email protected]>

clean up files changed for testing

ba103de

Signed-off-by: Jeffrey Wong <[email protected]>

fix managedcluster accept test error formatting

78f2194

Signed-off-by: Jeffrey Wong <[email protected]>

Addressed review comments

5efb103

suvaanshkumar force-pushed the test-labels-sync-e2e branch from 857d79d to 5efb103 Compare June 3, 2025 21:39

qiujian16 reviewed Jun 4, 2025

View reviewed changes

jeffw17 added 3 commits June 4, 2025 10:25

Update vendor

a3cecc0

Signed-off-by: Jeffrey Wong <[email protected]>

Fix tests and formatting imports

772544e

Signed-off-by: Jeffrey Wong <[email protected]>

Remove label from crd template and add labels after creating crd asset

933d328

Signed-off-by: Jeffrey Wong <[email protected]>

jeffw17 force-pushed the test-labels-sync-e2e branch 2 times, most recently from b802ec7 to ce7b700 Compare June 5, 2025 01:01

Fix verify and unit tests

a5c33be

Signed-off-by: Jeffrey Wong <[email protected]>

jeffw17 force-pushed the test-labels-sync-e2e branch from ce7b700 to a5c33be Compare June 5, 2025 01:23

qiujian16 reviewed Jun 5, 2025

View reviewed changes

suvaanshkumar force-pushed the test-labels-sync-e2e branch from 41a2367 to 74e01d5 Compare June 5, 2025 21:27

Update test case to update the hubmanageroptions and clustermanager l…

aa86a23

…abels during the test Signed-off-by: suvaanshkumar <[email protected]>

suvaanshkumar force-pushed the test-labels-sync-e2e branch from 74e01d5 to aa86a23 Compare June 5, 2025 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

✨ Enable sync labels for clustermanager operator and registration controller #1021

✨ Enable sync labels for clustermanager operator and registration controller #1021

Uh oh!

jeffw17 commented Jun 2, 2025

Uh oh!

openshift-ci bot commented Jun 2, 2025

Uh oh!

codecov bot commented Jun 2, 2025 •

edited

Loading

Uh oh!

qiujian16 Jun 3, 2025

Uh oh!

qiujian16 Jun 5, 2025

Uh oh!

suvaanshkumar Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiujian16 Jun 3, 2025

Uh oh!

jeffw17 Jun 4, 2025

Uh oh!

qiujian16 Jun 6, 2025

Uh oh!

Uh oh!

qiujian16 Jun 3, 2025

Uh oh!

jeffw17 Jun 4, 2025

Uh oh!

qiujian16 Jun 4, 2025

Uh oh!

qiujian16 Jun 5, 2025

Uh oh!

qiujian16 Jun 5, 2025

Uh oh!

Uh oh!

✨ Enable sync labels for clustermanager operator and registration controller #1021

Are you sure you want to change the base?

✨ Enable sync labels for clustermanager operator and registration controller #1021

Uh oh!

Conversation

jeffw17 commented Jun 2, 2025

Summary

Related issue(s)

Uh oh!

openshift-ci bot commented Jun 2, 2025

Uh oh!

codecov bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 2, 2025 •

edited

Loading