search

kubernetes-sigs / kube-scheduler-wasm-extension

All the things to make the scheduler extendable with wasm.
Apache License 2.0
110 stars 22 forks source link

Switches from UID to pointer for identifying cycle IDs #39

Closed codefromthecrypt closed 1 year ago

codefromthecrypt commented 1 year ago

What type of PR is this?

/kind bug

What this PR does / why we need it:

Before, we used the UID to identify which cycle is in progress. However, when a scheduling cycle fails, the same UID can happen the next time.

This uses the pointer instead as this is different per run, and easier to compare in wasm. We use the lower bits as that's likely to not conflict, easier to use in wasm, and also can't identify fully the real host memory address.

Finally, this resets the pointer between benchmark runs. This makes sure a run isn't accidentally faster. Before, it was faster due to colliding on the same UID. By resetting the pointer, we avoid re-creating that issue.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

I noticed curious results in the prior PR. The current results look a lot more realistic

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e
BenchmarkPluginFilter
BenchmarkPluginFilter/noop-wat
BenchmarkPluginFilter/noop-wat/params:_small
BenchmarkPluginFilter/noop-wat/params:_small-12              4514036           271.1 ns/op
BenchmarkPluginFilter/noop-wat/params:_real
BenchmarkPluginFilter/noop-wat/params:_real-12               4434201           263.2 ns/op
BenchmarkPluginFilter/noop
BenchmarkPluginFilter/noop/params:_small
BenchmarkPluginFilter/noop/params:_small-12                  3605126           335.3 ns/op
BenchmarkPluginFilter/noop/params:_real
BenchmarkPluginFilter/noop/params:_real-12                   3170498           339.0 ns/op
BenchmarkPluginFilter/test
BenchmarkPluginFilter/test/params:_small
BenchmarkPluginFilter/test/params:_small-12                   154036          7219 ns/op
BenchmarkPluginFilter/test/params:_real
BenchmarkPluginFilter/test/params:_real-12                      9352        121479 ns/op
BenchmarkPluginScore
BenchmarkPluginScore/noop-wat
BenchmarkPluginScore/noop-wat/params:_small
BenchmarkPluginScore/noop-wat/params:_small-12               4664058           257.6 ns/op
BenchmarkPluginScore/noop-wat/params:_real
BenchmarkPluginScore/noop-wat/params:_real-12                4602486           261.2 ns/op
BenchmarkPluginScore/noop
BenchmarkPluginScore/noop/params:_small
BenchmarkPluginScore/noop/params:_small-12                   3169807           377.1 ns/op
BenchmarkPluginScore/noop/params:_real
BenchmarkPluginScore/noop/params:_real-12                    3494535           344.6 ns/op
BenchmarkPluginScore/test
BenchmarkPluginScore/test/params:_small
BenchmarkPluginScore/test/params:_small-12                    283779          4048 ns/op
BenchmarkPluginScore/test/params:_real
BenchmarkPluginScore/test/params:_real-12                      22026         54050 ns/op
BenchmarkPluginFilterAndScore
BenchmarkPluginFilterAndScore/noop-wat
BenchmarkPluginFilterAndScore/noop-wat/params:_small
BenchmarkPluginFilterAndScore/noop-wat/params:_small-12      3251036           367.1 ns/op
BenchmarkPluginFilterAndScore/noop-wat/params:_real
BenchmarkPluginFilterAndScore/noop-wat/params:_real-12       3233397           369.3 ns/op
BenchmarkPluginFilterAndScore/noop
BenchmarkPluginFilterAndScore/noop/params:_small
BenchmarkPluginFilterAndScore/noop/params:_small-12          2127667           565.0 ns/op
BenchmarkPluginFilterAndScore/noop/params:_real
BenchmarkPluginFilterAndScore/noop/params:_real-12           2246898           531.6 ns/op
BenchmarkPluginFilterAndScore/test
BenchmarkPluginFilterAndScore/test/params:_small
BenchmarkPluginFilterAndScore/test/params:_small-12           105128         10904 ns/op
BenchmarkPluginFilterAndScore/test/params:_real
BenchmarkPluginFilterAndScore/test/params:_real-12              5520        211922 ns/op
k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codefromthecrypt, sanposhiho

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/blob/main/OWNERS)~~ [sanposhiho] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
sanposhiho commented 1 year ago

Thanks for having this as well as some discussion. I'm glad that, even though we're entering the jungle of complex preemption area, we have some clear insights about problems to solve

codefromthecrypt commented 1 year ago

Thanks, @sanposhiho I learned a lot too, and any tricks to identify pre-emption sound really useful. I think we'll end up revisiting the design a couple times at least, especially when we get pre-emption tests in!