cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

✨ Add KCP feature to clusterctl alpha rollout

Open tobiasgiese opened this issue 3 years ago • 6 comments

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)

tobiasgiese avatar Jul 07 '22 14:07 tobiasgiese

/retest

tests were successful on Prow

tobiasgiese avatar Jul 07 '22 16:07 tobiasgiese

/test pull-cluster-api-e2e-informing-main

flaky CC test

tobiasgiese avatar Jul 07 '22 16:07 tobiasgiese

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.

chrischdi avatar Aug 08 '22 15:08 chrischdi

One question: Does making use of RolloutAfter limit 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.

tobiasgiese avatar Aug 13 '22 08:08 tobiasgiese

cc @ykakarap

sbueringer avatar Aug 16 '22 12:08 sbueringer

[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.

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

k8s-ci-robot avatar Oct 19 '22 09:10 k8s-ci-robot

@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.

k8s-ci-robot avatar Nov 08 '22 00:11 k8s-ci-robot

@ykakarap @sbueringer do you know what current state of this PR is?

tobiasgiese avatar Dec 06 '22 10:12 tobiasgiese

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)

fabriziopandini avatar Dec 06 '22 21:12 fabriziopandini

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?

tobiasgiese avatar Dec 07 '22 18:12 tobiasgiese

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?

tobiasgiese avatar Dec 07 '22 18:12 tobiasgiese

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?

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 avatar Dec 08 '22 22:12 ykakarap

@ykakarap updated the PR as discussed

tobiasgiese avatar Dec 14 '22 21:12 tobiasgiese

LGTM pending rebase.

ykakarap avatar Jan 13 '23 05:01 ykakarap

LGTM pending rebase.

Rebased 👍🏻

tobiasgiese avatar Jan 13 '23 08:01 tobiasgiese

Thank you for working on this!

/lgtm

/assign @fabriziopandini

ykakarap avatar Jan 14 '23 07:01 ykakarap

LGTM label has been added.

Git tree hash: 8f8e55b499d6686963300e457365b34719dfe223

k8s-ci-robot avatar Jan 14 '23 07:01 k8s-ci-robot

last nit from my side, otherwise lgtm

sbueringer avatar Jan 20 '23 11:01 sbueringer

Thx!

/lgtm /assign @fabriziopandini

sbueringer avatar Jan 20 '23 11:01 sbueringer

LGTM label has been added.

Git tree hash: fd80f3e466139572edcad38c9b0b573273afcc25

k8s-ci-robot avatar Jan 20 '23 11:01 k8s-ci-robot

/retest

sbueringer avatar Jan 20 '23 11:01 sbueringer

/lgtm

ykakarap avatar Jan 21 '23 00:01 ykakarap

/hold cancel

tobiasgiese avatar Jan 23 '23 20:01 tobiasgiese

/hold

sorry, overlooked your hold Vince

tobiasgiese avatar Jan 23 '23 20:01 tobiasgiese

Will solve the merge conflict after the review from @fabriziopandini

tobiasgiese avatar Jan 25 '23 09:01 tobiasgiese

@tobiasgiese Can you please rebase the PR? I would merge then

sbueringer avatar Jan 25 '23 12:01 sbueringer

@tobiasgiese Can you please rebase the PR? I would merge then

Done, thank you 👍🏻

tobiasgiese avatar Jan 25 '23 12:01 tobiasgiese

Thank you very much!! (especially for the patience :))

/lgtm /approve

sbueringer avatar Jan 25 '23 12:01 sbueringer

LGTM label has been added.

Git tree hash: fad73e5eb2d6bfa576ff62c98f846f5987bce042

k8s-ci-robot avatar Jan 25 '23 12:01 k8s-ci-robot

[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

Needs approval from an approver in each of these files:
  • ~~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

k8s-ci-robot avatar Jan 25 '23 12:01 k8s-ci-robot

/hold cancel

tobiasgiese avatar Jan 25 '23 12:01 tobiasgiese

thanks for pushing this over the line @tobiasgiese!

fabriziopandini avatar Jan 25 '23 14:01 fabriziopandini