Skip to content

Refactor cloud provider creation options #8583

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Refactor cloud provider creation options #8583

wants to merge 5 commits into from
+432 −204

Conversation

elmiko
Copy link
Contributor

@elmiko elmiko commented Sep 29, 2025

What type of PR is this?

/kind cleanup
/kind api-change

What this PR does / why we need it:

This patch series changes an argument to the NewCloudProvider function to use an AutoscalerOptions struct instead of AutoscalingOptions. This change allows cloud providers to have more control over the core functionality of the cluster autoscaler.

In specific, this patch series also adds a method named RegisterScaleDownNodeProcessor to the AutoscalerOptions so that cloud providers can inject a custom scale down processor.

Lastly, this change adds a custom scale down processor to the clusterapi provider to help it avoid removing the wrong instance during scale down operations that occur during a cluster upgrade.

Which issue(s) this PR fixes:

Fixes #8494

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area area/cluster-autoscaler area/provider/alicloud Issues or PRs related to the AliCloud cloud provider implementation area/provider/aws Issues or PRs related to aws provider and removed do-not-merge/needs-area labels Sep 29, 2025
@k8s-ci-robot k8s-ci-robot added the area/provider/azure Issues or PRs related to azure provider label Sep 29, 2025
@k8s-ci-robot k8s-ci-robot added area/provider/cluster-api Issues or PRs related to Cluster API provider area/provider/coreweave area/provider/digitalocean Issues or PRs related to digitalocean provider area/provider/equinixmetal Issues or PRs related to the Equinix Metal cloud provider for Cluster Autoscaler size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/provider/externalgrpc Issues or PRs related to the External gRPC provider area/provider/gce area/provider/hetzner Issues or PRs related to Hetzner provider area/provider/huaweicloud area/provider/ionoscloud area/provider/kwok Issues or PRs related to the kwok cloud provider for Cluster Autoscaler area/provider/linode Issues or PRs related to linode provider area/provider/magnum Issues or PRs related to the Magnum cloud provider for Cluster Autoscaler area/provider/oci Issues or PRs related to oci provider area/provider/rancher area/provider/utho Issues or PRs related to Utho provider labels Sep 29, 2025
@elmiko elmiko mentioned this pull request Sep 29, 2025
Closed
@elmiko
Copy link
Contributor Author

elmiko commented Sep 29, 2025

this is an alternate solution instead of #8531

@elmiko elmiko force-pushed the provider-options-refactor branch 3 times, most recently from f60b7f0 to 1afa484 Compare September 30, 2025 02:49
@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

/test pull-cluster-autoscaler-e2e-azure-master

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

the circular import in the unit tests is giving me some problems, not quite sure where it's starting.

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

i am not able to reproduce the unit test failure with the circular import locally. not sure the best course of action.

/retest

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

/retest-required

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

/retest

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

not sure why the tests aren't triggering.

@elmiko
refactor core.AutoscalerOptions in a new package
766ee87 
This change helps to prevent circular dependencies between the core and
builder packages as we start to pass the AutoscalerOptions to the cloud
provider builder functions.
@elmiko
refactor NewCloudProvider to accept AutoscalerOptions
f59084e 
this changes the options input to the cloud provider builder function so
that the full autoscaler options are passed. This is being proposed so
that cloud providers will have new options for injecting behavior into
the core parts of the autoscaler.
@elmiko
add scale down registration method
b8fb85a 
this change adds a method to the AutoscalerOptions struct to allow
registering a scale down node processor.
@elmiko
add clusterapi scale down upgrade processor
a5f07fe 
This change adds a custom scale down node processor for cluster api to
reject nodes that are undergoing upgrade.
@elmiko elmiko force-pushed the provider-options-refactor branch from 1afa484 to a5f07fe Compare September 30, 2025 19:52
@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

rebased on master

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

i'm not sure how i've broken this, but these error lines are not making sense to me:

package k8s.io/autoscaler/cluster-autoscaler/processors/customresources
	imports k8s.io/autoscaler/cluster-autoscaler/cloudprovider/gce from gpu_processor_test.go
	imports k8s.io/autoscaler/cluster-autoscaler/core/options from gce_cloud_provider.go
	imports k8s.io/autoscaler/cluster-autoscaler/processors from autoscaler.go
Error: 	imports k8s.io/autoscaler/cluster-autoscaler/processors/customresources from processors.go: import cycle not allowed in test

@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

@towca does this error looks familiar to you at all?

@elmiko
refactor gpu_processor_test to remove cyclic dependency
b82aa40 
this change removes the import from the gce module in favor of using the
string value directly.
@elmiko
Copy link
Contributor Author

elmiko commented Sep 30, 2025

ok, think my last commit on this chain fixed the circular import.

@jackfrancis jackfrancis mentioned this pull request Sep 30, 2025
Closed
@jackfrancis
Copy link
Contributor

We have a conversation going on here that will potentially improve the gpu node label concern that this PR encountered:

/lgtm
/approve
/hold

for @towca to give his thumbs up

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Oct 2, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

We have a conversation going on here that will potentially improve the gpu node label concern that this PR encountered:

i'm fine to change the label in the test for this PR, i was just trying to stop the circular import error.

@jackfrancis jackfrancis mentioned this pull request Oct 3, 2025
Open
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2025
@k8s-ci-robot
Copy link
Contributor

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackfrancis jackfrancis jackfrancis left review comments

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

@apricote apricote Awaiting requested review from apricote

+1 more reviewer

@sbueringer sbueringer sbueringer left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jackfrancis jackfrancis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler area/provider/alicloud Issues or PRs related to the AliCloud cloud provider implementation area/provider/aws Issues or PRs related to aws provider area/provider/azure Issues or PRs related to azure provider area/provider/cluster-api Issues or PRs related to Cluster API provider area/provider/coreweave area/provider/digitalocean Issues or PRs related to digitalocean provider area/provider/equinixmetal Issues or PRs related to the Equinix Metal cloud provider for Cluster Autoscaler area/provider/externalgrpc Issues or PRs related to the External gRPC provider area/provider/gce area/provider/hetzner Issues or PRs related to Hetzner provider area/provider/huaweicloud area/provider/ionoscloud area/provider/kwok Issues or PRs related to the kwok cloud provider for Cluster Autoscaler area/provider/linode Issues or PRs related to linode provider area/provider/magnum Issues or PRs related to the Magnum cloud provider for Cluster Autoscaler area/provider/oci Issues or PRs related to oci provider area/provider/rancher area/provider/utho Issues or PRs related to Utho provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CA ClusterAPI provider can delete wrong node when scale-down occurs during MachineDeployment upgrade
5 participants
@elmiko @k8s-triage-robot @jackfrancis @k8s-ci-robot @sbueringer