OCPBUGS-50587: Reject new NodeStatus with non-zero revision set #2208
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
everettraven
force-pushed
the
reject-new-nodestatus-with-nonzero-revision-set
branch
from
1dcee82 to
f2adc00
Compare
|
/test |
|
@everettraven: The The following commands are available to trigger optional jobs: Use 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-sigs/prow repository. |
|
/test all |
|
/payload-job periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.19-periodics-e2e-azure |
|
@everettraven: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7038d640-ee42-11ef-9b4a-a5a4a7d518e4-0 |
|
/retest |
@everettraven I figured out that the failures are related to this PR. Jobs will fail. |
openshift-ci
bot
added
size/XL
|
Attaching just for easy accessibility. #2207 is prerequisite for this PR. |
|
/payload-job periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.19-periodics-e2e-azure |
|
@ardaguclu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/33717610-eee5-11ef-9963-846be62b9323-0 |
|
/payload-job periodic-ci-openshift-cluster-control-plane-machine-set-operator-release-4.19-periodics-e2e-gcp |
|
@everettraven: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/603a58a0-eee6-11ef-87b5-1ffd47bf88a2-0 |
|
What do you think running all payload blockings? |
|
@ardaguclu I'm hesitant to do that because it is quite an expensive operation and my understanding is it should be used very sparingly. That being said, if it is the only way to get these jobs to run maybe it is worth it since this targets resolving component readiness failures? |
Goof point. Maybe after the review and if recommended from approver, we can trigger. |
|
@everettraven: This PR was included in a payload test run from openshift/cluster-control-plane-machine-set-operator#347
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7a75e780-eeea-11ef-9c70-5891df82d5b3-0 |
operator/v1/types.go
Outdated
| @@ -258,15 +258,21 @@ type StaticPodOperatorStatus struct { | |||
|
|
|||
| // NodeStatus provides information about the current state of a particular node managed by this operator. | |||
| // +kubebuilder:validation:XValidation:rule="has(self.currentRevision) || !has(oldSelf.currentRevision)",message="cannot be unset once set",fieldPath=".currentRevision" | |||
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || (has(self.currentRevision) ? self.currentRevision == 0 : true) ",message="when specified on creation of a nodeStatus, currentRevision must be set to 0 on creation",optionalOldSelf=true | |||
There was a problem hiding this comment.
Not saying this is better, but I think you could also write this as
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || (has(self.currentRevision) ? self.currentRevision == 0 : true) ",message="when specified on creation of a nodeStatus, currentRevision must be set to 0 on creation",optionalOldSelf=true | |
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.?currentRevision.orValue(0) == 0) ",message="when specified on creation of a nodeStatus, currentRevision must be set to 0 on creation",optionalOldSelf=true |
There was a problem hiding this comment.
I personally find the way it is currently written to be more straightforward than that from a reader perspective. I'd prefer to keep the rules easier to read and understand than more succinct even if it does the same thing.
There was a problem hiding this comment.
Perfectly valid, was just giving you a second option in case you felt this way might have been easier, I'm happy with either
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-critical
|
@everettraven: This pull request references Jira Issue OCPBUGS-50587, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
|
Huge thanks to @JoelSpeed for getting https://prow.ci.openshift.org/view/gs/test-platform-results/logs/multi-pr-openshift-cluster-control-plane-machine-set-operator-347-openshift-api-2208-e2e-azure-periodic-pre/1892272835435433984 run - looks like this change doesn't immediately break the payload job we noticed the regression on. I think we should be good to merge these changes pending review and watch the 7 day lookback that Ben had mentioned while we work on the backport process for 4.18. |
|
/retest |
|
As far as I can see, manual runs for machine-set-operator didn't give us a clue. We need to run payload blockings to have something; |
|
@ardaguclu: trigger 15 job(s) of type blocking for the nightly release of OCP 4.19
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c4ee4200-ef47-11ef-87e1-c1f8e3a92868-0 |
|
This PR openshift/cluster-control-plane-machine-set-operator#348 could have successfully run e2e-azure-periodic-pre |
|
/retest |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.19-e2e-metal-ipi-ovn-bm |
|
@ardaguclu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b7b85cb0-ef5b-11ef-96cb-e64ebe137026-0 |
https://prow.ci.openshift.org/view/gs/test-platform-results/logs/multi-pr-openshift-cluster-control-plane-machine-set-operator-348-openshift-api-2208-e2e-azure-periodic-pre/1892443956839452672 passed. Running a couple of more... |
…n not set to 0 Entries of static pod operators' node statuses are co-managed: - A node controller, responsible for ensuring a node status entry for per control plane node - An installer controller, responsible for managing operand revisions for each entry New node status entries with a non-zero revision populated signals that a single client is managing both aspects and is not intentional. This validation should serve as a pragmatic against multi-writer field errors. Signed-off-by: Bryce Palmer <[email protected]>
everettraven
force-pushed
the
reject-new-nodestatus-with-nonzero-revision-set
branch
from
1b31db9 to
46fc4d5
Compare
|
/payload-job periodic-ci-openshift-release-master-nightly-4.19-e2e-metal-ipi-ovn-bm |
|
@everettraven: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ad79f130-ef8a-11ef-9669-7234f2a65e3a-0 |
|
/retest |
Signed-off-by: Bryce Palmer <[email protected]>
openshift-ci
bot
added
size/L
|
/testwith e2e-azure openshift/origin#29558 |
|
@everettraven, |
|
/testwith openshift/api/main/e2e-azure openshift/origin#29558 |
|
@everettraven, |
|
/testwith openshift/api/master/e2e-azure openshift/origin#29558 |
|
@everettraven: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
No description provided.