Add feature gate for disabling the MHC controller #1151

slintes · 2023-07-10T17:17:49Z

There is a plan to let the Node Healthcheck Operator, which is an optional day 2 operator today, also implement the MHC controller. For being able to test the POC in NHC, we need to be able to disable MAO's MHC controller. So for now this feature gate is just for dev usage. The actual transition of the MHC controller from MAO to NHC will be discussed and planned in the future.

ECOPROJECT-1423

There is a plan to let the Node Healthcheck Operator, which is an optional day 2 operator today, also implement the MHC controller. For being able to test the POC in NHC, we need to be able to disable MAO's MHC controller. So for now this feature gate is just for dev usage. The actual transition of the MHC controller from MAO to NHC will be discussed and planned in the future. Signed-off-by: Marc Sluiter <msluiter@redhat.com>

slintes · 2023-07-10T17:18:58Z

pkg/operator/operator.go

@@ -382,7 +369,7 @@ func (optr *Operator) maoConfigFromInfrastructure() (*OperatorConfig, error) {
 		return nil, err
 	}

-	featureGate, err := getFeatureGate(optr.featureGateLister)
+	featureGate, err := optr.osClient.ConfigV1().FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{})


I didn't understand why tbh, but the existing getFeatureGate() function didn't work in the unit test at least

By using the direct client you're skipping the caching that should be happening as part of the informer. Looking at this, we are using the old way of listing feature gates here, we should update it to use the new feature gate access.

You can see how that works for openshift/cluster-cloud-controller-manager-operator#249

Would be good to update to use this new method here now.

Especially as the way you're calculating whether this feature should be on/off is a little unconventional

slintes · 2023-07-10T17:19:18Z

pkg/operator/operator_test.go

@@ -32,10 +33,9 @@ import (
 )

 const (
-	deploymentName   = "machine-api-controllers"
-	targetNamespace  = "test-namespace"
-	hcControllerName = "machine-healthcheck-controller"


JoelSpeed · 2023-07-10T17:44:07Z

pkg/operator/operator.go

@@ -382,7 +369,7 @@ func (optr *Operator) maoConfigFromInfrastructure() (*OperatorConfig, error) {
 		return nil, err
 	}

-	featureGate, err := getFeatureGate(optr.featureGateLister)
+	featureGate, err := optr.osClient.ConfigV1().FeatureGates().Get(context.Background(), "cluster", metav1.GetOptions{})


By using the direct client you're skipping the caching that should be happening as part of the informer. Looking at this, we are using the old way of listing feature gates here, we should update it to use the new feature gate access.

You can see how that works for openshift/cluster-cloud-controller-manager-operator#249

Would be good to update to use this new method here now.

Especially as the way you're calculating whether this feature should be on/off is a little unconventional

JoelSpeed · 2023-07-10T17:44:34Z

pkg/util/featuregates/featuregates.go

+
+const (
+	// DeployMHCControllerFeatureGateName is the name of the feature gate for enabling the MHC controller
+	DeployMHCControllerFeatureGateName = "MachineAPIOperatorDeployMHCController"


Feature gates need to live in config/v1 in openshift/api

JoelSpeed · 2023-07-10T17:46:07Z

pkg/util/featuregates/featuregates.go

+func IsDeployMHCControllerEnabled(fg *v1.FeatureGate) bool {
+	deployMHCControllerFeatureGate := v1.FeatureGateName(DeployMHCControllerFeatureGateName)
+	if fg != nil && fg.Spec.CustomNoUpgrade != nil {
+		for _, enabled := range fg.Spec.CustomNoUpgrade.Enabled {
+			if enabled == deployMHCControllerFeatureGate {
+				klog.V(2).Info("MHC controller enabled by feature gate")
+				return true
+			}
+		}
+		for _, disabled := range fg.Spec.CustomNoUpgrade.Disabled {
+			if disabled == deployMHCControllerFeatureGate {
+				klog.V(2).Info("MHC controller disabled by feature gate")
+				return false
+			}
+		}
+	}
+	// switch to false once NHC is the default!
+	klog.V(4).Info("MHC controller enabled (default)")
+	return true
+}


The featuregateaccess pattern gives you a way to check if a specific feature gate is enabled or not. We should be using that rather than using our own logic, it helps to co-ordinate between multiple components and versions. I realise we are doing this for testing initially but I suspect at some point we may end up evolving this so we should probably get it right first time.

You can read more in this FAQ

Signed-off-by: Marc Sluiter <msluiter@redhat.com>

slintes · 2023-07-12T19:17:30Z

install/0000_30_machine-api-operator_09_rbac.yaml

@@ -314,6 +314,24 @@ rules:
      - patch
      - delete

+  - apiGroups:


needed for the getting the controller ref for the recorder for the feature gate accessor...

slintes · 2023-07-12T19:17:46Z

install/0000_30_machine-api-operator_11_deployment.yaml

@@ -68,6 +68,10 @@ spec:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
+        - name: POD_NAME


needed for the getting the controller ref for the recorder for the feature gate accessor...

slintes · 2023-07-12T19:24:31Z

I finally got the feature gate accessor working, thanks for the links to the FAQ and the cluster-cloud-controller-manager-operator PR, that was helpful. Successfully tested manually using OCP 4.14.0-0.ci-2023-07-12-081430 as base.

Just for reference, here the related PRs for adding the feature gate to the api and cluster config operator:
openshift/api#1523
openshift/cluster-config-operator#327

slintes · 2023-07-12T19:29:18Z

/retest

openshift-ci · 2023-07-12T22:22:57Z

@slintes: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-operator	`ce3fe45`	link	true	`/test e2e-aws-operator`
ci/prow/e2e-nutanix	`ce3fe45`	link	false	`/test e2e-nutanix`
ci/prow/e2e-vsphere-ovn-upgrade	`ce3fe45`	link	false	`/test e2e-vsphere-ovn-upgrade`
ci/prow/e2e-gcp-ovn	`ce3fe45`	link	false	`/test e2e-gcp-ovn`
ci/prow/e2e-gcp-operator	`ce3fe45`	link	false	`/test e2e-gcp-operator`
ci/prow/e2e-vsphere-ovn	`ce3fe45`	link	false	`/test e2e-vsphere-ovn`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

JoelSpeed

One quick question otherwise LGTM assuming we can get the E2E green

JoelSpeed · 2023-07-13T08:48:53Z

pkg/operator/operator.go

@@ -39,6 +41,9 @@ const (
 	// 5ms, 10ms, 20ms, 40ms, 80ms, 160ms, 320ms, 640ms, 1.3s, 2.6s, 5.1s, 10.2s, 20.4s, 41s, 82s
 	maxRetries         = 15
 	maoOwnedAnnotation = "machine.openshift.io/owned"
+
+	releaseVersionEnvVariableName = "RELEASE_VERSION"
+	releaseVersionUnknownValue    = "unknown"


Is this the same value as the raw version string when it's not initialised?

I didn't understand how/when this is used tbh, I copied it from here: https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/43facae32a53c73ad567129150db6865673bdf1d/pkg/controllers/status.go#L224

Ok, I think it's fine to leave as that for now, lets see if anything breaks!

slintes · 2023-07-13T09:27:45Z

several e2e tests failed with an etcd issue, let's try again before digging more into it...

/retest

JoelSpeed · 2023-07-13T09:37:38Z

/approve
/lgtm

openshift-ci · 2023-07-13T09:38:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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~~ [JoelSpeed]

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

openshift-ci-robot · 2023-07-13T10:28:49Z

/retest-required

Remaining retests: 0 against base HEAD 8f0f041 and 2 for PR HEAD ce3fe45 in total

slintes · 2023-07-14T11:12:57Z

/retest

slintes commented Jul 10, 2023

View reviewed changes

openshift-ci bot requested review from elmiko and JoelSpeed July 10, 2023 17:19

slintes commented Jul 10, 2023

View reviewed changes

JoelSpeed reviewed Jul 10, 2023

View reviewed changes

slintes added 3 commits July 11, 2023 12:14

Update openshift/api for new feature gate

c1d10d5

Signed-off-by: Marc Sluiter <msluiter@redhat.com>

Update openshift/library for feature gate accessor

772a8b0

Signed-off-by: Marc Sluiter <msluiter@redhat.com>

Use feature gate accessor

ce3fe45

Signed-off-by: Marc Sluiter <msluiter@redhat.com>

slintes commented Jul 12, 2023

View reviewed changes

JoelSpeed reviewed Jul 13, 2023

View reviewed changes

openshift-ci bot assigned JoelSpeed Jul 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2023

slintes mentioned this pull request Jul 14, 2023

MHC support medik8s/node-healthcheck-operator#224

Merged

5 tasks

openshift-merge-robot merged commit 62edc82 into openshift:master Jul 14, 2023
22 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature gate for disabling the MHC controller #1151

Add feature gate for disabling the MHC controller #1151

slintes commented Jul 10, 2023 •

edited by openshift-ci bot

slintes Jul 10, 2023 •

edited

JoelSpeed Jul 10, 2023

slintes Jul 10, 2023

JoelSpeed Jul 10, 2023

JoelSpeed Jul 10, 2023

JoelSpeed Jul 10, 2023

slintes Jul 12, 2023

slintes Jul 12, 2023

slintes commented Jul 12, 2023

slintes commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

JoelSpeed left a comment

JoelSpeed Jul 13, 2023

slintes Jul 13, 2023

JoelSpeed Jul 13, 2023

slintes commented Jul 13, 2023

JoelSpeed commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

openshift-ci-robot commented Jul 13, 2023

slintes commented Jul 14, 2023

Add feature gate for disabling the MHC controller #1151

Add feature gate for disabling the MHC controller #1151

Conversation

slintes commented Jul 10, 2023 • edited by openshift-ci bot

slintes Jul 10, 2023 • edited

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

slintes commented Jul 12, 2023

slintes commented Jul 12, 2023

openshift-ci bot commented Jul 12, 2023

JoelSpeed 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

slintes commented Jul 13, 2023

JoelSpeed commented Jul 13, 2023

openshift-ci bot commented Jul 13, 2023

openshift-ci-robot commented Jul 13, 2023

slintes commented Jul 14, 2023

slintes commented Jul 10, 2023 •

edited by openshift-ci bot

slintes Jul 10, 2023 •

edited