Implement EnqueueExtensions #55

codefromthecrypt · 2023-07-13T08:50:08Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This implements framework.EnqueueExtensions which is needed for common sample plugins, such as nodenumber

This also reduces namespace conflicts by moving the guest code that is related to protos into its own package. Notably there's a conflict on the framework symbol Node and the proto type.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

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
                                                        │ before.txt  │              now.txt               │
                                                        │   sec/op    │   sec/op     vs base               │
PluginPreFilter/noop-wat/params:_small-12                 131.6n ± 0%   132.0n ± 1%   +0.30% (p=0.011 n=6)
PluginPreFilter/noop-wat/params:_real-12                  132.0n ± 0%   133.0n ± 1%   +0.76% (p=0.006 n=6)
PluginPreFilter/noop/params:_small-12                     277.1n ± 0%   289.1n ± 4%   +4.33% (p=0.002 n=6)
PluginPreFilter/noop/params:_real-12                      279.5n ± 4%   280.9n ± 2%        ~ (p=0.485 n=6)
PluginPreFilter/test/params:_small-12                     4.329µ ± 1%   4.342µ ± 1%        ~ (p=0.145 n=6)
PluginPreFilter/test/params:_real-12                      48.75µ ± 2%   47.95µ ± 1%   -1.64% (p=0.015 n=6)
PluginFilter/noop-wat/params:_small-12                    267.5n ± 1%   351.8n ± 1%  +31.50% (p=0.002 n=6)
PluginFilter/noop-wat/params:_real-12                     266.6n ± 0%   351.2n ± 1%  +31.76% (p=0.002 n=6)
PluginFilter/noop/params:_small-12                        421.3n ± 0%   544.9n ± 1%  +29.35% (p=0.002 n=6)
PluginFilter/noop/params:_real-12                         421.9n ± 1%   546.5n ± 1%  +29.52% (p=0.002 n=6)
PluginFilter/test/params:_small-12                        7.833µ ± 0%   7.952µ ± 0%   +1.52% (p=0.002 n=6)
PluginFilter/test/params:_real-12                         111.8µ ± 1%   111.4µ ± 1%        ~ (p=0.394 n=6)
PluginScore/noop-wat/params:_small-12                     268.6n ± 2%   351.5n ± 0%  +30.85% (p=0.002 n=6)
PluginScore/noop-wat/params:_real-12                      269.3n ± 1%   352.1n ± 1%  +30.76% (p=0.002 n=6)
PluginScore/noop/params:_small-12                         517.4n ± 1%   637.7n ± 0%  +23.24% (p=0.002 n=6)
PluginScore/noop/params:_real-12                          463.3n ± 0%   581.5n ± 0%  +25.50% (p=0.002 n=6)
PluginScore/test/params:_small-12                         4.695µ ± 0%   4.802µ ± 0%   +2.28% (p=0.002 n=6)
PluginScore/test/params:_real-12                          49.18µ ± 2%   48.73µ ± 1%        ~ (p=0.065 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_small-12   403.2n ± 1%   488.6n ± 0%  +21.18% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_real-12    402.1n ± 1%   490.0n ± 1%  +21.88% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop/params:_small-12       655.9n ± 0%   785.6n ± 1%  +19.78% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop/params:_real-12        659.8n ± 1%   792.4n ± 0%  +20.09% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_small-12       8.612µ ± 1%   9.027µ ± 1%   +4.81% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_real-12        111.8µ ± 2%   112.8µ ± 1%        ~ (p=0.132 n=6)
PluginEnqueue/noop-wat-12                                               77.31n ± 1%
PluginEnqueue/noop-12                                                   112.7n ± 2%
PluginEnqueue/test-12                                                   130.7n ± 2%
geomean                                                   1.325µ        1.113µ       +12.93%

k8s-ci-robot · 2023-07-13T08:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codefromthecrypt

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~~ [codefromthecrypt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codefromthecrypt

notes

codefromthecrypt · 2023-07-13T08:51:52Z

guest/api/clusterevent.go

+
+// Constants for GVKs.
+const (
+	Pod GVK = iota


after I moved the other types to another package, I was able to mostly copy/paste this. I didn't make a RATIONALE entry about coercion from string to uint32, but on approval I will. The main thing is that this makes encoding far simpler. We should add documentation for framework.GVK that custom values are not supported: it is only controlled by kubernetes scheduler framework.

guest/api/clusterevent.go

codefromthecrypt · 2023-07-13T08:53:22Z

guest/enqueue/clusterevent.go

+import "sigs.k8s.io/kube-scheduler-wasm-extension/guest/api"
+
+// minEncodedClusterEvent is the size in bytes to encode api.ClusterEvent with
+// 32-bit little endian gvk, ActionType, NUL-terminated Label,


note: later we need to make an ABI doc with details like this consolidated, but as everything is in-flux not doing this right now and instead relying on tests.

codefromthecrypt · 2023-07-13T08:54:32Z

guest/enqueue/enqueue.go

+// enqueue is only exported to the host.
+//
+//export enqueue
+func _enqueue() {


currently enqueue returns nothing, not a framework status. We can change it if this does. I chose the name enqueue as it is a lot simpler than EventsToRegister. However, if there is another function for enqueue, let me know and I can change it...

codefromthecrypt · 2023-07-13T08:55:34Z

guest/testdata/enqueue/main.go

+
+func main() {
+	if len(os.Args) == 2 {
+		switch os.Args[1] {


I made this switch on args as I really couldn't figure out how to export a mutable global in tinygo target=wasi. cc @deadprogram in case you know? This is fine anyway..

scheduler/plugin/clusterevent.go

guest/enqueue/clusterevent.go

guest/api/proto/proto.go

guest/api/clusterevent.go

codefromthecrypt · 2023-07-13T08:58:39Z

scheduler/plugin/plugin.go

+}
+
+// EventsToRegister implements the same method as documented on framework.EnqueueExtensions.
+func (pl *wasmPlugin) EventsToRegister() (clusterEvents []framework.ClusterEvent) {


to make things stop explosion, we add a default impl for enqueue extensions with the same behavior as if it wasn't if the guest doesn't support them

scheduler/plugin/plugin.go

scheduler/plugin/pool.go

codefromthecrypt · 2023-07-14T08:55:59Z

ok I cleaned up the PR so that it is ready to merge if ok (backfilled test coverage and notes).

After this, we can add guest config (e.g. plugin to read yaml or otherwise) and PreScore, so we can implement a realistic, configurable plugin like nodenumber.

guest/filter/filter.go

codefromthecrypt · 2023-07-14T10:14:11Z

ok all good. bench should have become a little worse on noop, due to extra hook. Things look correct now.

sanposhiho

Thanks for the effort.
Looks great overall!

scheduler/plugin/guest.go

guest/api/clusterevent.go

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt · 2023-07-17T02:35:21Z

@sanposhiho PTAL I think I got everything (pre-squashed)

sanposhiho

Thanks. Only one nit, lgtm other things.

/lgtm
/hold

Feel free to /unhold by yourself to get this merged after the fix, please.

sanposhiho · 2023-07-17T08:02:18Z

guest/api/clusterevent.go

+	ActionType ActionType
+}
+
+// ^-- Note: This does not include Label.


RATIONALE.md

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

k8s-ci-robot · 2023-07-17T11:37:11Z

New changes are detected. LGTM label has been removed.

codefromthecrypt · 2023-07-17T11:37:31Z

thanks for the catch @sanposhiho!

codefromthecrypt · 2023-07-17T11:48:54Z

/unhold

codefromthecrypt · 2023-07-17T11:50:16Z

tomorrow I'll work on pre-score and hopefully config. after that, we should be able to port a basic plugin.

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 13, 2023

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 13, 2023

k8s-ci-robot requested a review from sanposhiho July 13, 2023 08:50

codefromthecrypt commented Jul 13, 2023

View reviewed changes

scheduler/plugin/plugin.go Outdated Show resolved Hide resolved

codefromthecrypt commented Jul 14, 2023

View reviewed changes

scheduler/plugin/pool.go Show resolved Hide resolved

codefromthecrypt force-pushed the enqueue branch from d256c5a to 430dc59 Compare July 14, 2023 08:53

codefromthecrypt force-pushed the enqueue branch from 430dc59 to e3ec274 Compare July 14, 2023 09:20

codefromthecrypt commented Jul 14, 2023

View reviewed changes

guest/filter/filter.go Outdated Show resolved Hide resolved

codefromthecrypt force-pushed the enqueue branch from e3ec274 to df0f66e Compare July 14, 2023 09:41

sanposhiho reviewed Jul 15, 2023

View reviewed changes

scheduler/plugin/guest.go Outdated Show resolved Hide resolved

guest/api/clusterevent.go Outdated Show resolved Hide resolved

Implement EnqueueExtensions

bc43ae9

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt force-pushed the enqueue branch from df0f66e to bc43ae9 Compare July 17, 2023 02:11

codefromthecrypt requested a review from sanposhiho July 17, 2023 02:35

sanposhiho reviewed Jul 17, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023

k8s-ci-robot assigned sanposhiho Jul 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2023

Update RATIONALE.md

237642b

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023

codefromthecrypt merged commit 742a858 into kubernetes-sigs:main Jul 17, 2023
5 of 6 checks passed

codefromthecrypt deleted the enqueue branch July 17, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EnqueueExtensions #55

Implement EnqueueExtensions #55

codefromthecrypt commented Jul 13, 2023 •

edited

k8s-ci-robot commented Jul 13, 2023

codefromthecrypt left a comment

codefromthecrypt Jul 13, 2023

codefromthecrypt Jul 13, 2023

codefromthecrypt Jul 13, 2023

codefromthecrypt Jul 13, 2023

codefromthecrypt Jul 13, 2023

codefromthecrypt commented Jul 14, 2023

codefromthecrypt commented Jul 14, 2023

sanposhiho left a comment

codefromthecrypt commented Jul 17, 2023

sanposhiho left a comment

sanposhiho Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

Implement EnqueueExtensions #55

Implement EnqueueExtensions #55

Conversation

codefromthecrypt commented Jul 13, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

What are the benchmark results of this change?

k8s-ci-robot commented Jul 13, 2023

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt Jul 13, 2023

Choose a reason for hiding this comment

codefromthecrypt Jul 13, 2023

Choose a reason for hiding this comment

codefromthecrypt Jul 13, 2023

Choose a reason for hiding this comment

codefromthecrypt Jul 13, 2023

Choose a reason for hiding this comment

codefromthecrypt Jul 13, 2023

Choose a reason for hiding this comment

codefromthecrypt commented Jul 14, 2023

codefromthecrypt commented Jul 14, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Jul 17, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

sanposhiho Jul 17, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

codefromthecrypt commented Jul 17, 2023

codefromthecrypt commented Jul 13, 2023 •

edited