-
Notifications
You must be signed in to change notification settings - Fork 1.9k
OSDOCS-2428: Adding CVO conditions #35453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
✔️ Deploy Preview for osdocs ready! 🔨 Explore the source changes: 5c377fe 🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/61a63a50ed7a0e0008439369 😎 Browse the preview: https://deploy-preview-35453--osdocs.netlify.app/openshift-enterprise/latest/updating/understanding-the-update-service |
wking
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a bunch of nits inline, but overall looks good to me, and will be nice to have these in our official docs :). Apologies for the nits and stale bits that you're inheriting from the CVO's local docs... Among which, I don't think this current list is complete, but haven't run a full audit of our current code. We could do that audit, but it would take some time, and I dunno if we need to be complete before landing something is useful. I'm also not clear if this is really an openshift-docs thing, or if we'd be better off landing this in runbooks, where we can link it from the alerts like CannotRetrieveUpdates that will go off when cluster is sad. Recent CVO work has overhauled those alerts, and perhaps things like this are now sufficiently self-describing that we don't need to cover them independently in docs? Or maybe there are some subset of especially-confusing types/reasons that are worth calling out in docs, beyond what can be covered by runbooks and alerts?
40f322e to
9e2dd47
Compare
@wking, the content drastically changed since the last time you looked at it. I pulled out all the stale information, and mostly did it based on these two links:
Would love to get your new thoughts, if you have time :) |
|
LGTM, lets get this in. |
|
@sdodson Who is the QE that should review this? |
|
@jianlinliu is QE for OTA team, I'm unsure if they cover CVO / ClusterOperator definitions as well. |
|
@jianlinliu Could you review this from the QE side? TY |
modules/update-cvo-conditions.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/cluster-version operator/Cluster Version Operator (CVO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do these conditions appear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdodson - If I'm not mistaken the CVO conditions appear during an upgrade in the Log right? I'm not 100% sure but thought you might know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay in getting to this, they are reflected in the clusterversion object, which can be used programmatically, parts of which are expressed in the Cluster Settings page during upgrades.
oc get clusterversion -o yaml
modules/update-cvo-conditions.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a Degraded state represents persistent observation of a condition
Is condition the right word here? It seems odd that the Degraded condition reports on a condition. Maybe problem or error or such??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just drop "of a condition" all together?
modules/update-cvo-conditions.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what missing condition means. The CV is not reporting any Upgradeable condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just saying that in absence of Upgradeable=False it's assumed to be Upgradeable. Some operators may set Unknown or not set this condition while initializing, though hopefully that's brief.
|
@sagidlow A few more nits. Otherwise LGTM |
The topic is talking about CVO conditions, but the main description is talking about "CO". And go through the whole PR, sounds like there is no much description about the relationship and dependency between the status of COs and the conditions of CVO, then how it affect the final upgrade operations. And per my knowledge, CVO collects and summarizes the status of all COs, then determinate CVO conditions, sometimes, even CVO also set some conditions by itself. For example: In the above example, "Upgradeable=False" and "RetrievedUpdates=False" are set by CVO itself. BTW, this PR is also not mentioned So this PR sounds like a bit confused for user. |
9e2dd47 to
5c377fe
Compare
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
@sagidlow: PR needs rebase. DetailsInstructions 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. |
|
The This is because your PR targets the If the update in your PR does NOT apply to version 4.11 onward, please retarget this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main. |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn 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. |
Applies to 4.7+
QE ack required.
SME ack required.
Preview Link: Understanding CVO conditions
JIRA Link: https://issues.redhat.com/browse/OSDOCS-2428