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

MachinePool experiment API group should be under cluster.x-k8s.io #3424

Closed
vincepri opened this issue Jul 30, 2020 · 18 comments · Fixed by #4574
Closed

MachinePool experiment API group should be under cluster.x-k8s.io #3424

vincepri opened this issue Jul 30, 2020 · 18 comments · Fixed by #4574
Assignees
Labels
area/api Issues or PRs related to the APIs help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/release-blocking Issues or PRs that need to be closed before the next CAPI release
Milestone

Comments

@vincepri
Copy link
Member

MachinePool is today an experiment, and the API group we originally decided to pick was exp.cluster.x-k8s.io. Given that the intent is in the future to move MachinePool to the core API group, we should rewrite the experiment to use cluster.x-k8s.io or whatever the final group should be.

/kind cleanup
/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 30, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Jul 30, 2020
@rudoi
Copy link
Contributor

rudoi commented Jul 30, 2020

/assign @mytunguyen
/lifecycle active

@k8s-ci-robot
Copy link
Contributor

@rudoi: GitHub didn't allow me to assign the following users: mytunguyen.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mytunguyen
/lifecycle active

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 k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jul 30, 2020
@rudoi
Copy link
Contributor

rudoi commented Jul 30, 2020

yeah yeah CI bot, we know :(

@vincepri
Copy link
Member Author

@mytunguyen You should open an issue in k8s-sigs to the into the org, I'm happy to sponsor as well.

One thing to note, this would be for v1alpha4 and we haven't opened the main branch yet, if you want to get a head start please do :), just a warning that it might have to be rebased later

@vincepri
Copy link
Member Author

/area api

@k8s-ci-robot k8s-ci-robot added the area/api Issues or PRs related to the APIs label Jul 30, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

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

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels Oct 28, 2020
@fabriziopandini
Copy link
Member

/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 Oct 29, 2020
@fabriziopandini
Copy link
Member

/kind api-change

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 12, 2020
@CecileRobertMichon
Copy link
Contributor

/assign

@CecileRobertMichon
Copy link
Contributor

@vincepri any thoughts on how to do this without completely breaking existing machine pool users? If we change the group now, existing machine pools won't be able to upgrade to new versions. Is that acceptable given that the feature was in exp until now?

@vincepri
Copy link
Member Author

vincepri commented Jan 4, 2021

Yeah better to do this now, rather than later. We need to document the transition. There isn't any good way to do this without breaking user in one way or another; we could have a controller that looks for the old resources and recreates them, although that might cause the infrastructure to be deleted and recreated, unless maybe we have some way to make it seamless, which we might be able to do with MachinePool, given that machines are "virtual".

One way we could approach this problem:

  • Have two reconcilers, one for the old group and one for the new one.
  • Have a webhook that disallows create (and maybe update as well?) operations on the old API group. We should return a nice message that the group has changed.
  • When the old group reconciler sees a resource, it creates a new one in the new group (if it doesn't exist), with the paused annotation.
  • The old resource should be forcibly deleted by removing any finalizer.
  • Remove the paused annotation from the new resource.

@detiber
Copy link
Member

detiber commented Jan 4, 2021

We could leverage clusterctl to provide a custom migration step to re-write the resources safely rather than a controller, especially since it should be short lived.

Longer term, we could also potentially leverage the management cluster operator (?? can't remember what naming was decided upon) to perform these types of actions.

I do think it would be good to make sure that we are publishing the old CRD info and have webhook validation to reject requests with a friendly message.

@CecileRobertMichon
Copy link
Contributor

IMO, a reconciler to migrate the CRDs is a bit overkill for an exp/ feature in an alpha API. We would be introducing more complexity and more code to maintain. I'm leaning towards ripping off the band-aid now and avoiding making the same mistake in the future with other exp APIs.

We could leverage clusterctl to provide a custom migration step to re-write the resources safely rather than a controller, especially since it should be short lived.

That seems like a reasonable compromise to me, especially if any users speak up and tell us they would suffer from a breaking change on MachinePools.

I do think it would be good to make sure that we are publishing the old CRD info and have webhook validation to reject requests with a friendly message.

Definitely.

/cc @devigned

@vincepri
Copy link
Member Author

@CecileRobertMichon @devigned Should we do this in v0.4.0, as release blocker?

@CecileRobertMichon
Copy link
Contributor

/unassign
I'm not actively working on this

@nprokopic you had expressed interest in keeping back compat for MachinePool in office hours if I recall correctly.
Also cc @dthorsen since I know your team uses MachinePools.

This change is going to be breaking so we need to get it in before v0.4.0 I believe. Anyone interested in working on this?

@CecileRobertMichon
Copy link
Contributor

/kind release-blocking
/help

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/kind release-blocking
/help

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 k8s-ci-robot added kind/release-blocking Issues or PRs that need to be closed before the next CAPI release help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Apr 5, 2021
@vincepri
Copy link
Member Author

During April 28th community meeting, most folks were ok with a breaking change without a migration path. We should definitely document the change and communicate it.

/assign @CecileRobertMichon
/assign @yastij to send an email to the mailing list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Issues or PRs related to the APIs help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/release-blocking Issues or PRs that need to be closed before the next CAPI release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants