support hubRegistrationFeatureGates and spokeRegistrationFeatureGates #230

ivan-cai · 2022-04-01T09:21:27Z

This PR is depend on the PR in api open-cluster-management-io/api#149, must modifying go.mod and updating vendor afeter api pr 149 is merged

reconcile hubRegistrationFeatureGates and spokeRegistrationFeatureGates to registration deployment()

Signed-off-by: ivan-cai caijing.cai@alibaba-inc.com

qiujian16 · 2022-05-19T02:33:28Z

pkg/helpers/helpers.go

+
+func FeatureGatesArgs(featureGates []string) []string {
+	var featureGatesArgs []string
+	for _, featureGate := range featureGates {


let's filter out the unknown features and output an error condition if some features are not correct.

qiujian16

@ivan-cai would you like to rebase this PR thanks!

ivan-cai · 2022-06-06T09:40:24Z

@ivan-cai would you like to rebase this PR thanks!
ok👌🏻

ivan-cai · 2022-06-07T07:00:43Z

pkg/helpers/helpers.go

+	ComponentHubKey   = "hub"
+	ComponentSpokeKey = "spoke"
+
+	ClusterClaim               featuregate.Feature = "ClusterClaim"


@qiujian16 We need know registration's featrue gates. So should we import registration project in registration-operator to use constants or variables in open-cluster-management-io/registration/pkg/features/feature.go? Or, we write these constants or variables to open-cluster-management-io/api, then, we import api project in registration and registration-operator?
And, registration project can not go get.

I think we put those in api repo

@qiujian16 ok, I would move open-cluster-management-io/registration/pkg/features/feature.go to open-cluster-management-io/api/operator/v1/feature.go. what do you think?

I commit a pr in api repo for this: open-cluster-management-io/api#162

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai · 2022-06-09T04:08:17Z

@qiujian16 Plz review again. I have changed to use api repo's variables DefaultHubRegistrationFeatureGates & DefaultSpokeRegistrationFeatureGates to check registration if valid.

ivan-cai · 2022-06-10T02:38:51Z

@qiujian16 Plz review again for registration-operator repo

…tionFeatureGates in api repo to check registration variable if valid Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

qiujian16

I notice that with this change we will disable addonmanaged and defaultClusterset feature by default. It will bring some backward compatible problem. How about we keep the original setting if RegistrationConfiguration is nil?

qiujian16 · 2022-06-13T01:44:01Z

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

+	if clusterManager.Spec.RegistrationConfiguration != nil && len(clusterManager.Spec.RegistrationConfiguration.FeatureGates) > 0 {
+		featureGateArgs, invalidFeatureGates := helpers.FeatureGatesArgs(clusterManager.Spec.RegistrationConfiguration.FeatureGates, helpers.ComponentHubKey)
+		if len(invalidFeatureGates) == 0 {
+			config.RegistrationFeatureGates = featureGateArgs


I think you would want to set the same condition to true when there is no invalid feature gates.

Otherwise, we will always see false condition if user change the feature gates to correct

I notice that with this change we will disable addonmanaged and defaultClusterset feature by default. It will bring some backward compatible problem. How about we keep the original setting if RegistrationConfiguration is nil?

Yes, you are right. I have fixed.

I think you would want to set the same condition to true when there is no invalid feature gates.

Otherwise, we will always see false condition if user change the feature gates to correct

ok

nit comment. Other looks good.

/approve

I think we should add some integration tests to check whether the condition is returned correctly with invalid features. Not a blocker for this one, but could be in a followup PR,

👌🏻

qiujian16 · 2022-06-13T01:45:12Z

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

+				Type: hubRegistrationFeatureGatesInvalid, Status: metav1.ConditionFalse, Reason: "InvalidFeatureGatesExisting",
+				Message: invalidFGErr.Error(),
+			}))
+			return invalidFGErr


I think we only return updateErr here. It is not necessary to return invalidErr, since backoff does not help here, right? It may need some doc here also.

I think we only return updateErr here. It is not necessary to return invalidErr, since backoff does not help here, right? It may need some doc here also.

👌🏻

…onfiguration is nil Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai · 2022-06-14T02:50:59Z

manifests/klusterlet/management/klusterlet-registration-deployment.yaml

@@ -47,7 +47,13 @@ spec:
          - "agent"
          - "--cluster-name={{ .ClusterName }}"
          - "--bootstrap-kubeconfig=/spoke/bootstrap/kubeconfig"
+          {{ if gt (len .RegistrationFeatureGates) 0 }}


@qiujian16 I have check len(RegistrationFeatureGates) ,if equal 0, using original setting

ivan-cai · 2022-06-14T02:52:26Z

@qiujian16 I have changed by your advices, plz review again.

qiujian16

nit comment. Other looks good.

/approve

I think we should add some integration tests to check whether the condition is returned correctly with invalid features. Not a blocker for this one, but could be in a followup PR,

qiujian16 · 2022-06-14T10:31:11Z

deploy/cluster-manager/config/samples/operator_open-cluster-management_clustermanagers.cr.yaml

+  registrationConfiguration:
+    featureGates:
+    - feature: DefaultClusterSet
+      mode: Enable


qiujian16 · 2022-06-14T10:31:17Z

deploy/klusterlet/config/samples/operator_open-cluster-management_klusterlets.cr.yaml

+  registrationConfiguration:
+    featureGates:
+    - feature: AddonManagement
+      mode: Enable


openshift-ci · 2022-06-14T10:35:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivan-cai, qiujian16

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [qiujian16]

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

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

openshift-ci · 2022-06-14T11:39:25Z

@ivan-cai: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-06-14T11:40:05Z

@ivan-cai: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ivan-cai · 2022-06-14T11:41:56Z

@qiujian16 I have fix some errors in unit test, plz review again. Thank you very much!

qiujian16 · 2022-06-14T14:46:58Z

@ivan-cai two small nit comment. PTAL again and we are good to go! Thanks!

ivan-cai · 2022-06-15T02:36:36Z

@qiujian16 Yes, I see, I will add more integration tests in a followup PR. Could you merge this PR?

qiujian16 · 2022-06-15T08:48:40Z

ah, sorry, I think I forgot to push the comment...I think you would also need to add default feature gates for registration webhooks and also this one #230 (comment).

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai · 2022-06-22T05:00:05Z

ah, sorry, I think I forgot to push the comment...I think you would also need to add default feature gates for registration webhooks and also this one #230 (comment).

@qiujian16 I have add feature gates for egistration webhook, plz review again.

qiujian16 · 2022-06-22T06:45:47Z

/lgtm

FYI @zhiweiyin318 I think this pr is pretty careful to avoid any regression failure, but we still need to monitor it

openshift-ci bot requested review from mdelder and zhiweiyin318 April 1, 2022 09:21

ivan-cai mentioned this pull request Apr 1, 2022

support Registration FeatureGates to pass feature gates(e.g. V1beta1CSRAPICompatibility) to registration open-cluster-management-io/api#149

Merged

ivan-cai force-pushed the support_registration_featuregates branch from 02056ec to b8a1d95 Compare April 1, 2022 09:25

openshift-ci bot added the needs-rebase label May 10, 2022

qiujian16 reviewed May 19, 2022

View reviewed changes

ivan-cai force-pushed the support_registration_featuregates branch from b8a1d95 to dd81e89 Compare June 7, 2022 06:42

openshift-ci bot removed the needs-rebase label Jun 7, 2022

ivan-cai force-pushed the support_registration_featuregates branch 5 times, most recently from f5443c6 to 2f617c3 Compare June 7, 2022 06:53

ivan-cai commented Jun 7, 2022

View reviewed changes

ivan-cai mentioned this pull request Jun 8, 2022

move registration feature gates to api, can be used by registration-o… open-cluster-management-io/api#162

Merged

ivan-cai force-pushed the support_registration_featuregates branch from 2f617c3 to d3f1932 Compare June 8, 2022 03:26

support RegistrationFeatureGates in ClusterManager and Klusterlet

a0c9a3b

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai force-pushed the support_registration_featuregates branch from d3f1932 to a0c9a3b Compare June 8, 2022 04:11

update to use DefaultHubRegistrationFeatureGates&DefaultSpokeRegistra…

456389c

…tionFeatureGates in api repo to check registration variable if valid Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai force-pushed the support_registration_featuregates branch from cf83db8 to 456389c Compare June 13, 2022 03:13

ivan-cai requested a review from qiujian16 June 13, 2022 03:22

qiujian16 reviewed Jun 13, 2022

View reviewed changes

enable addonmanagement and defaultClusterset feature if RegistrationC…

9d957ce

…onfiguration is nil Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

ivan-cai force-pushed the support_registration_featuregates branch from 8b368b0 to 9d957ce Compare June 14, 2022 02:47

ivan-cai commented Jun 14, 2022

View reviewed changes

qiujian16 reviewed Jun 14, 2022

View reviewed changes

openshift-ci bot added the approved label Jun 14, 2022

fix some errors in unit test

dca5155

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

add RegistrationFeatureGates to registration webhook

2981b12

Signed-off-by: ivan-cai <caijing.cai@alibaba-inc.com>

openshift-ci bot assigned qiujian16 Jun 22, 2022

openshift-ci bot added the lgtm label Jun 22, 2022

openshift-ci bot merged commit 3401c8e into open-cluster-management-io:main Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support hubRegistrationFeatureGates and spokeRegistrationFeatureGates #230

support hubRegistrationFeatureGates and spokeRegistrationFeatureGates #230

ivan-cai commented Apr 1, 2022 •

edited

qiujian16 May 19, 2022

qiujian16 left a comment

ivan-cai commented Jun 6, 2022

ivan-cai Jun 7, 2022

qiujian16 Jun 7, 2022

ivan-cai Jun 8, 2022 •

edited

ivan-cai commented Jun 9, 2022

ivan-cai commented Jun 10, 2022

qiujian16 left a comment

qiujian16 Jun 13, 2022

ivan-cai Jun 14, 2022

ivan-cai Jun 14, 2022

ivan-cai Jun 14, 2022

qiujian16 Jun 13, 2022

ivan-cai Jun 14, 2022

ivan-cai Jun 14, 2022

ivan-cai commented Jun 14, 2022

qiujian16 left a comment

qiujian16 Jun 14, 2022

qiujian16 Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

ivan-cai commented Jun 14, 2022

qiujian16 commented Jun 14, 2022

ivan-cai commented Jun 15, 2022

qiujian16 commented Jun 15, 2022 •

edited

ivan-cai commented Jun 22, 2022

qiujian16 commented Jun 22, 2022

support hubRegistrationFeatureGates and spokeRegistrationFeatureGates #230

support hubRegistrationFeatureGates and spokeRegistrationFeatureGates #230

Conversation

ivan-cai commented Apr 1, 2022 • edited

Choose a reason for hiding this comment

qiujian16 left a comment

Choose a reason for hiding this comment

ivan-cai commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-cai Jun 8, 2022 • edited

Choose a reason for hiding this comment

ivan-cai commented Jun 9, 2022

ivan-cai commented Jun 10, 2022

qiujian16 left a 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

Choose a reason for hiding this comment

ivan-cai commented Jun 14, 2022

qiujian16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

ivan-cai commented Jun 14, 2022

qiujian16 commented Jun 14, 2022

ivan-cai commented Jun 15, 2022

qiujian16 commented Jun 15, 2022 • edited

ivan-cai commented Jun 22, 2022

qiujian16 commented Jun 22, 2022

ivan-cai commented Apr 1, 2022 •

edited

ivan-cai Jun 8, 2022 •

edited

qiujian16 commented Jun 15, 2022 •

edited