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.

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

chore: Create state.Node interface methods to access cluster state details #152

Merged

Conversation

jonathan-innis
Copy link
Member

@jonathan-innis jonathan-innis commented Jan 4, 2023

Fixes #

Description

  • Create state.Node interface methods to access cluster state details
  • Decoupled Provisioner from ExistingNode so that ExistingNodes can be built purely based on state.Node

How was this change tested?

make presubmit
FOCUS=Scheduling make e2etests

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

@jonathan-innis jonathan-innis force-pushed the merged-cluster-state branch 2 times, most recently from 1170f22 to 5251ce7 Compare January 4, 2023 22:10
@coveralls
Copy link

coveralls commented Jan 4, 2023

Pull Request Test Coverage Report for Build 3851545127

  • 425 of 462 (91.99%) changed or added relevant lines in 17 files are covered.
  • 29 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.2%) to 76.311%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/counter/controller.go 0 1 0.0%
pkg/controllers/provisioning/provisioner.go 10 11 90.91%
pkg/controllers/state/zz_generated.deepcopy.go 45 47 95.74%
pkg/controllers/controllers.go 0 3 0.0%
pkg/controllers/provisioning/scheduling/scheduler.go 24 27 88.89%
pkg/controllers/state/informer/pod.go 2 6 33.33%
pkg/controllers/state/informer/node.go 20 28 71.43%
pkg/controllers/state/cluster.go 162 177 91.53%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/deprovisioning/controller.go 3 70.33%
pkg/controllers/provisioning/batcher.go 3 70.0%
pkg/controllers/provisioning/provisioner.go 3 85.25%
pkg/controllers/state/cluster.go 4 87.87%
pkg/controllers/node/controller.go 7 74.19%
pkg/controllers/deprovisioning/events/events.go 9 80.0%
Totals Coverage Status
Change from base Build 3849449765: 0.2%
Covered Lines: 5779
Relevant Lines: 7573

💛 - Coveralls

@jonathan-innis jonathan-innis force-pushed the merged-cluster-state branch 3 times, most recently from 8c567a2 to ef4bca4 Compare January 4, 2023 22:46
@jonathan-innis jonathan-innis marked this pull request as ready for review January 4, 2023 22:51
@jonathan-innis jonathan-innis requested a review from a team as a code owner January 4, 2023 22:51
@jonathan-innis jonathan-innis force-pushed the merged-cluster-state branch 5 times, most recently from 911a232 to cfff5af Compare January 5, 2023 02:33
@jonathan-innis
Copy link
Member Author

Before PR

=== RUN   TestSchedulingProfile
scheduled 7610 against 21 nodes in total in 3.927209844s 1937.7625088271193 pods/sec
400 instances 10 pods    1 nodes  3.157028ms per scheduling    315.702µs per pod
400 instances 100 pods   1 nodes  32.386241ms per scheduling   323.862µs per pod
400 instances 500 pods   1 nodes  148.849796ms per scheduling  297.699µs per pod
400 instances 1000 pods  3 nodes  338.482729ms per scheduling  338.482µs per pod
400 instances 1500 pods  4 nodes  536.756792ms per scheduling  357.837µs per pod
400 instances 2000 pods  5 nodes  648.753041ms per scheduling  324.376µs per pod
400 instances 2500 pods  6 nodes  736.882146ms per scheduling  294.752µs per pod
--- PASS: TestSchedulingProfile (13.59s)
PASS

After PR

=== RUN   TestSchedulingProfile
scheduled 7610 against 20 nodes in total in 4.016303065s 1894.7773305050625 pods/sec
400 instances 10 pods    1 nodes  3.097744ms per scheduling    309.774µs per pod
400 instances 100 pods   1 nodes  32.313276ms per scheduling   323.132µs per pod
400 instances 500 pods   1 nodes  153.221583ms per scheduling  306.443µs per pod
400 instances 1000 pods  3 nodes  339.300361ms per scheduling  339.3µs per pod
400 instances 1500 pods  4 nodes  541.017812ms per scheduling  360.678µs per pod
400 instances 2000 pods  4 nodes  588.58475ms per scheduling   294.292µs per pod
400 instances 2500 pods  6 nodes  792.721666ms per scheduling  317.088µs per pod
--- PASS: TestSchedulingProfile (14.09s)
PASS

Effectively the same benchmarking between before/after PR

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.

nice changes -- let's hope those tests hold ;)

pkg/controllers/state/node.go Outdated Show resolved Hide resolved
pkg/controllers/state/informer/node.go Show resolved Hide resolved
@jonathan-innis jonathan-innis force-pushed the merged-cluster-state branch 2 times, most recently from 72716d8 to c0b9477 Compare January 6, 2023 00:05
Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

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

lgtm, performance impact is small and worth the flexibility IMO

@jonathan-innis jonathan-innis merged commit a4b0513 into kubernetes-sigs:main Jan 6, 2023
@jonathan-innis jonathan-innis deleted the merged-cluster-state branch January 6, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants