-
Notifications
You must be signed in to change notification settings - Fork 213
Bug 1999092: Add and enable admin ack Upgradeable condition gate #647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1999092: Add and enable admin ack Upgradeable condition gate #647
Conversation
|
@jottofar: An error was encountered querying GitHub for users with public email ([email protected]) for bug 1999092 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with DetailsIn 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. |
install/0000_00_cluster-version-operator_01_admingate_configmap.yaml
Outdated
Show resolved
Hide resolved
install/0000_00_cluster-version-operator_01_admingate_configmap.yaml
Outdated
Show resolved
Hide resolved
| data: | ||
| ack-4.8-kube-122-api-removals-in-4.9: | | ||
| Kubernetes 1.22 and thus OpenShift 4.9 remove several APIs which require admin consideration. Please see | ||
| https://docs.openshift.com/container-platform/4.8/release_notes/ocp-4-8-release-notes.html#ocp-4-8-deprecated-removed-features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hope we get to the point where we can point to better documentation for what an admin needs to go investigate/check before we ship this.
we should probably not merge this PR until we have that in place, since this gate will immediately start showing up in 4.8.z clusters.
|
/hold we need the proper openshift documentation for what an admin is expected to do prior to ugprading to 4.9 (how to check api usage, how to ACK the gate, etc) before this merges, because once this merges anyone who upgrades to 4.8.z is going to start seeing this non-upgradeable condition. |
941919f to
c97ad5c
Compare
|
/bugzilla refresh |
|
@wking: This pull request references Bugzilla bug 1999092, which is valid. 6 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
|
/retest |
ef50f62 to
07f522c
Compare
519b466 (Bug 1978376: Add admin ack Upgradeable condition gate, 2021-07-27, openshift#633) had these commented out, because 4.9 has no built-in acks. But with the code commented out, it's hard to verify that the logic works in 4.9 before backporting to 4.8 [1]. Enabling these checks should be a no-op outside of verification, because admins are unlikely to inject additional keys in the openshift-config-managed namespace's admin-gates ConfigMap. And it allows us to verify the logic in 4.9 and cook there with live code before approving the 4.8 backports. It's also one less thing we might forget before enabling new admin acks in future versions, like 4.10 or later. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1978376#c19
07f522c to
9e21155
Compare
install/0000_00_cluster-version-operator_01_admingate_configmap.yaml
Outdated
Show resolved
Hide resolved
wking
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With cherry-pick-diff:
$ cherry-pick-diff origin/release-4.8..origin/pr/647 origin/release-4.9
fatal: bad revision 'origin/release-4.9^{/admin-gates. Add ack-4.8-kube-122-api-removals-in-4.9 gate}'
??? -> 9e21155d admin-gates: Add ack-4.8-kube-122-api-removals-in-4.9 gate
5b5db7db -> a5db8728 pkg/cvo/upgradeable: Enable admin-ack logic
519b4667 -> 61946347 Bug 1978376: Add admin ack Upgradeable condition gate
fatal: bad revision 'origin/release-4.9^{/admin-gates. Add ack-4.8-kube-122-api-removals-in-4.9 gate}'
5b5db7db -> a5db8728 pkg/cvo/upgradeable: Enable admin-ack logic
...unimportant context changes...
519b4667 -> 61946347 Bug 1978376: Add admin ack Upgradeable condition gate
...unimportant context changes, plus some *Operator vs. Operator changes because 4.8 doesn't have #603...
...and in 4.9, #637's etcd backup landed after #633's admin ack, while in 4.8 this admin ack is landing after #649's etcd backup...The unique-to-4.8 9e21155 looks good to me.
/lgtm
9e21155 to
1504de0
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, sdodson, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[patch-manager] ⌛ This pull request was not picked by the patch manager for the current z-stream window and have to wait for the next window. skipped for today
NOTE: This message was automatically generated, if you have questions please ask on #forum-release |
|
/hold cancel |
|
/retest |
|
[patch-manager] 🚀 Approved for z-stream by score: 1.80 picked |
|
/override ci/prow/e2e-agnostic-upgrade |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade DetailsIn 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. |
|
@jottofar: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 1999092 has not been moved to the MODIFIED state. DetailsIn 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. |
…e-1.22-api-removals-in-4.9
Avoid [1]:
alert ClusterNotUpgradeable fired for 1115 seconds with labels: {condition="Upgradeable", endpoint="metrics", name="version", severity="warning"}
for tests that install 4.8 and then sit on it for over an hour, now
that [2] has landed in 4.8. [3] will follow up with code to handle CI
that installs an earlier release and updates into 4.8 or later.
[1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-release-master-nightly-4.8-e2e-aws-serial/1443018123379740672
[2]: openshift/cluster-version-operator#647
[3]: openshift/origin#26446
This PR backports "Bug 1978376: Add admin ack Upgradeable condition gate" and "pkg/cvo/upgradeable: Enable admin-ack logic". It also adds the 4.8 gate "ack-4.8-kube-122-api-removals-in-4.9" which blocks upgrades to 4.9 until an admin performs the required acknowledgement step.
The PR is composed of 3 commits the first 2 of which were created with cherry-pick:
$ git cherry-pick 519b466
$ git cherry-pick 5b5db7d