API-1835: Migrate a few existing controllers to ApplyOperatorStatus #698

bertinatto · 2024-09-26T17:04:27Z

In this PR I'm focusing on converting UpdateOperatorStatus to ApplyOperatorSTatus and measuring the overhead. There are several usages of Update and UpdateStatus that needs to be looked into, but I'll likely leave that for a second PR.

Verb usage beween e2e-operator job from this PR and the same job from #704:

openshift-ci-robot · 2024-09-26T17:04:30Z

@bertinatto: This pull request references API-1835 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

bertinatto · 2024-09-27T19:39:31Z

/test e2e-operator

bertinatto · 2024-09-27T19:43:53Z

/test e2e-operator

pkg/controllers/common/conditions.go

pkg/controllers/readiness/wellknown_ready_controller.go

bertinatto · 2024-09-27T20:33:43Z

/test e2e-operator

bertinatto · 2024-09-27T21:28:18Z

/test e2e-operator

bertinatto · 2024-09-27T22:58:27Z

/test e2e-operator

bertinatto · 2024-09-28T00:38:57Z

/test e2e-operator

bertinatto · 2024-09-28T11:32:00Z

/test e2e-operator

bertinatto · 2024-09-28T13:15:47Z

/test e2e-operator

bertinatto · 2024-09-28T16:34:00Z

/test e2e-operator

bertinatto · 2024-09-28T17:13:33Z

/test e2e-operator

bertinatto · 2024-10-01T18:21:37Z

/retest-required

bertinatto · 2024-10-01T20:38:51Z

/test e2e-operator

: TestKeycloakAsOIDCPasswordGrantCheckAndGroupSync expand_less 	10m0s
{=== RUN   TestKeycloakAsOIDCPasswordGrantCheckAndG ...  === RUN   TestKeycloakAsOIDCPasswordGrantCheckAndG ...}

Looks like it's happening in 4.17 too: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/703/pull-ci-openshift-cluster-authentication-operator-release-4.17-e2e-operator/1841111144366870528

p0lyn0mial · 2024-10-02T08:19:44Z

pkg/controllers/common/conditions.go

@@ -79,8 +80,8 @@ func ControllerProgressingConditionName(controllerName string) string {
 	return controllerName + "Progressing"
 }

-func UpdateControllerConditions(ctx context.Context, operatorClient v1helpers.OperatorClient, allConditionNames sets.String, updatedConditions []operatorv1.OperatorCondition) error {
-	updateConditionFuncs := []v1helpers.UpdateStatusFunc{}
+func UpdateControllerConditions(ctx context.Context, operatorClient v1helpers.OperatorClient, fieldManager string, allConditionNames sets.String, updatedConditions []operatorv1.OperatorCondition) error {


nit: wouldn't be better to rename the function to ApplyControllerConditions ?

p0lyn0mial · 2024-10-02T08:30:14Z

pkg/controllers/common/conditions.go

 	}

-	return nil
+	return operatorClient.ApplyOperatorStatus(ctx, fieldManager, status)


should we skip the call when allConditionNames is empty ?

Added a check to allConditionNames in the beginning of the function.

p0lyn0mial · 2024-10-02T08:37:54Z

pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go

@@ -53,7 +56,7 @@ func NewIngressNodesAvailableController(
 		).
 		WithSync(controller.sync).
 		ResyncEvery(wait.Jitter(time.Minute, 1.0)).
-		ToController("IngressNodesAvailableController", eventRecorder)
+		ToController(instanceName, eventRecorder)


I think that in library-go we are passing c.controllerInstanceName (applies to the other changes in this PR)

Hm, interesting. I checked ResourceSyncController and DeploymentController and in both cases they use instanceName. It seems like the only exception is APIServiceController.

IIUC instanceName is what we used to call name and it fits better here because the Degraded condition name (set because of WithSyncDegradedOnError) would not change. On the other hand, controllerInstanceName would be mostly for setting the SSA's fieldManager.

Does that make sense?

i'm fine either way but we should be consistent.

given that for some controller we cannot change the name then maybe we should use instanceName ?

That's my understanding as well.

Actually I take it back. @deads2k created this m****s and was not willing to take into account feedback given in openshift/library-go#1794 :(.

The instanceName is the name of the operator, for example, openshift-apiserver or openshift-apiserver-operator, and the controllerInstanceName is the instanceName combined with the name of the controller.

Given this, it doesn't make sense to use the instanceName as the controller's name, as this would result in multiple controllers sharing the same name.

For the IngressNodesAvailableController, we can change its name because the previous name was not used to generate a condition. However, for controllers created with WithSyncDegradedOnError, it is not safe to change their name.

Does that make sense ?

NewCSIDriverNodeServiceController from the library-go PR mentioned earlier use WithSyncDegradedOnError, thus its name could not be changed.

Given how this controller is created, the instanceName will beNFSDriverNodeServiceController and the controllerInstanceName will be NFSDriverNodeServiceController-CSIDriverNodeService

Thanks for the explanation, @p0lyn0mial.
I'm worried that some variables, like instanceName, have different meanings in different places. For example, in this operator it would be oauth-apiserver, but in storage it would be NFSDriverNodeServiceController, as you pointed out.

p0lyn0mial · 2024-10-02T08:42:57Z

pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go

@@ -83,7 +87,7 @@ func NewWebhookAuthenticatorController(
 	).ResyncEvery(wait.Jitter(time.Minute, 1.0)).
 		WithSync(c.sync).
 		WithSyncDegradedOnError(operatorClient).
-		ToController("WebhookAuthenticatorController", recorder.WithComponentSuffix("webhook-authenticator-controller"))
+		ToController(instanceName, recorder.WithComponentSuffix("webhook-authenticator-controller"))


I think that we cannot rename this controller without removing existing condition added by the WithSyncDegradedOnError.

Actually we aren't renaming the controller, it's got the same name here: https://github.com/openshift/cluster-authentication-operator/pull/698/files#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R720

ok, then please add a comment where you set the name to not change it without clearing the condition.

for example: https://github.com/openshift/library-go/blob/ac65cd59c78f04a82617f7aecd6a0f1d16f985cb/pkg/operator/apiserver/controller/auditpolicy/auditpolicy_controller.go#L62

Good idea; done

bertinatto · 2024-10-02T23:36:03Z

/retest

bertinatto · 2024-10-03T17:51:29Z

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

@@ -54,7 +57,9 @@ func NewEndpointAccessibleController(
 		WithSync(c.sync).
 		ResyncEvery(wait.Jitter(time.Minute, 1.0)).
 		WithSyncDegradedOnError(operatorClient).
-		ToController(controllerName, recorder.WithComponentSuffix(name+"endpoint-accessible-controller"))
+		ToController(
+			controllerName, // Don't change what is passed here unless you also remove the old FooDegraded condition


This one is a bit strange. Degraded is prefixed by controllerName, but Available is prefixed by name.

Yes, but we cannot change these names I think.

p0lyn0mial

LGTM

p0lyn0mial · 2024-10-04T08:24:57Z

go.mod

+	k8s.io/client-go v0.31.1
+	k8s.io/component-base v0.31.1
+	k8s.io/klog/v2 v2.130.1
+	k8s.io/kube-aggregator v0.31.1
 	k8s.io/pod-security-admission v0.29.0


no 1.31.1 version ?

This one is a direct dependency of this module, so it needs to be updated manually.

If you prefer I can add a commit to update it to v0.31.1, but from what I can see, there are no changes, just the version.

ok, then let's leave it on 0.29.0

p0lyn0mial · 2024-10-04T08:34:31Z

pkg/libs/endpointaccessible/endpoint_accessible_controller.go

@@ -54,7 +57,9 @@ func NewEndpointAccessibleController(
 		WithSync(c.sync).
 		ResyncEvery(wait.Jitter(time.Minute, 1.0)).
 		WithSyncDegradedOnError(operatorClient).
-		ToController(controllerName, recorder.WithComponentSuffix(name+"endpoint-accessible-controller"))
+		ToController(
+			controllerName, // Don't change what is passed here unless you also remove the old FooDegraded condition


Yes, but we cannot change these names I think.

p0lyn0mial · 2024-10-04T08:40:44Z

pkg/operator/starter.go

@@ -717,6 +717,7 @@ func prepareOauthAPIServerOperator(ctx context.Context, controllerContext *contr
 	)

 	webhookAuthController := webhookauthenticator.NewWebhookAuthenticatorController(
+		"openshift-authentication",


I like this name. Thanks.

bertinatto · 2024-10-04T11:19:38Z

/retest

p0lyn0mial · 2024-10-04T12:12:20Z

/lgtm

deads2k · 2024-10-04T13:05:21Z

/approve

openshift-ci · 2024-10-04T13:05:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, deads2k, p0lyn0mial

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

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

openshift-ci · 2024-10-04T16:56:47Z

@bertinatto: all tests passed!

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

openshift-bot · 2024-10-04T19:42:47Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-authentication-operator
This PR has been included in build ose-cluster-authentication-operator-container-v4.18.0-202410041911.p0.g7ef902b.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 26, 2024

bertinatto changed the title ~~API-1835: Try apply~~ [WIP] API-1835: Try apply Sep 26, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2024

openshift-ci bot requested review from ibihim and liouk September 26, 2024 17:05

bertinatto force-pushed the try-apply branch from 7281703 to 43ef5f8 Compare September 27, 2024 14:45

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024

bertinatto force-pushed the try-apply branch from 43ef5f8 to d99071f Compare September 27, 2024 14:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024

bertinatto force-pushed the try-apply branch 3 times, most recently from f56427c to 1530f0f Compare September 27, 2024 18:53

bertinatto marked this pull request as draft September 27, 2024 19:37

bertinatto force-pushed the try-apply branch from 1530f0f to d8ece82 Compare September 27, 2024 19:38

deads2k reviewed Sep 27, 2024

View reviewed changes

pkg/controllers/common/conditions.go Outdated Show resolved Hide resolved

deads2k reviewed Sep 27, 2024

View reviewed changes

pkg/controllers/readiness/wellknown_ready_controller.go Outdated Show resolved Hide resolved

bertinatto force-pushed the try-apply branch from 18a2283 to c95538c Compare September 27, 2024 20:33

bertinatto force-pushed the try-apply branch from c95538c to 5b35177 Compare September 27, 2024 22:52

bertinatto force-pushed the try-apply branch from d6214c0 to 9a7234e Compare September 28, 2024 11:24

openshift-ci bot assigned liouk and p0lyn0mial Oct 1, 2024

p0lyn0mial reviewed Oct 2, 2024

View reviewed changes

bertinatto force-pushed the try-apply branch from 1da06d9 to 3323ceb Compare October 2, 2024 19:51

Bump api, client-go and library-go

6cf2698

bertinatto force-pushed the try-apply branch from 3323ceb to 3d11982 Compare October 2, 2024 20:05

bertinatto added 9 commits October 3, 2024 14:45

update UpdateControllerConditions helpers to use ApplyOperatorStatus

d3a8fc4

migrate ingressNodesAvailableController to ApplyOperatorStatus

df82481

migrate ingressStateController to ApplyOperatorStatus

dbe0ff5

migrate payloadConfig controller to ApplyOperatorStatus

b4d7f6f

migrate endpointAccessibleController to ApplyOperatorStatus

cd01f3e

migrate routerCertsDomainValidationController to ApplyOperatorStatus

3f5f575

migrate serviceCAController to ApplyOperatorStatus

345d045

migrate metadataController to ApplyOperatorStatus

0f075de

migrate webhookAuthenticatorController to ApplyOperatorStatus

35bf676

bertinatto force-pushed the try-apply branch from 3d11982 to 35bf676 Compare October 3, 2024 17:46

bertinatto commented Oct 3, 2024

View reviewed changes

p0lyn0mial reviewed Oct 4, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2024

openshift-merge-bot bot merged commit 7ef902b into openshift:master Oct 4, 2024
11 checks passed

API-1835: Migrate a few existing controllers to ApplyOperatorStatus #698

API-1835: Migrate a few existing controllers to ApplyOperatorStatus #698

Conversation

bertinatto commented Sep 26, 2024 • edited Loading

openshift-ci-robot commented Sep 26, 2024 • edited by openshift-ci bot Loading

bertinatto commented Sep 27, 2024

bertinatto commented Sep 27, 2024

bertinatto commented Sep 27, 2024

bertinatto commented Sep 27, 2024

bertinatto commented Sep 27, 2024

bertinatto commented Sep 28, 2024

bertinatto commented Sep 28, 2024

bertinatto commented Sep 28, 2024

bertinatto commented Sep 28, 2024

bertinatto commented Sep 28, 2024

bertinatto commented Oct 1, 2024

bertinatto commented Oct 1, 2024

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

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

Choose a reason for hiding this comment

bertinatto commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial 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

bertinatto commented Oct 4, 2024

p0lyn0mial commented Oct 4, 2024

deads2k commented Oct 4, 2024

openshift-ci bot commented Oct 4, 2024

openshift-ci bot commented Oct 4, 2024

openshift-bot commented Oct 4, 2024

bertinatto commented Sep 26, 2024 •

edited

Loading

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading