OCPBUGS-3440: Restart authentication operator if console capability is enabled by bparees · Pull Request #589 · openshift/cluster-authentication-operator

bparees · 2022-11-09T14:08:17Z

No description provided.

openshift-ci-robot · 2022-11-09T14:26:28Z

@bparees: This pull request references Jira Issue OCPBUGS-3440, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xingxingxia

The bug has been updated to refer to the pull request using the external bug tracker.

Details

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 kubernetes/test-infra repository.

jhadvig · 2022-11-11T11:52:42Z

/assign @stlaz

bparees · 2022-11-14T14:38:22Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-11-14T14:38:23Z

@bparees: once the present PR merges, I will cherry-pick it on top of release-4.12 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.12

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.

stlaz · 2022-11-14T14:56:47Z

+	"github.com/openshift/cluster-authentication-operator/pkg/controllers/configobservation"
+)
+
+func ObserveClusterCapabilities(genericlisters configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (ret map[string]interface{}, _ []error) {


I understand it's tempting to use an observer for this but I'd rather have this as a separate controller.

The observers are supposed to change the observed config in a way.

Please create a "TerminationController" (or something like that) using the lib-go controller factory (other controllers in this repo are using that, too).

redone and pushed

stlaz · 2022-11-14T15:07:21Z

+		if capability == configv1.ClusterVersionCapabilityConsole {
+			recorder.Eventf("ObserveClusterCapabilities", "Console capability enabled, restarting cluster authentication operator")
+			klog.Infof("Console capability enabled, restarting cluster authentication operator")
+			f, err := os.OpenFile("/tmp/terminate", os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0600)


That's an interesting pattern. Basically we're working around not being able to cancel() the global context from the inside of the operator 🤔

yeah, the global context/cancel function created in the library isn't exposed to the operator itself so i couldn't just cancel it, at least i didn't see a way...i'd certainly have preferred to have just done that. But i also didn't want to go refactoring openshift-library to make it possible.

bparees · 2022-11-14T23:23:53Z

/hold cancel

bparees · 2022-11-14T23:24:09Z

/retest-required

bparees · 2022-11-15T18:44:13Z

/retest-required

bparees · 2022-11-17T22:18:24Z

/retest-required

stlaz

LGTM, if you could only document the controller a bit. I think it might even be worth removing the observer-internal logic on determining whether the cap is allowed now that we don't let it run in case the capability is just not there.

stlaz · 2022-11-18T11:28:04Z

+	recorder             events.Recorder
+}
+
+func NewTerminationController(configInformer configinformers.SharedInformerFactory, recorder events.Recorder) factory.Controller {


godoc please

…pability is enabled

bparees · 2022-11-21T16:49:08Z


 			for i := range errs {
-				if strings.Contains(tt.expectedErrs[i], errs[i].Error()) {
+				if !strings.Contains(errs[i].Error(), tt.expectedErrs[i]) {


this logic was wrong in two ways.

as the tests are written, the expected error string is a subset of the actual error string.

and of course the test should fail if an error occurs that was not in the expected list, not fail when an expected error occurs.

openshift-ci · 2022-11-21T19:43:53Z

@bparees: The following test 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-single-node	`fa9cd61`	link	false	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

Details

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.

bparees · 2022-11-22T14:55:49Z

/retest-required

stlaz · 2022-11-23T10:47:56Z

/lgtm

openshift-ci · 2022-11-23T10:48:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, stlaz

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [stlaz]

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 · 2022-11-23T10:52:51Z

@bparees: All pull requests linked via external trackers have merged:

openshift/cluster-authentication-operator#589

Jira Issue OCPBUGS-3440 has been moved to the MODIFIED state.

Details

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 kubernetes/test-infra repository.

openshift-cherrypick-robot · 2022-11-23T10:53:44Z

@bparees: failed to push cherry-picked changes in GitHub: pushing failed, output: "fatal: unable to access 'https://github.com/openshift-cherrypick-robot/cluster-authentication-operator/': Could not resolve host: github.com\n", error: exit status 128

Details

In response to this:

/cherry-pick release-4.12

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.

bparees · 2022-11-23T14:50:44Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-11-23T14:51:26Z

@bparees: new pull request created: #593

Details

In response to this:

/cherry-pick release-4.12

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.

bparees added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2022

openshift-ci Bot requested review from deads2k and stlaz November 9, 2022 14:09

bparees force-pushed the restart branch from 66ffb96 to e9015f5 Compare November 9, 2022 14:09

bparees changed the title ~~Restart authentication operator if console capability is enabled~~ [OCPBUGS-3440] Restart authentication operator if console capability is enabled Nov 9, 2022

bparees changed the title ~~[OCPBUGS-3440] Restart authentication operator if console capability is enabled~~ OCPBUGS-3440: Restart authentication operator if console capability is enabled Nov 9, 2022

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Nov 9, 2022

openshift-ci Bot requested a review from xingxingxia November 9, 2022 14:26

bparees force-pushed the restart branch 2 times, most recently from 0bfa6fd to ea9b424 Compare November 10, 2022 18:46

openshift-ci Bot assigned stlaz Nov 11, 2022

stlaz reviewed Nov 14, 2022

View reviewed changes

bparees force-pushed the restart branch 3 times, most recently from 4a4d3d1 to f28143a Compare November 14, 2022 20:22

bparees commented Nov 14, 2022

View reviewed changes

Comment thread pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2022

stlaz reviewed Nov 18, 2022

View reviewed changes

bparees force-pushed the restart branch from f28143a to be80e8b Compare November 18, 2022 14:41

OCPBUGS-3440: Restart cluster authentication operator when console ca…

fa9cd61

…pability is enabled

bparees force-pushed the restart branch from be80e8b to fa9cd61 Compare November 21, 2022 16:47

bparees commented Nov 21, 2022

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2022

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2022

openshift-merge-robot merged commit 0ce0bcc into openshift:master Nov 23, 2022

openshift-cherrypick-robot mentioned this pull request Nov 23, 2022

[release-4.12] OCPBUGS-4040: Restart authentication operator if console capability is enabled #593

Merged

sebrandon1 mentioned this pull request Jul 8, 2026

OCPBUGS-85579: Move Console resources from bundle to runtime with capability detection openshift/cert-manager-operator#455

Merged

Uh oh!

Conversation

bparees commented Nov 9, 2022

Uh oh!

openshift-ci-robot commented Nov 9, 2022

Uh oh!

jhadvig commented Nov 11, 2022

Uh oh!

bparees commented Nov 14, 2022

Uh oh!

openshift-cherrypick-robot commented Nov 14, 2022

Uh oh!

stlaz Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

bparees Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stlaz Nov 14, 2022 • 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!

bparees Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bparees commented Nov 14, 2022

Uh oh!

bparees commented Nov 14, 2022

Uh oh!

bparees commented Nov 15, 2022

Uh oh!

bparees commented Nov 17, 2022

Uh oh!

stlaz left a comment

Choose a reason for hiding this comment

Uh oh!

stlaz Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bparees Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bparees commented Nov 22, 2022

Uh oh!

stlaz commented Nov 23, 2022

Uh oh!

openshift-ci Bot commented Nov 23, 2022

Uh oh!

openshift-ci-robot commented Nov 23, 2022

Uh oh!

openshift-cherrypick-robot commented Nov 23, 2022

Uh oh!

bparees commented Nov 23, 2022

Uh oh!

openshift-cherrypick-robot commented Nov 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stlaz Nov 14, 2022 •

edited

Loading

openshift-ci Bot commented Nov 21, 2022 •

edited

Loading