Bug 1952174: status: Report old versions while rolling out new #274
Conversation
* pkg/operator/controller/status/controller.go (Reconcile): Delete receiver in call to computeOperatorStatusVersions. (computeOperatorStatusVersions): Delete unused receiver. * pkg/operator/controller/status/controller_test.go (TestComputeOperatorStatusVersions): Adjust call to computeOperatorStatusVersions.
openshift-ci
bot
added
the
bugzilla/severity-high
|
@Miciah: This pull request references Bugzilla bug 1952174, 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
Requesting review from QA contact: 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-ci
bot
added
the
bugzilla/valid-bug
|
@Miciah: This pull request references Bugzilla bug 1952174, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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-ci
bot
added
the
approved
Miciah
force-pushed
the
BZ1952174-status-report-old-versions-while-rolling-out-new
branch
3 times, most recently
from
84f2683
to
f62d668
Compare
| namespace corev1.Namespace | ||
|
|
||
| // haveDNS indicates whether the "default" dnses.openshift.operator.io | ||
| // CRD exists. |
There was a problem hiding this comment.
| // CRD exists. | |
| // CR exists. |
total nit, don't feel obligated to fix this.
There was a problem hiding this comment.
Sorry, couldn't help feeling obligated to fix "CRD" here and above, as well as the API group name.
| k8s.io/api v0.21.0 | ||
| k8s.io/apimachinery v0.21.0 | ||
| k8s.io/client-go v0.21.0 | ||
| k8s.io/kubectl v0.21.0 |
There was a problem hiding this comment.
At first this made me a little nervous!
But then I remembered that go mod is smart enough to only import what we actually need from kubectl.
Miciah
force-pushed
the
BZ1952174-status-report-old-versions-while-rolling-out-new
branch
from
f62d668
to
c2b2577
Compare
|
/test e2e-upgrade |
For each operand, keep reporting the operand's old version in the clusteroperator status.versions value until the new version of the operand is fully rolled out, so that the clusteroperator doesn't report the new version before the operand has finished upgrading. Likewise, keep reporting the operator's old version until all operands are updated to their respective new versions. This commit fixes bug 1952174. https://bugzilla.redhat.com/show_bug.cgi?id=1952174 * pkg/operator/controller/status/controller.go (Reconcile): Use the new operatorState fields and the new computeOldVersions, computeNewVersions, and computeCurrentVersions functions to compute versions maps. Pass these versions maps to computeOperatorProgressingCondition. Pass the current versions map to computeOperatorStatusVersions. (operatorState): Add dnsDaemonSet, nodeResolverDaemonSet, dnsPodList, and nodeResolverPodList fields. (getOperatorState): Initialize the new fields in operatorState. (computeOperatorStatusVersions): Rewrite to take the new versions map and return a slice of OperandVersion. (computeOperatorProgressingCondition): Replace the OperandVersion slice and string parameter values with parameters for the old, new, and current versions maps. Use the versions maps to compute the "Progressing" status condition. (computeOldVersions): New function. Convert the OperandVersion slice value from the clusteroperator's status.versions field into a versions map. (computeNewVersions): New function. Convert the versions from the reconciler into a versions map. (computeCurrentVersions): New function. Use the old and new versions maps and dns and node-resolver daemonsets and pods to compute the current versions map. * pkg/operator/controller/status/controller_test.go (TestComputeOperatorProgressingCondition): Adjust for changes to computeOperatorProgressingCondition. (TestComputeOperatorStatusVersions): Delete test. (TestComputeCurrentVersions): New test. Verify that computeCurrentVersions behaves as expected. * go.mod: Add k8s.io/kubectl dependency for the "k8s.io/kubectl/pkg/util/podutils".IsPodAvailable function, used in computeCurrentVersions. * go.sum: * vendor/*: Regenerate.
|
Reviewing the latest run of the e2e-upgrade CI job, {
"level": "Warning",
"locator": "clusteroperator/dns",
"message": "condition/Progressing status/True reason/Upgrading kube-rbac-proxy to \"registry.build02.ci.openshift.org/ci-op-d7g7114b/stable@sha256:3869910c1e208b125bdecd4ac2d8b2cae42efe221c704491b86aa9b18ce95a65\".\nUpgrading operator to \"4.8.0-0.ci.test-2021-05-21-025413-ci-op-d7g7114b-latest\".\nUpgrading coredns to \"registry.build02.ci.openshift.org/ci-op-d7g7114b/stable@sha256:bcdefdbcee8af1e634e68a850c52fe1e9cb31364525e30f5b20ee4eacb93c3e8\".\nUpgrading openshift-cli to \"registry.build02.ci.openshift.org/ci-op-d7g7114b/stable@sha256:7c886e097d93dec6633844c20611937b028d283ddcd11fb2c33d89233dd00abe\".",
"from": "2021-05-21T04:29:04Z",
"to": "2021-05-21T04:33:11Z"
},And looking at And then the reported versions for the coredns and kube-rbac-proxy operands as well as for the operator itself were updated at 04:33:11: So versions were updated at 04:29:37 and 04:33:11, which fall within the period of 04:29:04 to 04:33:11 during which the operator indicated it was progressing, which is as expected. I'll push a fix for #274 (comment) and hope for another good CI run. |
Miciah
force-pushed
the
BZ1952174-status-report-old-versions-while-rolling-out-new
branch
from
c2b2577
to
1b3e6d6
Compare
|
failed on the |
|
In the latest run of the e2e-upgrade CI job, {
"level": "Warning",
"locator": "clusteroperator/dns",
"message": "condition/Progressing status/True reason/Upgrading operator to \"4.8.0-0.ci.test-2021-05-21-202215-ci-op-k590kbmz-latest\".\nUpgrading coredns to \"registry.build02.ci.openshift.org/ci-op-k590kbmz/stable@sha256:bcdefdbcee8af1e634e68a850c52fe1e9cb31364525e30f5b20ee4eacb93c3e8\".\nUpgrading openshift-cli to \"registry.build02.ci.openshift.org/ci-op-k590kbmz/stable@sha256:9c0914ba655514ee0a144b255f34c9ed7c5c66a7d90e04024355b9edb080dca9\".\nUpgrading kube-rbac-proxy to \"registry.build02.ci.openshift.org/ci-op-k590kbmz/stable@sha256:3869910c1e208b125bdecd4ac2d8b2cae42efe221c704491b86aa9b18ce95a65\".",
"from": "2021-05-21T22:17:33Z",
"to": "2021-05-21T22:21:36Z"
},And looking at And then the reported versions for the coredns and kube-rbac-proxy operands as well as for the operator itself were updated at 22:21:36: So versions were updated at 22:18:24 and 22:21:36, which fall within the period of 22:17:33 to 22:21:36 during which the operator indicated it was progressing, which is as expected. |
|
/test e2e-upgrade |
|
looks good! Thanks for the in-depth explanation in #274 (comment) /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 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:
Approvers can indicate their approval by writing |
|
@Miciah: All pull requests linked via external trackers have merged: Bugzilla bug 1952174 has been moved to the MODIFIED state. 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. |
computeOperatorStatusVersions: Delete receiverpkg/operator/controller/status/controller.go(Reconcile): Delete receiver in call tocomputeOperatorStatusVersions.(
computeOperatorStatusVersions): Delete unused receiver.pkg/operator/controller/status/controller_test.go(TestComputeOperatorStatusVersions): Adjust call tocomputeOperatorStatusVersions.status: Report old versions while rolling out new
For each operand, keep reporting the operand's old version in the clusteroperator
status.versionsvalue until the new version of the operand is fully rolled out, so that the clusteroperator doesn't report the new version before the operand has finished upgrading. Likewise, keep reporting the operator's old version until all operands are updated to their respective new versions.pkg/operator/controller/status/controller.go(Reconcile): Use the newoperatorStatefields and the newcomputeOldVersions,computeNewVersions, andcomputeCurrentVersionsfunctions to compute versions maps. Pass these versions maps tocomputeOperatorProgressingCondition. Pass the current versions map tocomputeOperatorStatusVersions.(
operatorState): AdddnsDaemonSet,nodeResolverDaemonSet,dnsPodList, andnodeResolverPodListfields.(
getOperatorState): Initialize the new fields inoperatorState.(
computeOperatorStatusVersions): Rewrite to take the new versions map and return a slice ofOperandVersion.(
computeOperatorProgressingCondition): Replace theOperandVersionslice and string parameter values with parameters for the old, new, and current versions maps. Use the versions maps to compute theProgressingstatus condition.(
computeOldVersions): New function. Convert theOperandVersionslice value from the clusteroperator'sstatus.versionsfield into a versions map.(
computeNewVersions): New function. Convert the versions from the reconciler into a versions map.(
computeCurrentVersions): New function. Use the old and new versions maps and dns and node-resolver daemonsets and pods to compute the current versions map.pkg/operator/controller/status/controller_test.go(TestComputeOperatorProgressingCondition): Adjust for changes tocomputeOperatorProgressingCondition.(
TestComputeOperatorStatusVersions): Delete test.(
TestComputeCurrentVersions): New test. Verify thatcomputeCurrentVersionsbehaves as expected.go.mod: Addk8s.io/kubectldependency for the"k8s.io/kubectl/pkg/util/podutils".IsPodAvailablefunction, used incomputeCurrentVersions.go.sum:vendor/*: Regenerate.Follow-up to #269.