Skip to content

Conversation

jackfrancis
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes two funcs in the Cluster Autoscaler simulator code that are no longer called anywhere (except UT, which are also cleaned up).

Which issue(s) this PR fixes:

Fixes #

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area labels Aug 27, 2025
@k8s-ci-robot k8s-ci-robot requested review from elmiko and x13n August 27, 2025 16:15
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-area labels Aug 27, 2025
@jackfrancis
Copy link
Contributor Author

/assign @x13n

Do you know whether or not these exportable methods are used downstream? Is there any reason not to delete them?

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.

seems reasonable to me if they are unused.

@jackfrancis
Copy link
Contributor Author

gentle ping @x13n

@x13n
Copy link
Member

x13n commented Sep 18, 2025

Turns out we use FindNodesToRemove in GKE internal fork, but we should probably just move it there - it just wraps the SimulateNodeRemoval function which is public anyway. FindEmptyNodesToRemove is unused as far as I can tell. Btw, while checking this I also found unused ScaleDownFindNodesToRemove const in metrics.go that should probably also be dropped.

@x13n
Copy link
Member

x13n commented Sep 18, 2025

/lgtm
/hold

Putting on hold in case you want to delete ScaleDownFindNodesToRemove in the same PR.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 18, 2025
@jackfrancis jackfrancis force-pushed the cas-rm-dead-similator-code branch from 07c4b5a to 77164e3 Compare September 19, 2025 16:24
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@jackfrancis
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 19, 2025
@jackfrancis
Copy link
Contributor Author

@x13n thank you, removed ScaleDownFindNodesToRemove as well.

FYI I audited metrics/metrics.go and found that ScaleDownMiscOperations is similarly unused. If you can confirm we can clean that up as well I can make one more change, otherwise feel free to unhold.

thx!

@x13n
Copy link
Member

x13n commented Sep 24, 2025

Good catch, I think ScaleDownMiscOperations can be deleted as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler 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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants