Bug 1794678: Add clusteroperator status #71
Conversation
openshift-ci-robot
added
the
do-not-merge/hold
|
@enxebre: An error was encountered adding this pull request to the external tracker bugs for bug 1794678 on the Bugzilla server at https://bugzilla.redhat.com:
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. |
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/severity-medium
|
@enxebre: This pull request references Bugzilla bug 1794678, 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
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-robot
added
the
bugzilla/valid-bug
enxebre
changed the title
|
/hold cancel |
|
@enxebre: This pull request references Bugzilla bug 1794678, which is valid. 3 validation(s) were run on this bug
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-robot
removed
the
do-not-merge/hold
enxebre
force-pushed
the
fix-1794678
branch
2 times, most recently
from
fd32360
to
5006411
Compare
|
/retest |
|
@enxebre: This pull request references Bugzilla bug 1794678, which is valid. 3 validation(s) were run on this bug
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. |
1 similar comment
|
@enxebre: This pull request references Bugzilla bug 1794678, which is valid. 3 validation(s) were run on this bug
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. |
|
/hold A follow up will create PR against https://github.com/openshift/cluster-api-actuator-pkg to validate the object exists. |
openshift-ci-robot
added
the
do-not-merge/hold
| submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' | ||
| type: string | ||
| metadata: | ||
| type: object |
There was a problem hiding this comment.
would we ever use a json pointer notation here to point back to the openapi v1.ObjectMeta definition in the kubernetes repo?
it would be crazy long reference, but i'm just curious.
There was a problem hiding this comment.
This is a snapshot just so the tests have the CRD available 3ee0a68#diff-390d58653a7a71d7b34e4ef2d39ae697R1
ObjectMeta is an api machinery top level known field for every resource.
There was a problem hiding this comment.
got it, thanks
i know ObjectMeta is all over the place, i was just kinda curious if these external schemas should use json pointers to reference back.
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
The payload build process replace "0.0.1-snapshot" in the manifest with the targeted version. This manifests is then used by the CVO to compare targeted version against the one in the living clusterOperator object. The controller is responsible for setting the targeted one in the living clusterOperator object during an upgrade. https://github.com/openshift/cluster-version-operator/blob/ed67bf413cc1765f36c31da22a72950bf9f3b7eb/docs/dev/clusteroperator.md#how-should-i-include-clusteroperator-custom-resource-in-manifests
|
/test e2e-aws-upgrade |
|
@enxebre: The specified target(s) for
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/test-infra repository. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@enxebre: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@enxebre: All pull requests linked via external trackers have merged: openshift/cluster-machine-approver#71. Bugzilla bug 1794678 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. |
Since we added support for the machine approver cluster operator, we want to validate it works as expected. openshift/cluster-machine-approver#71
Since we added support for the machine approver cluster operator, we want to validate it works as expected. openshift/cluster-machine-approver#71
Since we added support for the machine approver cluster operator, we want to validate it works as expected. openshift/cluster-machine-approver#71
We've used empty-string reasons since the status handling landed in ed1c807 (CO status controller: Add logic to sync status, 2020-04-22, openshift#71). But if we feel like we have a message we want to set to help humans understand the available condition, we should be setting a reason string for machines too. AsExpected follows the library-go precedent [1]. Also use AsExpected for some message-less but expected conditions, like Progressing=False and Degraded=False. Having a reason makes it easier for folks skimming logs to see that the condition is in its happy state, and doesn't cost us any effort to set. I am looking forward to the smarter logic promised by 91a58e6 (CO status controller: Create controller skeleton, 2020-04-22, openshift#71), because the current blind assumption that everything is working seems... very optimistic ;). [1]: https://github.com/openshift/library-go/blob/94c59dec54be25c8527e51e8c0a885712aeb01b5/pkg/operator/status/condition.go#L67
We've used empty-string reasons since the status handling landed in ed1c807 (CO status controller: Add logic to sync status, 2020-04-22, openshift#71). But if we feel like we have a message we want to set to help humans understand the available condition, we should be setting a reason string for machines too. AsExpected follows the library-go precedent [1]. Also use AsExpected for some message-less but expected conditions, like Progressing=False and Degraded=False. Having a reason makes it easier for folks skimming logs to see that the condition is in its happy state, and doesn't cost us any effort to set. I am looking forward to the smarter logic promised by 91a58e6 (CO status controller: Create controller skeleton, 2020-04-22, openshift#71), because the current blind assumption that everything is working seems... very optimistic ;). [1]: https://github.com/openshift/library-go/blob/94c59dec54be25c8527e51e8c0a885712aeb01b5/pkg/operator/status/condition.go#L67
This adds a controller to manage the clusterOperator status for the cluster-machine-approver which is currently missing.
Reconciliation is driven by a
status.VersionGetterand a single object informer watcher."Available" is always reported atm but this enables the base to set degraded in desired scenarios in the near future e.g too many pending CSRs.
This also enables integration with must gather.