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

Refactor StartDeletion usage patterns and enforce periodic scaledown #6575

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Refactor StartDeletion usage patterns and enforce periodic scaledown #6575

merged 1 commit into from
Mar 18, 2024

Conversation

mtrqq
Copy link
Contributor

@mtrqq mtrqq commented Feb 27, 2024 •
edited
Loading

What type of PR is this?

/cleanup

What this PR does / why we need it:

This PR does 2 things:

  • Removes responsibility from Actuator.StartDeletion to create full ScaleDownStatus object. It allows us to remove some semantically incorrect usage patterns like this and this and instead query for status object explicitly via Actuator.ScaleDownStatus call
  • Enforces periodic checks of ScaleDownStatusProcessor, because as of right now - it can be called twice in case of cooldown period due to scaleDownStatusProcessorAlreadyCalled not being set in the case of scale down cooldown. Generally, the logic of explicit status processor calls around the loop itself was removed in favor of the periodic check in a defer statement

Which issue(s) this PR fixes:

Special notes for your reviewer:

  • Please give a special attention to legacy wrapper around Actuator because we are changing interface quite significantly
  • Some tests were not completely changed from using partial ScaleDownStatus objects as it brings as bit more readability (arguable)

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2024
@k8s-ci-robot k8s-ci-robot requested a review from x13n February 27, 2024 13:12
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2024
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from 67bd5e2 to c882739 Compare February 27, 2024 13:16
@mtrqq mtrqq marked this pull request as ready for review February 27, 2024 13:34
@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 27, 2024
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from c882739 to 5fd81ca Compare February 27, 2024 15:25
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2024
towca
towca requested changes Mar 5, 2024
View reviewed changes
cluster-autoscaler/core/static_autoscaler.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/scaledown/actuation/actuator.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/scaledown/actuation/actuator.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/static_autoscaler.go Outdated Show resolved Hide resolved
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from 5fd81ca to 693da97 Compare March 7, 2024 10:18
@mtrqq mtrqq requested a review from towca March 7, 2024 10:18
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from 693da97 to afcca75 Compare March 7, 2024 10:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2024
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch 2 times, most recently from 05bc948 to 06766d1 Compare March 7, 2024 10:33
towca
towca reviewed Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@towca towca left a comment

Choose a reason for hiding this comment

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

Just some last comments, overall LGTM. Have you tested this manually? Do you think it'd be viable to add a unit test that checks if the results are reported correctly (something that would break if we were to repeat the bug in the future)?

cluster-autoscaler/core/scaledown/actuation/actuator.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/scaledown/scaledown.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/static_autoscaler.go Outdated Show resolved Hide resolved
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch 2 times, most recently from ec681ef to 482d93c Compare March 12, 2024 12:10
@mtrqq mtrqq requested a review from towca March 12, 2024 12:11
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from 482d93c to 2d0125c Compare March 12, 2024 17:23
towca
towca requested changes Mar 13, 2024
View reviewed changes
cluster-autoscaler/core/static_autoscaler_test.go Show resolved Hide resolved
cluster-autoscaler/core/static_autoscaler_test.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/static_autoscaler_test.go Outdated Show resolved Hide resolved
cluster-autoscaler/core/static_autoscaler.go Outdated Show resolved Hide resolved
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch 3 times, most recently from b680239 to 535d5ac Compare March 14, 2024 09:32
@mtrqq mtrqq requested a review from towca March 14, 2024 10:11
@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from 535d5ac to f0216e7 Compare March 15, 2024 15:41
@towca
Copy link
Collaborator

towca commented Mar 15, 2024

/lgtm
/approve
/hold

Thanks for the change! Left one nit, feel free to unhold the PR if you prefer not to fix.

@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 Mar 15, 2024
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 15, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrqq, towca

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

@mtrqq mtrqq force-pushed the feat/periodic-scaledown-status-calls branch from f0216e7 to bed5058 Compare March 15, 2024 20:31
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2024
@mtrqq mtrqq requested a review from towca March 15, 2024 20:32
@towca
Copy link
Collaborator

towca commented Mar 18, 2024

/lgtm
/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 18, 2024
@k8s-ci-robot k8s-ci-robot merged commit d08db94 into kubernetes:master Mar 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer Awaiting requested review from feiskyer

@x13n x13n Awaiting requested review from x13n

@BigDarkClown BigDarkClown Awaiting requested review from BigDarkClown

@towca towca Awaiting requested review from towca

Assignees

@towca towca

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. 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.
3 participants
@mtrqq @towca @k8s-ci-robot