Skip to content
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

OTA-541: enhancements/update/do-not-block-on-degraded: New enhancement proposal #1719

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wking
Copy link
Member

@wking wking commented Nov 25, 2024

The cluster-version operator (CVO) uses an update-mode when transitioning between releases, where the manifest operands are sorted into a task-node graph, and the CVO walks the graph reconciling. Since 4.1, the cluster-version operator has blocked during update and reconcile modes (but not during install mode) on Degraded=True ClusterOperator. This enhancement proposes ignoring Degraded when deciding whether to block on a ClusterOperator manifest.

The goal of blocking on manifests with sad resources is to avoid further destabilization. For example, if we have not reconciled a namespace manifest or ServiceAccount RoleBinding, there's no point in trying to update the consuming operator Deployment. Or if we are unable to update the Kube-API-server operator, we don't want to inject unsupported kubelet skew by asking the machine-config operator to update nodes.

However, blocking the update on a sad resource has the downside that later manifest-graph task-nodes are not reconciled, while the CVO waits for the sad resource to return to happiness. We maximize safety by blocking when progress would be risky, while continuing when progress would be safe, and possibly helpful.

Our expirience with Degraded=True blocks turns up cases where blocking is not helpful, so this enhancement proposes no longer blocking on that condition. We will conditinue to block on Available=False ClusterOperator, or when the ClusterOperator versions have not yet reached the values requested by the ClusterOperator's release manifest.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 25, 2024

@wking: This pull request references OTA-541 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The cluster-version operator (CVO) uses an update-mode when transitioning between releases, where the manifest operands are sorted into a task-node graph, and the CVO walks the graph reconciling. Since 4.1, the cluster-version operator has blocked during update and reconcile modes (but not during install mode) on Degraded=True ClusterOperator. This enhancement proposes ignoring Degraded when deciding whether to block on a ClusterOperator manifest.

The goal of blocking on manifests with sad resources is to avoid further destabilization. For example, if we have not reconciled a namespace manifest or ServiceAccount RoleBinding, there's no point in trying to update the consuming operator Deployment. Or if we are unable to update the Kube-API-server operator, we don't want to inject unsupported kubelet skew by asking the machine-config operator to update nodes.

However, blocking the update on a sad resource has the downside that later manifest-graph task-nodes are not reconciled, while the CVO waits for the sad resource to return to happiness. We maximize safety by blocking when progress would be risky, while continuing when progress would be safe, and possibly helpful.

Our expirience with Degraded=True blocks turns up cases where blocking is not helpful, so this enhancement proposes no longer blocking on that condition. We will conditinue to block on Available=False ClusterOperator, or when the ClusterOperator versions have not yet reached the values requested by the ClusterOperator's release manifest.

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 openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 25, 2024
@wking wking force-pushed the do-not-block-updates-on-ClusterOperator-degraded branch from b0c8d2e to 69eca53 Compare November 25, 2024 20:55
## Proposal

The cluster-version operator currently has [a mode switch][cvo-degraded-mode-switch] that makes `Degraded` ClusterOperator a non-blocking condition that is still proagated through to `Failing`.
This enhancement proposes making that an unconditional `UpdateEffectReport`, regardless of the CVO's current mode (installing, updating, reconciling, etc.).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openshift/cluster-version-operator#482 is in flight with this change, if folks want to test pre-merge.

@petr-muller
Copy link
Member

/cc

@openshift-ci openshift-ci bot requested a review from petr-muller November 26, 2024 12:30
@wking wking force-pushed the do-not-block-updates-on-ClusterOperator-degraded branch from 69eca53 to 11f8243 Compare November 26, 2024 18:29

### Goals

ClusterVersion updates will no longer block on ClusterOperators solely based on `Degraded=True`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it mean, if no operator is unavailable, then the upgrade should always complete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClusterOperators aren't the only CVO-manifested resources, and if something else breaks like we fail to reconcile a RoleBinding or whatever, that will block further update progress. And for ClusterOperators, we'll still block on status.versions not being as far along as the manifest claimed, in addition to blocking if Available isn't True. Personally, status.versions seems like the main thing that's relevant, e.g. a component coming after the Kube API server knows it can use 4.18 APIs if the Kube API server has declared 4.18 versions. As an example of what the 4.18 Kube API server asks the CVO to wait on:

$ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.18.0-rc.0-x86_64
Extracted release payload from digest sha256:054e75395dd0879e8c29cd059cf6b782742123177a303910bf78f28880431d1c created at 2024-12-02T21:11:00Z
$ yaml2json <manifests/0000_20_kube-apiserver-operator_07_clusteroperator.yaml | jq -c '.status.versions[]'
{"name":"operator","version":"4.18.0-rc.0"}
{"name":"raw-internal","version":"4.18.0-rc.0"}
{"name":"kube-apiserver","version":"1.31.3"}

A recent example of this being useful is openshift/machine-config-operator#4637, which got the CVO to block until the MCO had rolled out a single-arch -> multi-arch transition, without the MCO needing to touch its Degraded or Available conditions to slow the CVO down.

Copy link

@jiajliu jiajliu Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so could I say, if failing=true for an upgrade, the reason should not be ClusterOperatorDegraded only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we'll still propagate ClusterOperator(s)Degraded through to Failing, it just will no longer block the update's progress. So if the only issue Failing is talking about is ClusterOperator(s)Degraded, we expect the update to be moving towards completion, and not stalling.

@wking wking force-pushed the do-not-block-updates-on-ClusterOperator-degraded branch from 11f8243 to e10df2a Compare December 3, 2024 20:51

## Test Plan

**Note:** *Section not required until targeted at a release.*
Copy link
Contributor

@DavidHurta DavidHurta Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enhancement and the tracking card OTA-541 are not targeted at a release. However, changes in the dev-guide/cluster-version-operator/user/reconciliation.md file suggest that the enhancement is targeted at the 4.19 release, and thus the Test Plan section should be addressed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not strongly opinionated on what the test plan looks like. We don't do a lot of intentional-sad-path update testing today in CI, and I'm fuzzy on what QE does in that space that could be expanded into this new space (or maybe they already test pushing a ClusterOperator component to Degraded=True mid update to see how the cluster handles that?).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test pushing a ClusterOperator component to Degraded=True mid update to see how the cluster handles that?

+1, that's also what I want to explore during test. I also had some other immature checkpoints in my mind when I read this enhancement doc at the first time, but I still need some inputs from @wking to help me tidy up them. For example #1719 (comment).
I asked this because there's already some cv.conditions check in CI, I'm thinking about if we could update the logic to help catching issues once the feature implemented.

@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2025
…ew enhancement proposal

The cluster-version operator (CVO) uses an update-mode when
transitioning between releases, where the manifest operands are sorted
into a task-node graph, and the CVO walks the graph reconciling.
Since 4.1, the cluster-version operator has blocked during update and
reconcile modes (but not during install mode) on Degraded=True
ClusterOperator.  This enhancement proposes ignoring Degraded when
deciding whether to block on a ClusterOperator manifest.

The goal of blocking on manifests with sad resources is to avoid
further destabilization.  For example, if we have not reconciled a
namespace manifest or ServiceAccount RoleBinding, there's no point in
trying to update the consuming operator Deployment.  Or if we are
unable to update the Kube-API-server operator, we don't want to inject
unsupported kubelet skew by asking the machine-config operator to
update nodes.

However, blocking the update on a sad resource has the downside that
later manifest-graph task-nodes are not reconciled, while the CVO
waits for the sad resource to return to happiness.  We maximize safety
by blocking when progress would be risky, while continuing when
progress would be safe, and possibly helpful.

Our expirience with Degraded=True blocks turns up cases where blocking
is not helpful, so this enhancement proposes no longer blocking on
that condition.  We will conditinue to block on Available=False
ClusterOperator, or when the ClusterOperator versions have not yet
reached the values requested by the ClusterOperator's release
manifest.
@wking wking force-pushed the do-not-block-updates-on-ClusterOperator-degraded branch from e10df2a to 9498fb9 Compare January 9, 2025 23:49
Copy link
Contributor

openshift-ci bot commented Jan 9, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: petr-muller
Once this PR has been reviewed and has the lgtm label, please assign pratikmahajan for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment


The goal of blocking on manifests with sad resources is to avoid further destabilization.
For example, if we have not reconciled a namespace manifest or ServiceAccount RoleBinding, there's no point in trying to update the consuming operator Deployment.
Or if we are unable to update the Kube-API-server operator, we don't want to inject [unsupported kubelet skew][kubelet-skew] by asking the machine-config operator to update nodes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we observed another kind of upgrade blocker here. Applying the infrastructures.config.openshift.io manifest failed as the CRD had introduced some validations and that needed the apiserver to be upgraded to support it. Unfortunately, the upgrade didn't progress and we had to manually step in to update the kube-apiserver to let the upgrade proceed. Is there a way to enhance these cases to at least let the apiserver upgrade before blocking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to enhance these cases...

I've been trying to talk folks into the narrow Degraded handling pivot this enhancement currently covers since 2021. I accept that there may be other changes that we could make to help updates go more smoothly, but I'd personally rather limit the scope of this enhancement to the Degraded handling.

…pproval list

David Eads suggested these acks to avoid surprising anyone.  List
generated with:

  $ oc adm release extract --to manifests quay.io/openshift-release-dev/ocp-release:4.19.0-ec.0-x86_64
  $ grep -rl 'kind: ClusterOperator' manifests | while read MANIFEST; do yaml2json < "${MANIFEST}" | jq -r '.[] | select(.kind == "ClusterOperator").metadata.name'; done | sort | uniq
@wking wking force-pushed the do-not-block-updates-on-ClusterOperator-degraded branch from c6616a3 to 111c8fe Compare January 14, 2025 21:48
@openshift-bot
Copy link

Stale enhancement proposals rot after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Rotten proposals close after an additional 7d of inactivity.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 22, 2025

> There MUST be a version with the name `operator`, which is watched by the CVO to know if a cluster operator has achieved the new level.

* [ ] authentication
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty new to the code base for cluster-authentication-operator but scanning through the code there is nothing that stands out in this operator that is concerning with this change.

Ack from @liouk or @ibihim would also be nice to have as an additional sanity check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking internal org docs, the Auth team seems like they might be responsible for the service-ca ClusterOperator, in addition to this line's authentication ClusterOperator. In case those maintainers want to comment with something like:

I approve this pull and the existing status.versions[name=operator] semantics for the following ClusterOperators, where I'm a maintainer: authentication, service-ca.

or whatever, assuming they are ok making that assertion for the operators they maintain. Also fine if they want to say "I'm a maintainer for $CLUSTER_OPERATORS, and I'm not ok with this enhancement as it stands, because..." or whatever, I'm just trying to give folks a way to satisfy David's requested sign-off if they do happen to be on board.

@wking
Copy link
Member Author

wking commented Jan 27, 2025

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 27, 2025
@petr-muller
Copy link
Member

Note that a lot of MCO degrades will be due to a node unable to update, which means that a new incoming update will likely be similarly blocked until the MCO degrade is cleared.

This is one of the cases we actually want to address by the change, we see this happen with busy clusters (like CI build farms):

  1. MCO starts updating MCPs
  2. One of the workers that are supposed to drain have a pod protected by strict PDB, so after ~1h timeout MCO gets degraded because of the drain failure
  3. Master nodes update slowly and all three take >1h to complete their update, so MCO only flips its version after that, and MCO is degraded already
  4. The degraded worker pool (because of the slow drain) keeps the control plane update in progress because CVO waits for the degraded to clear

So in this scenario the only difference in whether CVO is able to "close" the update is whether the masters manage to update before the first to-be-drained worker starts to fire its "fail to drain" alert and degrade MCO. If before -> CVO completes control plane update, MCO goes degraded, stays that way for some time, eventually finishes (or not, depending on drain success). If after -> CVO keeps control plane update progressing until MCO is healthy again.

@yuqi-zhang
Copy link
Contributor

Makes sense, consistency is good in this case 👍

Copy link
Contributor

openshift-ci bot commented Jan 31, 2025

@wking: all tests passed!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants