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

perf: factor out replacement/termination waits from deprovisioning #542

Merged
merged 58 commits into from
Nov 14, 2023
Merged

perf: factor out replacement/termination waits from deprovisioning #542

merged 58 commits into from
Nov 14, 2023

Conversation

njtran
Copy link
Contributor

@njtran njtran commented Sep 27, 2023 •
edited
Loading

Fixes #N/A

Description
This adds in an orchestration queue that commands will be added to. This doesn't change any of the core logic but will let Karpenter compute commands while it waits for a command's replacements to be initialized, speeding up Karpenter.

How was this change tested?

  • make presubmit
  • applying to my local cluster.

Scale Tests

  • Testing with a normal non-KwoK cluster with the change, I was able to scale down and replace nodes at about 24 seconds/node.
  • Testing with KwoK, doing a 250 node, 1750 pod test.
    • With the change saw nodes disrupted at a rate of about 1.25 seconds/node
    • Without the change, I saw the nodes disrupted at a rate of about 5 seconds/node
      This is about 400% quicker on average.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@njtran njtran requested a review from a team as a code owner September 27, 2023 00:01
@njtran njtran requested a review from engedaam September 27, 2023 00:01
@njtran njtran marked this pull request as draft September 27, 2023 00:01
@njtran njtran marked this pull request as ready for review September 29, 2023 07:56
jonathan-innis
jonathan-innis reviewed Oct 2, 2023
View reviewed changes
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/drift.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/orchestration/queue.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/controller.go Outdated Show resolved Hide resolved
pkg/controllers/deprovisioning/expiration.go Outdated Show resolved Hide resolved
This was referenced Oct 10, 2023
Closed
Closed
@github-actions
Copy link

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 20, 2023
@njtran njtran added blocked Unable to make progress due to some dependency and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 20, 2023
@thjwhite
Copy link

Wanted to follow up on this - it looks like this PR has been marked stale. I was linked to this PR from another issue I posted #670 . Is this still an active line of work? Happy to help explain our use case (please see the linked ticket I shared).

@jonathan-innis jonathan-innis mentioned this pull request Oct 12, 2023
Closed
@njtran njtran removed the blocked Unable to make progress due to some dependency label Nov 8, 2023
ellistarn
ellistarn previously approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

LGTM, once @jonathan-innis's comments are addressed. Nice work!

jonathan-innis
jonathan-innis approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@jonathan-innis jonathan-innis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work 🎉 Big effort here. Super excited to see this merged!

@njtran njtran merged commit 0938a63 into kubernetes-sigs:main Nov 14, 2023
6 checks passed
@njtran njtran deleted the orchestrate branch December 26, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-innis jonathan-innis jonathan-innis approved these changes

@ellistarn ellistarn ellistarn left review comments

@engedaam engedaam Awaiting requested review from engedaam engedaam was automatically assigned from aws/karpenter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@njtran @thjwhite @coveralls @ellistarn @jonathan-innis