Bug 2009879: Add admin ack Upgradeable condition gate test #26525

jottofar · 2021-10-13T15:20:46Z

Create test to verify Admin Ack functionality and include in both extendedand e2e/upgrade tests.

openshift-ci · 2021-10-13T15:20:54Z

@jottofar: This pull request references Bugzilla bug 2009879, 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.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, 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:

Bug 2009879: Add admin ack Upgradeable condition gate test

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 · 2021-10-14T13:56:36Z

/retest

jottofar · 2021-10-18T12:45:14Z

/retest

wking · 2021-10-19T21:35:27Z

test/extended/util/adminack.go

+		return
+	}
+	var msg string
+	if msg = gateCm.Data["ack-4.8-kube-1.22-api-removals-in-4.9"]; msg == "" {


we will not have this gate in born-in-4.9+ clusters. Can we replace this with something generic that walks keys which happen to be present in gateCm.Data?

This gets into whether the purpose of this is to test the code or to simply set the gate. The latter really being a pre-test step. Since this is a test I thought explicitly having to name the gate(s) was okay. As gates are added the test has to be updated in a very explicit and purposeful way. Otherwise I will also start to duplicate the code under test in the test.

The latter really being a pre-test step.

It's hard to make setting the gate a pre-test step for steps that install 4.y and perform a chained update through 4.(y+1) to 4.(y+2) and beyond, if gate is new in 4.(y+1). So I'm hoping that this can be both "test the functionality" and "set the gate". But I can see some concerns about setting the gate in non-update conformance tests, so perhaps just extend openshift/release#22368 and actively create that ConfigMap when it doesn't exist, and populate it with all the acks we ever have?

As gates are added the test has to be updated in a very explicit and purposeful way.

This isn't clear to me. Can't the "test the code" bit just ensure that Upgradeable responds as expected when all configured gates are acked (it goes False) and when any configured gate is not acked (it goes True)? I guess you'd need to include something to pick out gates that applied to the current 4.y. But I don't understand how this would be tied to a specific gate beyond the minor-version association.

I just went ahead an looped through the configured gates to keep must of the logic here instead of adding to release. Having the test assume all the configured gates are valid, i.e. should be there, is reasonable enough.

wking · 2021-10-19T21:39:34Z

test/extended/util/adminack.go

+		if !upgradeable(ctx, t.Config) {
+			if adminAckRequiredWithMessage(ctx, t.Config, msg) {
+				framework.Failf(fmt.Sprintf("Gate ack-4.8-kube-1.22-api-removals-in-4.9 has been ack'ed but Upgradeable is "+
+					"false with reason AdminAckRequired and message %q.", msg))


!upgradeable(...) is looking for "Upgradeable is not True". That will often be "because it's False", but might be "because it's unset". Can we use "not True" here? Or teach upgradeable to return a status pointer or something so we can distinguish between the various not-True cases in the failure message?

Actually it ... returns true if the Upgradeable condition is nil or is set to true.

so still, !upgradeable(...) could be "false" or could be "unknown" or whatever. So can we pull the whole Upgradeable condition back and do something like:

framework.Failf(fmt.Sprintf("Gate ack-4.8-kube-1.22-api-removals-in-4.9 has been ack'ed but Upgradeable is %s with reason %s and message: %s", cond.Status, cond.Reason, cond.Message))

so still, !upgradeable(...) could be "false" or could be "unknown" or whatever

No. Put another way upgradeable only returns false when cond != nil && cond.Status == configv1.ConditionFalse so that's why I hard-coded status and reason. Perhaps changing the method name would help. Maybe upgradeableNotFalse, but then we can have a double negative, so perhaps upgradeableNilOrTrue.

Ah, it's that the condition's Status is not set (unknown ) not that cond is nil. Changed to reflect this understanding.

test/extended/util/adminack.go

jottofar · 2021-11-03T19:14:35Z

/retest

wking

/lgtm

🎉 Now we just need to track down an approver...

deads2k · 2021-11-04T12:50:04Z

/approve

openshift-ci · 2021-11-04T12:59:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~test/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-bot · 2021-11-04T14:41:12Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-11-04T14:54:09Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-11-04T16:26:09Z

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

openshift/origin#26525

Bugzilla bug 2009879 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2009879: Add admin ack Upgradeable condition gate test

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 · 2021-11-19T20:29:50Z

/cherrypick release-4.9

openshift-cherrypick-robot · 2021-11-19T20:30:33Z

@wking: new pull request created: #26629

Details

In response to this:

/cherrypick release-4.9

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.

…tForAdminAck(Not)Required message We'd had an empty-string msg since this test-case landed in 4f5be5a (test: Add admin ack Upgradeable condition gate test, 2021-10-13, openshift#26525), as seen in [1]: Dec 9 19:12:51.005: INFO: Waiting for Upgradeable to be AdminAckRequired for "" ... Dec 9 19:12:51.279: INFO: Waiting for Upgradeable to not be AdminAckRequired for "" ... That's fine as long as we only have one ack gate, but this commit passes through the gate value to match the adminAckRequiredWithMessage call slightly earlier in AdminAckTest.test. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2027929#c2

…MultipleReasons When there's another reason like TechPreviewNoUpgrade, the CVO will be complaining about both and set MultipleReasons. We want the test suite to understand that that's still an AdminAckRequired complaint, and not fail like [1]: : [sig-cluster-lifecycle] TestAdminAck should succeed [Suite:openshift/conformance/parallel] Run #0: 3m3s { fail [github.com/openshift/origin/test/extended/util/openshift/clusterversionoperator/adminack.go:117]: May 6 07:24:05.028: Error while waiting for Upgradeable to go AdminAckRequired with message "Kubernetes 1.25 and therefore OpenShift 4.12 remove several APIs which require admin consideration. Please see\nthe knowledge article https://access.redhat.com/articles/6955381 for details and instructions.\n": timed out waiting for the condition Upgradeable: Status=False, Reason=MultipleReasons, Message="Cluster should not be upgraded between minor versions for multiple reasons: AdminAckRequired,FeatureGates_RestrictedFeatureGates_TechPreviewNoUpgrade\n* Kubernetes 1.25 and therefore OpenShift 4.12 remove several APIs which require admin consideration. Please see\nthe knowledge article https://access.redhat.com/articles/6955381 for details and instructions.\n\n* Cluster operator kube-apiserver should not be upgraded between minor versions: FeatureGatesUpgradeable: \"TechPreviewNoUpgrade\" does not allow updates".} The Contains logic that I'm removing is from the original test-case implementation in 4f5be5a (test: Add admin ack Upgradeable condition gate test, 2021-10-13, openshift#26525), and seems to have been assuming that we would concatenate the constituent reasons, instead of replacing all of them with MultipleReasons. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-release-master-ci-4.11-e2e-azure-techpreview/1522457624627384320

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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 Oct 13, 2021

openshift-ci bot requested review from jianlinliu, knobunc and mfojtik October 13, 2021 15:20

jottofar force-pushed the bug-2009879 branch 2 times, most recently from 21fa931 to 4b34b5d Compare October 13, 2021 20:16

jottofar mentioned this pull request Oct 15, 2021

Bug 1951835: reconciliation.md: Update for degraded status propagation during init openshift/enhancements#935

Closed

wking reviewed Oct 19, 2021

View reviewed changes

jottofar force-pushed the bug-2009879 branch 2 times, most recently from be5c058 to 17c7f3e Compare October 28, 2021 21:12

wking reviewed Oct 28, 2021

View reviewed changes

test/extended/util/adminack.go Outdated Show resolved Hide resolved

wking reviewed Oct 28, 2021

View reviewed changes

test/extended/util/adminack.go Show resolved Hide resolved

wking reviewed Oct 28, 2021

View reviewed changes

test/extended/util/adminack.go Outdated Show resolved Hide resolved

jottofar force-pushed the bug-2009879 branch from 17c7f3e to 7c0d05d Compare October 29, 2021 18:52

wking reviewed Nov 3, 2021

View reviewed changes

test/extended/util/adminack.go Outdated Show resolved Hide resolved

test: Add admin ack Upgradeable condition gate test

4f5be5a

jottofar force-pushed the bug-2009879 branch from 7c0d05d to 4f5be5a Compare November 3, 2021 15:31

wking approved these changes Nov 3, 2021

View reviewed changes

openshift-ci bot assigned wking Nov 3, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2021

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

openshift-merge-robot merged commit 0c86019 into openshift:master Nov 4, 2021

openshift-cherrypick-robot mentioned this pull request Nov 19, 2021

Bug 2025088: Add admin ack Upgradeable condition gate test #26629

Merged

wking mentioned this pull request May 6, 2022

test/extended/util/openshift/clusterversionoperator/adminack: Accept MultipleReasons #27110

Merged

Bug 2009879: Add admin ack Upgradeable condition gate test #26525

Bug 2009879: Add admin ack Upgradeable condition gate test #26525

Uh oh!

Conversation

jottofar commented Oct 13, 2021

Uh oh!

openshift-ci bot commented Oct 13, 2021

Uh oh!

jottofar commented Oct 14, 2021

Uh oh!

jottofar commented Oct 18, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jottofar commented Nov 3, 2021

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

deads2k commented Nov 4, 2021

Uh oh!

openshift-ci bot commented Nov 4, 2021

Uh oh!

openshift-bot commented Nov 4, 2021

Uh oh!

openshift-bot commented Nov 4, 2021

Uh oh!

openshift-ci bot commented Nov 4, 2021

Uh oh!

wking commented Nov 19, 2021

Uh oh!

openshift-cherrypick-robot commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants