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.

Sign up for GitHub

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

Jump to bottom

[clusterapi] Add support for MachinePools #4676

Merged
merged 1 commit into from
Apr 12, 2023
Merged

[clusterapi] Add support for MachinePools #4676

merged 1 commit into from
Apr 12, 2023

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Feb 10, 2022 •
edited
Loading

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for MachinePools in the cluster-api backend to cluster-autoscaler. MachinePools require individual Machine resources to be represented in order for the cluster-api backend to manage the pools (the way it currently does MachineDeployments).

This functionality is turned on by the --clusterapi-machinepool-machines feature flag default if MachinePool support is detected.

MachinePool Machines are a CAEP, and this PR is intended as a proof-of-concept to illuminate that discussion.

Which issue(s) this PR fixes:

Refs kubernetes-sigs/cluster-api#6088
See also kubernetes-sigs/cluster-api/pull#7938

Special notes for your reviewer:

Does this PR introduce a user-facing change?

[clusterapi] Add support for MachinePools

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Credit here also goes to @CecileRobertMichon, who started this code almost a year ago, and to @Jont828 who helped finish and test it.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2022
@mboersma mboersma changed the title [clusterapi] Add support for MachinePools [WIP] [clusterapi] Add support for MachinePools Feb 10, 2022
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 10, 2022
@elmiko elmiko mentioned this pull request Feb 15, 2022
Closed
@mboersma mboersma force-pushed the cluster-api-machinepools branch from e9d0875 to 7d4a12f Compare February 16, 2022 17:31
@mboersma mboersma changed the title [WIP] [clusterapi] Add support for MachinePools [clusterapi] Add support for MachinePools Feb 16, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2022
@mboersma mboersma mentioned this pull request Feb 16, 2022
Merged
elmiko
elmiko reviewed Mar 8, 2022
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of this makes sense to me, but i do have a concern about adding another flag.

cluster-autoscaler/main.go Outdated Show resolved Hide resolved
@dntosas
Copy link

dntosas commented Apr 5, 2022

hey @mboersma very nice work!! any updates in here?

@mboersma
Copy link
Contributor Author

any updates in here?

Eek, sorry! I've let this dangle for a while because it depends on kubernetes-sigs/cluster-api#6088 which is making its way toward consensus and merge still.

Having said that, I think this could be mergeable separately if I can implement @elmiko's comment above about auto-detecting MachinePool support rather than adding a new command-line argument. I'll try to get to that this week, thanks for the nudge!

@OmkarDeshpande7
Copy link

Hey @mboersma Thanks for these changes. Any updates since kubernetes-sigs/cluster-api#6088 is merged now?

@mboersma
Copy link
Contributor Author

mboersma commented Jun 3, 2022

@OmkarDeshpande7 I'm working on updating the actual MachinePool Machines implementation in CAPI (and CAPZ and perhaps CAPA). Once that's merged, this PR will finally make sense.

Sorry to have let it dangle so long! I hope to have all the work done in the next two weeks, including this PR.

@davidmortiz
Copy link

Any updates on this? Support for MachinePools in autoscaler would be a killer feature for us and would really simplify how we manage node groups.

@mboersma
Copy link
Contributor Author

mboersma commented Aug 8, 2022

@davidmortiz apologies, this is still waiting on the CAPI implementation of MachinePool Machines, which I am working on currently.

@mboersma
Copy link
Contributor Author

mboersma commented Aug 8, 2022

/assign

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2022
@anmolsachan anmolsachan mentioned this pull request Oct 11, 2022
Closed
@k8s-triage-robot
Copy link

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR 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 Nov 16, 2022
@AverageMarcus
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 Nov 17, 2022
@elmiko
Copy link
Contributor

elmiko commented Apr 5, 2023

should we squash before merge?

yeah, i think that would be nice since it's only 2 commits

@mboersma mboersma force-pushed the cluster-api-machinepools branch from f4c2aa8 to 03ea4bd Compare April 5, 2023 19:14
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2023
@elmiko
Copy link
Contributor

elmiko commented Apr 10, 2023 •
edited
Loading

@mboersma i think we were meant to merge this last friday (apologies). would you mind fixing up the last doc comment and then squashing the commits?

i'll merge after that.

@mboersma mboersma force-pushed the cluster-api-machinepools branch from 03ea4bd to 15ae863 Compare April 10, 2023 21:44
elmiko
elmiko reviewed Apr 11, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mboersma , i was about to approve and doing a final review and just have a question about a couple checks being removed.

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go Show resolved Hide resolved
cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go Show resolved Hide resolved
@mboersma mboersma force-pushed the cluster-api-machinepools branch 2 times, most recently from 5a198c3 to 182aeab Compare April 11, 2023 19:47
elmiko
elmiko reviewed Apr 11, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is looking pretty good, i'll take one more review tomorrow with fresh eyes. thanks again for the last minute changes @mboersma

elmiko
elmiko reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies @mboersma , i found one more line that i have a question about

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go Outdated Show resolved Hide resolved
@mboersma mboersma force-pushed the cluster-api-machinepools branch from 182aeab to 17d2bd9 Compare April 12, 2023 15:23
elmiko
elmiko reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all the hard work @mboersma , i think this is looking good.

/cancel hold
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko, mboersma

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2023
CecileRobertMichon
CecileRobertMichon reviewed Apr 12, 2023
View reviewed changes
Copy link
Member

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023
@elmiko
Copy link
Contributor

elmiko commented Apr 12, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1a37590 into kubernetes:master Apr 12, 2023
@jackfrancis jackfrancis mentioned this pull request May 17, 2023
Merged
jonathanmeier5 added a commit to jonathanmeier5/eks-anywhere-build-tooling that referenced this pull request May 22, 2023
@jonathanmeier5
Fix 1-27 Cluster Autoscaler Reconciling Error
028d74e 
kubernetes/autoscaler#4676 introduced a bug to the cluster autoscaler clusterapi reconciler.

The cluster autoscaler executable attempts to reconcile MachinePool resources, but does not have sufficient permissions to do so in its ClusterRole.
@jonathanmeier5 jonathanmeier5 mentioned this pull request May 22, 2023
Merged
eks-distro-bot pushed a commit to aws/eks-anywhere-build-tooling that referenced this pull request May 23, 2023
@jonathanmeier5
Fix 1-27 Cluster Autoscaler Reconciling Error (#2189)
a709860 
kubernetes/autoscaler#4676 introduced a bug to the cluster autoscaler clusterapi reconciler.

The cluster autoscaler executable attempts to reconcile MachinePool resources, but does not have sufficient permissions to do so in its ClusterRole.
@jonathanmeier5 jonathanmeier5 mentioned this pull request May 23, 2023
Closed
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 8, 2023
@enxebre
Update autocaler RBAC to accomodate machinepools support added upstream
23c072b 
This is needed to let the autoscaler to operate openshift/kubernetes-autoscaler#256 (comment)
kubernetes/autoscaler#4676
@enxebre enxebre mentioned this pull request Jun 8, 2023
Merged
4 tasks
enxebre added a commit to enxebre/hypershift that referenced this pull request Jun 14, 2023
@enxebre
Update autocaler RBAC to accomodate machinepools support added upstream
b864dee 
This is needed to let the autoscaler to operate openshift/kubernetes-autoscaler#256 (comment)
kubernetes/autoscaler#4676
orenc1 pushed a commit to orenc1/hypershift that referenced this pull request Jun 14, 2023
@enxebre @orenc1
Update autocaler RBAC to accomodate machinepools support added upstream
e9df021 
This is needed to let the autoscaler to operate openshift/kubernetes-autoscaler#256 (comment)
kubernetes/autoscaler#4676
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@com6056 com6056 com6056 left review comments

@enxebre enxebre enxebre left review comments

@elmiko elmiko elmiko left review comments

@aleksandra-malinowska aleksandra-malinowska Awaiting requested review from aleksandra-malinowska

Assignees

@mboersma mboersma

@enxebre enxebre

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler area/provider/cluster-api Issues or PRs related to Cluster API provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.