Add api.Plugin as a base type for guest plugins #54

codefromthecrypt · 2023-07-12T06:43:16Z

What type of PR is this?

/kind cleanup
/kind documentation

What this PR does / why we need it:

This adds api.Plugin as a base type for guest plugins. This is similar to framework.Plugin, but doesn't yet define Name() as there's no purpose for it. Notes on that are added to RATIONALE.md.

This mostly gets the guest SDK looking much more like the framework one. This also makes it easier to test for misconfiguration, such as registering multiple instances of plugins.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Added special notes in /RATIONALE.md as it is easy to forget why we can't use a real name.
Stopped GuestName as it really doesn't matter at all.

Does this PR introduce a user-facing change?

NONE because it isn't user-ready yet!

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                 132.1n ± 1%   133.0n ± 1%        ~ (p=0.905 n=6)
PluginPreFilter/noop-wat/params:_real-12                  132.6n ± 3%   132.4n ± 2%        ~ (p=0.556 n=6)
PluginPreFilter/noop/params:_small-12                     296.9n ± 0%   278.7n ± 1%   -6.15% (p=0.002 n=6)
PluginPreFilter/noop/params:_real-12                      297.2n ± 0%   278.3n ± 0%   -6.36% (p=0.002 n=6)
PluginPreFilter/test/params:_small-12                     4.401µ ± 1%   4.312µ ± 1%   -2.01% (p=0.002 n=6)
PluginPreFilter/test/params:_real-12                      49.79µ ± 0%   48.48µ ± 1%   -2.65% (p=0.002 n=6)
PluginFilter/noop-wat/params:_small-12                    266.1n ± 0%   271.7n ± 3%        ~ (p=0.165 n=6)
PluginFilter/noop-wat/params:_real-12                     266.9n ± 0%   270.9n ± 2%        ~ (p=0.225 n=6)
PluginFilter/noop/params:_small-12                        482.4n ± 2%   418.1n ± 2%  -13.33% (p=0.002 n=6)
PluginFilter/noop/params:_real-12                         482.6n ± 0%   427.0n ± 2%  -11.52% (p=0.002 n=6)
PluginFilter/test/params:_small-12                        7.895µ ± 1%   7.860µ ± 0%        ~ (p=0.225 n=6)
PluginFilter/test/params:_real-12                         115.5µ ± 1%   114.3µ ± 2%        ~ (p=0.071 n=6)
PluginScore/noop-wat/params:_small-12                     269.4n ± 0%   269.7n ± 1%        ~ (p=0.913 n=6)
PluginScore/noop-wat/params:_real-12                      270.0n ± 0%   269.8n ± 0%        ~ (p=1.000 n=6)
PluginScore/noop/params:_small-12                         538.8n ± 0%   519.4n ± 0%   -3.61% (p=0.002 n=6)
PluginScore/noop/params:_real-12                          493.4n ± 1%   465.0n ± 1%   -5.76% (p=0.002 n=6)
PluginScore/test/params:_small-12                         4.551µ ± 1%   4.720µ ± 1%   +3.71% (p=0.002 n=6)
PluginScore/test/params:_real-12                          48.48µ ± 1%   49.24µ ± 2%   +1.58% (p=0.004 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_small-12   401.0n ± 0%   412.8n ± 3%   +2.93% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop-wat/params:_real-12    401.4n ± 0%   402.4n ± 1%        ~ (p=0.288 n=6)
PluginPrefilterFilterAndScore/noop/params:_small-12       720.5n ± 2%   652.2n ± 1%   -9.49% (p=0.002 n=6)
PluginPrefilterFilterAndScore/noop/params:_real-12        746.7n ± 3%   674.3n ± 2%   -9.69% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_small-12       9.188µ ± 1%   8.510µ ± 1%   -7.38% (p=0.002 n=6)
PluginPrefilterFilterAndScore/test/params:_real-12        118.2µ ± 2%   110.2µ ± 2%   -6.76% (p=0.002 n=6)
geomean                                                   1.374µ        1.330µ        -3.18%

codefromthecrypt · 2023-07-12T06:45:10Z

guest/internal/plugin/plugin.go

+ if current == nil {
+ current = plugin
+ } else if current != plugin {
+ panic("only one plugin instance is supported")


this is important because even though we we handle state outside user code, they may define state and accidentally assign multiple instances, splitting it. Anyway it performs better per benchmarks.

codefromthecrypt · 2023-07-12T06:49:39Z

sorry I will fix the tinygo test.

codefromthecrypt · 2023-07-12T06:54:57Z

yeah can't test panics with tinygo+wasm yet. I'll change accordingly tinygo-org/tinygo#2914

This adds `api.Plugin` as a base type for guest plugins. This is similar to `framework.Plugin`, but doesn't yet define `Name()` as there's no purpose for it. Notes on that are added to RATIONALE.md. This mostly gets the guest SDK looking much more like the framework one. This also makes it easier to test for misconfiguration, such as registering multiple instances of plugins. Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt · 2023-07-12T07:13:43Z

tests pass re-running benchmark as I had to change the tinygo impl a bit

codefromthecrypt · 2023-07-12T07:21:13Z

bench is still a lot better. PTAL!

sanposhiho · 2023-07-12T09:49:44Z

guest/internal/plugin/plugin.go

+// MustSet sets the plugin once
+func MustSet(plugin api.Plugin) {
+ if !set(plugin) {
+ panic("only one plugin instance is supported")


sanposhiho · 2023-07-12T09:55:15Z

Whoops, I submited the review in a middle. Wait a min for the full review.

sanposhiho

/lgtm

k8s-ci-robot · 2023-07-12T09:59:57Z

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

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

k8s-ci-robot requested a review from sanposhiho July 12, 2023 06:43

codefromthecrypt commented Jul 12, 2023

View reviewed changes

codefromthecrypt force-pushed the plugin-api branch from e10da67 to d39d13c Compare July 12, 2023 07:08

sanposhiho approved these changes Jul 12, 2023

View reviewed changes

k8s-ci-robot assigned sanposhiho Jul 12, 2023

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

k8s-ci-robot merged commit 44c6a5c into kubernetes-sigs:main Jul 12, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add api.Plugin as a base type for guest plugins #54

Add api.Plugin as a base type for guest plugins #54

codefromthecrypt commented Jul 12, 2023 •

edited

Loading

codefromthecrypt Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

sanposhiho Jul 12, 2023

sanposhiho commented Jul 12, 2023 •

edited

Loading

sanposhiho left a comment

k8s-ci-robot commented Jul 12, 2023

Add api.Plugin as a base type for guest plugins #54

Add api.Plugin as a base type for guest plugins #54

Conversation

codefromthecrypt commented Jul 12, 2023 • edited Loading

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?

codefromthecrypt Jul 12, 2023

Choose a reason for hiding this comment

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023

sanposhiho Jul 12, 2023

Choose a reason for hiding this comment

sanposhiho commented Jul 12, 2023 • edited Loading

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 12, 2023

codefromthecrypt commented Jul 12, 2023 •

edited

Loading

sanposhiho commented Jul 12, 2023 •

edited

Loading