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

Support clusterctl alpha rollout kthreescontrolplane/... #10621

Closed
wikoion opened this issue May 15, 2024 · 19 comments · May be fixed by #10648
Closed

Support clusterctl alpha rollout kthreescontrolplane/... #10621

wikoion opened this issue May 15, 2024 · 19 comments · May be fixed by #10648
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates an issue lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@wikoion
Copy link

wikoion commented May 15, 2024

What would you like to be added (User Story)?

As a developer I would like to add support to clusterctl to rollout kthreescontrolplanes, similar to the support for kubeadm controlplanes.

Detailed Description

I'd like to discuss approach, I'm happy to do the work, but wanted to get some advice on how we could implement this. The easy option is something like this, but I understand that it might not be desireable to import these external packages. If that is the case could we refactor rollout to support any kind of resource that implementes rolloutAfter in it's spec? Currently cluster-api-k3s uses upgradeAfter, but if we chose this option we could add another api version similar to what was done in this project.

Anything else you would like to add?

No response

Label(s) to be applied

/kind feature
One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-priority Indicates an issue lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 15, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If CAPI contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@neolit123
Copy link
Member

refactor rollout to support any kind of resource

makes sense to me.
maybe the rollout command should be CP provider agnostic.

@sbueringer
Copy link
Member

sbueringer commented May 16, 2024

I think we should not start hard-coding non-"CAPI core" providers

@fabriziopandini
Copy link
Member

Agreed, and considering #10479, we should not add more on top of the clusterctl rollout commands.

@sbueringer
Copy link
Member

I think triggering a rollout should probably not be part of #10479 as we were not planning to deprecate the rolloutAfter fields

@fabriziopandini
Copy link
Member

fabriziopandini commented May 22, 2024

I think triggering a rollout should probably not be part of #10479 as we were not planning to deprecate the rolloutAfter fields

You are correct, sorry I wasn't specific in my reference.
What I was thinking is the fact that most of the considerations in the linked issue applies also here:

  • This feature (afaik) has been mostly copied from k/k Deployments & ReplicaSets
  • As far as we are aware it is barely used by CAPI users (we barely get any questions / requests about the entire feature)
  • It's not usable with GitOps
  • It's not usable with ClusterClass

Let me also add on top of that that AFAIK there is not a contract defining rolloutAfter field and as far as I'm aware of it is not used consistently. e.g. also in CAPI core, machines pools do not use it.

In other words, the idea to extend what we copied from k/k Deployments & ReplicaSets lays to other objects seems based on brittle foundations, and I wonder if also in this case we should really consider if it's worth the complexity and otherwise consider deprecation.

@wikoion
Copy link
Author

wikoion commented May 22, 2024

I think triggering a rollout should probably not be part of #10479 as we were not planning to deprecate the rolloutAfter fields

You are correct, sorry I wasn't specific in my reference. What I was thinking is the fact that most of the considerations in the linked issue applies also here:

* This feature (afaik) has been mostly copied from k/k Deployments & ReplicaSets

* As far as we are aware it is barely used by CAPI users  (we barely get any questions / requests about the entire feature)

* It's not usable with GitOps

* It's not usable with ClusterClass

Let me also add on top of that that AFAIK there is not a contract defining rolloutAfter field and as far as I'm aware of it is not used consistently. e.g. also in CAPI core, machines pools do not use it.

In other words, the idea to extend what we copied from k/k Deployments & ReplicaSets lays to other objects seems based on brittle foundations, and I wonder if also in this case we should really consider if it's worth the complexity and otherwise consider deprecation.

For what it's worth, we use this feature at Influx. It is very useful to be able to perform a rolling restart of machinedeployments, in the same way that it's useful to be able to perform a rolling restart of a kubernetes deployment or sts. When we replace an AWSMachineTemplate we have to roll the machinedeployments for example

@sbueringer
Copy link
Member

sbueringer commented May 22, 2024

I would really like to keep the existing rolloutAfter fields on MD & KCP. I think we don't get much requests because they simply work.

For MachineDeployments it is the only way to trigger a Machine rollout without making substantial changes (labels & annotations don't trigger rollouts anymore, which is why we introduced rolloutAfter for MachineDeployments). Not sure about KCP, but it might be the same there.

I would assume rolloutAfter also works with GitOps.

@fabriziopandini
Copy link
Member

I would really like to keep the existing rolloutAfter fields on MD & KCP. I think we don't get much requests because they simply work.

I never said we have to remove those fields from MD & KCP.
I'm just thinking about the clusterctl rollout command, and most specifically about the fact that we should not extend it outside of the current scope (and btw, the KCP implementation, which is in the current scope, is not based on any contract)

@sbueringer
Copy link
Member

Ah got it. I interpreted the comment here like we should drop rolloutAfter as well: #10621 (comment)

I think the reason we merged the KCP support at the time was because KCP is also part of core CAPI (which I think is also the reason why we found it okay to handle KubeadmConfig / KCP specifically in some of our core controllers).

@fabriziopandini
Copy link
Member

Given that the consensus is to not expand this other than KCP (special case), I'm leaning toward closing this issue and the corresponding PR

@wikoion
Copy link
Author

wikoion commented Jun 6, 2024

I would like to argue again for this being a useful feature of clusterctl and is frequently used for machine deployments, would be very useful to also be able to do it with our controlplanes (k3s)

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 4, 2024
@vaibhav2107
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 10, 2024
@fabriziopandini
Copy link
Member

@vaibhav2107 what is the reason to remove lifecycle stale?
It is expected, and healthy for the project, that if issue that are not active gets closed by the automation. This helps contributors and maintainers to focus on the ongoing work

@fabriziopandini
Copy link
Member

talked with @vaibhav2107, label was removed by mistake

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 10, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot 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 Oct 10, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2024
@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates an issue lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
7 participants