AUTH-543: Add workload deletion condition method and clean-up #1977

liouk · 2025-07-15T12:46:44Z

There might be cases (as demonstrated in OCPBUGS-44937) where we might want to gracefully delete the operand workload of the workload controller, and keep the operator status available (instead of unavailable or degraded).

This PR adds an optional way of specifying a deletion condition which will trigger the deletion of the operand gracefully, keeping the operator's status as Available=True.

This is needed in the scope of openshift/cluster-authentication-operator#740.

This PR replaces #1902.

openshift-ci-robot · 2025-07-15T12:47:22Z

@liouk: This pull request references AUTH-543 which is a valid jira issue.

In response to this:

There might be cases (as demonstrated in OCPBUGS-44937) where we might want to gracefully delete the operand workload of the workload controller, and keep the operator status available (instead of unavailable or degraded).

This PR adds an optional way of specifying a deletion condition which will trigger the deletion of the operand gracefully, keeping the operator's status as Available=True.

This is needed in the scope of openshift/cluster-authentication-operator#740.

This PR replaces #1902.

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.

p0lyn0mial · 2025-07-15T12:56:42Z

pkg/operator/apiserver/controller/workload/workload.go

+		// remove any conditions and generations owned by it, because the respective API fields have 'map'
+		// as the list type where field managers can be list element-specific
+		if err := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, applyoperatorv1.OperatorStatus()); err != nil {
+			return c.updateOperatorStatus(ctx, operatorStatus, nil, false, false, []error{err})


I would just simply return an err here, this is also what we do in updateOperatorStatus

p0lyn0mial · 2025-07-15T13:03:24Z

pkg/operator/status/status_controller.go


+// WithEmptyVersionRemoval returns a copy of the StatusSyncer that will
+// remove versions that are an empty string in VersionGetter from the status.
+func (c *StatusSyncer) WithEmptyVersionRemoval() *StatusSyncer {


could we use WithVersionRemoval instead ?

There's already a WithVersionRemoval() method that does something slightly different: it removes versions that are missing in VersionGetter from the status. Also, there's no way to delete a version from the version getter, as it only allows to Get/Set one, hence the empty string need.

mhm, maybe we should allow for version removal then ?

I think that would involve exposing a new method on https://github.com/openshift/cluster-openshift-apiserver-operator/blob/476eac3b927b2a935f01f8ac4ec74f9a0cf54bc4/vendor/github.com/openshift/library-go/pkg/operator/status/version.go#L33

If we had such method then we could pair it with WithVersionRemoval, right ?

p0lyn0mial · 2025-07-15T15:31:13Z

pkg/operator/status/version.go

+	v.lock.Lock()
+	defer v.lock.Unlock()
+
+	delete(v.versions, operandName)


should we check if the version wasn't removed ?

and if it was maybe we don't have to notify (?)

delete() is a no-op if the map is nil/empty or the key doesn't exist, so this is a safe call.

but we will call notifyChannelsLocked event if nothing was removed.
let's only call notifyChannelsLocked when we removed an entry from the map.

p0lyn0mial · 2025-07-15T15:31:50Z

pkg/operator/status/version.go

 	return channel
 }

+func (v *versionGetter) notifyChannels() {


please change the name of the method to notifyChannelsLocked so that it is clear it must be called under lock.

Great point

p0lyn0mial · 2025-07-16T07:19:49Z

pkg/operator/apiserver/controller/workload/workload.go

 		return err
 	}

+	if deleted, operandName, err := c.delegate.WorkloadDeleted(ctx); err != nil {


I think we need to move it down. In theory it could be possible to define preconditions for the new method as well.

So I think the order should be: Precondiditons, WorkladDeleted, Sync

p0lyn0mial · 2025-07-16T07:20:39Z

pkg/operator/apiserver/controller/workload/workload.go

+			return err
+		}
+
+		c.versionRecorder.UnsetVersion(operandName)


The corresponding Set method can be prefixed with c.operandNamePrefix. I think we need to do the same, no?

p0lyn0mial · 2025-07-16T07:21:45Z

pkg/operator/status/status_controller.go

 type VersionGetter interface {
 	// SetVersion is a way to set the version for an operand.  It must be thread-safe
 	SetVersion(operandName, version string)
+	// UnsetVersion removes a version for an operand.  It must be thread-safe


please expand the comment by explaining what happens when the given operandName was already deleted.

p0lyn0mial · 2025-07-16T07:22:38Z

pkg/operator/status/version.go

+	v.lock.Lock()
+	defer v.lock.Unlock()
+
+	delete(v.versions, operandName)


but we will call notifyChannelsLocked event if nothing was removed.
let's only call notifyChannelsLocked when we removed an entry from the map.

p0lyn0mial

just two more comments, other than that LGTM

p0lyn0mial · 2025-07-16T07:51:57Z

pkg/operator/apiserver/controller/workload/workload.go

 	return nil
 }

+func (c *Controller) addPrefix(name string) string {


nit: please rename to constructOperandNameFor(name string) string or constructOperandName(name string) string

p0lyn0mial · 2025-07-16T07:52:33Z

pkg/operator/status/version.go

+	v.lock.Lock()
+	defer v.lock.Unlock()
+
+	delete(v.versions, operandName)


…rface This determines whether the delegate controller has deleted the workload, and indicates to the workload controller that it must clear the respective operator status fields.

p0lyn0mial · 2025-07-16T08:02:28Z

/lgtm
/approve

/hold

please test this PR with the oas and maybe the auth operator.
feel free to cancel the hold if there are no errors. thanks!

openshift-ci · 2025-07-16T08:03:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liouk, 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:

~~pkg/operator/OWNERS~~ [p0lyn0mial]

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

liouk · 2025-07-16T08:21:40Z

Proof PRs:

everettraven · 2025-07-29T12:11:04Z

New proof PRs after dependency bumps:

DNM: WIP: Proof PR for openshift/library-go#1977 cluster-authentication-operator#777 - All green besides a permafailing test due to a known issue.
DNM: WIP: Proof PR for openshift/library-go#1977 cluster-openshift-apiserver-operator#621

everettraven · 2025-07-29T17:28:34Z

openshift/cluster-openshift-apiserver-operator#621 has passed all tests as well.

please test this PR with the oas and maybe the auth operator.
feel free to cancel the hold if there are no errors. thanks!

Unholding as this has been satisfied.

/hold cancel

openshift-ci-robot · 2025-07-29T18:01:32Z

/retest-required

Remaining retests: 0 against base HEAD 7f9bc3e and 2 for PR HEAD adfa251 in total

openshift-ci · 2025-07-29T18:23:04Z

@liouk: 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.

liouk mentioned this pull request Jul 15, 2025

AUTH-543: Add optional operand deletion condition #1902

Closed

liouk changed the title ~~Add workload deletion condition method and clean-up~~ AUTH-543: Add workload deletion condition method and clean-up Jul 15, 2025

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

openshift-ci bot requested review from deads2k and p0lyn0mial July 15, 2025 12:50

p0lyn0mial reviewed Jul 15, 2025

View reviewed changes

liouk force-pushed the AUTH-543 branch 2 times, most recently from cedc4e7 to cb97c15 Compare July 15, 2025 14:59

p0lyn0mial reviewed Jul 15, 2025

View reviewed changes

liouk force-pushed the AUTH-543 branch from cb97c15 to e062486 Compare July 15, 2025 15:35

p0lyn0mial reviewed Jul 16, 2025

View reviewed changes

liouk force-pushed the AUTH-543 branch from e062486 to 32e4e26 Compare July 16, 2025 07:47

p0lyn0mial reviewed Jul 16, 2025

View reviewed changes

liouk added 2 commits July 16, 2025 09:57

status: add method to remove an operand version from the VersionGetter

17cb27e

workload: add workload deletion condition method to the delegate inte…

adfa251

…rface This determines whether the delegate controller has deleted the workload, and indicates to the workload controller that it must clear the respective operator status fields.

liouk force-pushed the AUTH-543 branch from 32e4e26 to adfa251 Compare July 16, 2025 07:57

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2025

openshift-ci bot assigned p0lyn0mial Jul 16, 2025

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

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

This was referenced Jul 16, 2025

DO-NOT-MERGE: Prove library-go#1977 openshift/cluster-openshift-apiserver-operator#619

Closed

DO-NOT-MERGE: Prove library-go#1977 openshift/cluster-authentication-operator#774

Closed

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2025

openshift-merge-bot bot merged commit 0e81d05 into openshift:master Jul 29, 2025
4 checks passed

AUTH-543: Add workload deletion condition method and clean-up #1977

AUTH-543: Add workload deletion condition method and clean-up #1977

Conversation

liouk commented Jul 15, 2025

Uh oh!

openshift-ci-robot commented Jul 15, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liouk Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

p0lyn0mial left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

p0lyn0mial commented Jul 16, 2025

Uh oh!

openshift-ci bot commented Jul 16, 2025

Uh oh!

liouk commented Jul 16, 2025

Uh oh!

everettraven commented Jul 29, 2025

Uh oh!

everettraven commented Jul 29, 2025

Uh oh!

openshift-ci-robot commented Jul 29, 2025

Uh oh!

openshift-ci bot commented Jul 29, 2025

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 15, 2025 •

edited by openshift-ci bot

Loading

liouk Jul 15, 2025 •

edited

Loading

p0lyn0mial left a comment •

edited

Loading