cluster-api
cluster-api copied to clipboard
✨ Add KCP feature to clusterctl alpha rollout
Signed-off-by: Tobias Giese [email protected]
What this PR does / why we need it:
This PR implements the sub-commands rollout, pause and resume for clusterctl alpha rollout
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #3439 (replaces #6857)
/test pull-cluster-api-e2e-informing-main
flaky CC test
One question: Does making use of RolloutAfter limit the functionality for RolloutAfter.
E.g. a user may have already set RolloutAfter to a timestamp in the future (e.g. to enforce recreation of the control plane to renew certificates).
So what would happen if RolloutAfter is already set (and because of that we should not overwrite it?)
Edit: so I currently think we should not make use of RolloutAfter for clusterctl alpha rollout.
One question: Does making use of
RolloutAfterlimit the functionality for RolloutAfter.
AFAIK the RolloutAfter value will be overriden. Maybe we have to change the update behavior of the KCP? E.g., make use of the cluster.x-k8s.io/restartedAt for the KCP as well. In this case we can leave the RolloutAfter exclusively for the actual use case. This would also fit to #7053.
cc @ykakarap
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval by writing /assign @justinsb in a comment. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@tobiasgiese: PR needs rebase.
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.
@ykakarap @sbueringer do you know what current state of this PR is?
I also have lost track of this work, but before reviving the work I would like to understand if we have an answer to "how can we prevent the rollback to transition the cluster to an invalid state", which can happen e.g. if we rollback the Kubernetes version (which is an operation not supported by kubeadm/withe many tricky edge cases in Kubernetes itself)
I also have lost track of this work, but before reviving the work I would like to understand if we have an answer to "how can we prevent the rollback to transition the cluster to an invalid state", which can happen e.g. if we rollback the Kubernetes version (which is an operation not supported by kubeadm/withe many tricky edge cases in Kubernetes itself)
What do you mean with rollback exactly? This change will just trigger a re-creation of the KCP as is and is -- at least to my understanding -- unrelated to a rollback (or the chance of a rollback). Or do I miss something here?
I am not sure how often users set a rollout after to much later date and just rely on that value. If we use rolloutAfter to trigger a rollou, the user basically lost the date that was set on the KCP object.
We could validate if rolloutAfter is set and the timestamp is in the future. If so, we can forbid this rollout command and add a note that the user should remove it first. Maybe this is a feasible approach?
I am not sure how often users set a rollout after to much later date and just rely on that value. If we use rolloutAfter to trigger a rollou, the user basically lost the date that was set on the KCP object.
We could validate if
rolloutAfteris set and the timestamp is in the future. If so, we can forbid this rollout command and add a note that the user should remove it first. Maybe this is a feasible approach?
That sounds feasible. Throw a warning/error message that rolloutAfter is already set and adding more verbiage around what the user needs to do sounds like a good option.
@ykakarap updated the PR as discussed
LGTM pending rebase.
LGTM pending rebase.
Rebased 👍🏻
Thank you for working on this!
/lgtm
/assign @fabriziopandini
LGTM label has been added.
last nit from my side, otherwise lgtm
Thx!
/lgtm /assign @fabriziopandini
LGTM label has been added.
/retest
/lgtm
/hold cancel
/hold
sorry, overlooked your hold Vince
Will solve the merge conflict after the review from @fabriziopandini
@tobiasgiese Can you please rebase the PR? I would merge then
@tobiasgiese Can you please rebase the PR? I would merge then
Done, thank you 👍🏻
Thank you very much!! (especially for the patience :))
/lgtm /approve
LGTM label has been added.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: sbueringer, vincepri
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [sbueringer,vincepri]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
/hold cancel
thanks for pushing this over the line @tobiasgiese!