Add CP orchestrated in-place upgrade controller #71
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
berkayoz
reviewed
Oct 17, 2024
| } | ||
|
|
||
| // Upgrade is locked and a machine is already upgrading | ||
| if lockedMachine != nil { |
There was a problem hiding this comment.
What happens on if upgrade is failed on the machine, do we wait/hold the lock until it succeds?
There was a problem hiding this comment.
Thanks for pointing this out, I noticed that I had to move the IsMachineUpgradeFailed check a bit higher.
In short, yes. Lemme explain a bit further:
Scenario 1: Not locked
- Let's say the upgrade process is not locked. We select a machine to upgrade (let's call it
M) and lock the upgrade process while mentioning in the lock object that machineMis upgrading.
Scenario 2: Locked and the machine is upgraded
- We see that the process is locked, so we get machine
Mand notice that it is upgraded (releaseannotation). We unlock and requeue the reconciliation.
Scenario 3: Locked and the machine is not upgraded (still upgrading)
- We requeue the reconciliation.
Scenario 4: A machine that we tried to upgrade before is not there any more (somehow deleted)
- The lock config map has the machine information and tries to obtain it but notices that it's not there, so the
IsLockedwon't return anupgradingMachine, the lock config map is deleted (Unlockis called) and we continue on to select a fresh machine to upgrade.
Scenario 5: Locked and the machine does not have any upgrade instructions
- This is a weird situation (maybe the admin deleted the annotations accidentally), but can happen nonetheless. We consider that a stale lock and Unlock and requeue.
Scenario 6: Locked and the machine upgrade failed
- We mark the
CK8sControlPlaneobject withfailedstatus and publish the respective event and retry the process (basically the underlyingsingle machine in-place upgrade reconcileris doing the retry and we just wait).
There was a problem hiding this comment.
Thanks for the list - this was very helpful for the review.
bschimke95
reviewed
Oct 17, 2024
HomayoonAlimohammadi
force-pushed
the
KU-1562/add-orchestrated-in-place-upgrade-to-cp-provider
branch
from
8e9c788 to
b4c17be
Compare
bschimke95
approved these changes
Oct 23, 2024
eaudetcobello
approved these changes
Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds the orchestrated in-place upgrade controller to the control plane provider.
After annotating the
CK8sControlPlaneobject withinplace-upgrade-toannotation, this controller will make sure that each control plane machine is upgraded to the specified version.By design, this controller does not allow parallel upgrades to ensure that the control plane stays responsive (prevent breaking HA, quorum, etc.)
Details
pkg/upgrade/inplace. We should use these functionalities for theMachineDeploymentReconcileras well (instead of separately implementing them, which is what we're doing right now).MachineDeploymentReconcilershould get renamed to resemble its responsibilities more closely (e.g.bootstrapControllers.OrchestratedInPlaceUpgradeController) in the future.