Skip to content

Bug 1978376: Add admin ack Upgradeable condition gate#633

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jottofar:bug-1978376-admin-ack
Aug 25, 2021
Merged

Bug 1978376: Add admin ack Upgradeable condition gate#633
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jottofar:bug-1978376-admin-ack

Conversation

@jottofar
Copy link
Copy Markdown
Contributor

@jottofar jottofar commented Jul 27, 2021

This PR adds a generic upgrade blocking mechanism for blocking minor level upgrades. It is the general implementation of the Add admin ack upgrade gate enhancement. Therefore since 4.9 does not contain any active admin ack gates the check clusterAdminAcksCompletedUpgradeable is not added to defaultUpgradeableChecks. It is added as part of unit test and tested.

This PR will be backported to 4.8.z and the 4.8 specific implementation details will be added, i.e. the ack-4.8-kube-122-api-removals-in-4.9 gate will be added by adding check clusterAdminAcksCompletedUpgradeable to defaultUpgradeableChecks.

Once active the clusterAdminAcksCompletedUpgradeable Upgradeable check implementation differs from other Upgradeable checks by also being synchronously triggered when either configmap admin-gates or admin-acks is added, updated, or deleted. This will be achieved by adding an event handler to the cmConfigInformer and cmConfigManagedInformer Informers. The event handlers will simply invoke setUpgradeableConditions to ensure all conditions are properly evaluated. Since the check will be added as a default Upgradeable check when active it will also be evaluated during the existing periodic polling of Upgradeable conditions.

I also removed the unused parameter config from cvo.syncUpgradeable.

@openshift-ci 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 Jul 27, 2021
@openshift-ci openshift-ci Bot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 27, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 27, 2021

@jottofar: This pull request references Bugzilla bug 1978376, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianlinliu

Details

In response to this:

WIP: Bug 1978376: Add admin ack gate for deleted apis

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-ci openshift-ci Bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jul 27, 2021
@openshift-ci openshift-ci Bot requested a review from jianlinliu July 27, 2021 14:44
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch 10 times, most recently from 8a1297d to abc4ce7 Compare July 29, 2021 17:40
Comment thread pkg/cvo/upgradeable.go Outdated
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch 11 times, most recently from 44e9edf to 457b720 Compare August 3, 2021 21:13
@jottofar
Copy link
Copy Markdown
Contributor Author

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2021
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch from f92559c to a34c588 Compare August 20, 2021 18:29
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2021
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch 2 times, most recently from ea2f404 to ad746a8 Compare August 20, 2021 20:51
@jottofar
Copy link
Copy Markdown
Contributor Author

/unhold

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2021
Comment thread pkg/cvo/upgradeable.go
Comment thread pkg/cvo/upgradeable.go
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch 2 times, most recently from 094458b to f30aedb Compare August 24, 2021 17:20
Comment thread pkg/cvo/upgradeable.go Outdated
@jottofar jottofar force-pushed the bug-1978376-admin-ack branch from f30aedb to 519b466 Compare August 24, 2021 19:24
@jottofar
Copy link
Copy Markdown
Contributor Author

Tightened up gate regex to require one and only one following dash followed by something, i.e. the description. Changed error message to simply spit out "must comply with" and the regex expression itself since it's too hard to explain and these errors should only be consumed by developers. If an admin does add a gate in the field I don't think it's too much to ask that they understand regex.

@jottofar
Copy link
Copy Markdown
Contributor Author

/retest

Copy link
Copy Markdown
Member

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/retest

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jottofar, LalatenduMohanty, wking

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 [LalatenduMohanty,jottofar,wking]

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

@wking
Copy link
Copy Markdown
Member

wking commented Aug 25, 2021

The update job had terrible API-server / ingress connectivity. That's being tracked in rhbz#1955333 and/or rhbz#1997057, and is orthogonal to this PR, so:

/override ci/prow/e2e-agnostic-upgrade

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 25, 2021

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade

Details

In response to this:

The update job had terrible API-server / ingress connectivity. That's being tracked in rhbz#1955333 and/or rhbz#1997057, and is orthogonal to this PR, so:

/override ci/prow/e2e-agnostic-upgrade

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-merge-robot openshift-merge-robot merged commit c20e4d8 into openshift:master Aug 25, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 25, 2021

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

Bugzilla bug 1978376 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1978376: Add admin ack Upgradeable condition gate

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.

wking added a commit to wking/cluster-version-operator that referenced this pull request Aug 27, 2021
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
@jottofar
Copy link
Copy Markdown
Contributor Author

/cherry-pick release-4.8

@openshift-cherrypick-robot
Copy link
Copy Markdown

@jottofar: #633 failed to apply on top of branch "release-4.8":

Applying: Bug 1978376: Add admin ack Upgradeable condition gate
Using index info to reconstruct a base tree...
M	pkg/cvo/cvo.go
M	pkg/cvo/cvo_test.go
M	pkg/cvo/upgradeable.go
M	pkg/payload/precondition/clusterversion/upgradable_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/payload/precondition/clusterversion/upgradable_test.go
Auto-merging pkg/cvo/upgradeable.go
CONFLICT (content): Merge conflict in pkg/cvo/upgradeable.go
Auto-merging pkg/cvo/cvo_test.go
CONFLICT (content): Merge conflict in pkg/cvo/cvo_test.go
Auto-merging pkg/cvo/cvo.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 1978376: Add admin ack Upgradeable condition gate
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent 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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants