guest: add plugin.Set for simpler example and fixes URL parsing #69

codefromthecrypt · 2023-08-01T02:24:49Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This adds plugin.Set which makes it easier to write plugins. This also makes the example that uses it simpler by avoiding nottinygo. This is simpler because it reduces the amount of flags used, as well a flat package is easier to write. However, this can't be tested with tinygo anymore, and is slower.

Hence, this adds an "advanced" example with the former logic, which is more verbose and tricky, but faster.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

This also fixes some bugs in URL parsing due to some logic not copied over from dapr. Notably, the original logic handled relative paths and better error messages. This also simplifies the logic further as we were parsing into URL type, just to convert it back to a string! Also, we started using testify by accident where that's not used anywhere else.

cc @evacchi sorry I didn't look closely at your earlier PR or would have caught this!

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

You can see below that the simple plugin runs >5x slower than the advanced. However, because it is still less than millisecond, some users may be ok with this.

goos: darwin
goarch: arm64
pkg: sigs.k8s.io/kube-scheduler-wasm-extension/internal/e2e/scheduler
                                   │  before.txt  │       now.txt        │
                                   │    sec/op    │    sec/op      vs base   │
Example_NodeNumber/New-12            75.08m ±  2%
Example_NodeNumber/Run-12            47.86µ ± 10%
Example_NodeNumber/Simple/New-12                    62.23m ±   1%
Example_NodeNumber/Simple/Run-12                    188.6µ ± 109%
Example_NodeNumber/Advanced/New-12                  73.61m ±   2%
Example_NodeNumber/Advanced/Run-12                  47.03µ ±   2%
geomean                              1.895m         2.525m         ? ¹ ²
¹ benchmark set differs from baseline; geomeans may not be comparable
² ratios must be >0 to compute geomean

codefromthecrypt · 2023-08-01T02:29:08Z

examples/nodenumber/main_test.go

+	protoapi "sigs.k8s.io/kube-scheduler-wasm-extension/kubernetes/proto/api"
+)
+
+func Test_NodeNumber(t *testing.T) {


these tests still work in normal Go. In fact a lot of SDKs only test with Go and never TinyGo, probably because it is too complicated.

That said, I think the guest SDK itself and advanced example should stil work with tinygo test -target=wasi (and they do).

codefromthecrypt · 2023-08-01T02:31:12Z

guest/score/score.go

+	if score == nil { // Then, the user didn't define one.
+		// This is likely caused by use of plugin.Set(p), where 'p' didn't
+		// implement ScorePlugin: return success and score zero.
+		return 0


So, if you use the simple plugin.Set(plugin) we have to return something for score. Returning zero here (both status and score). There's no way to avoid this in simple mode because the host will call the function regardless. Only other way is complicated, but exposing a bitflag of functions that could be called.

I think that's too much, so if someone needs to assign score, really they should implement it. If they must not call score, they should not use plugin.Set(p) as the docs mention.

codefromthecrypt · 2023-08-01T02:32:53Z

scheduler/plugin/plugin.go

@@ -79,22 +76,6 @@ func NewFromConfig(ctx context.Context, config PluginConfig) (framework.Plugin,
 	}
 }

-func readFromURI(ctx context.Context, u string) ([]byte, error) {
-	uri, err := url.ParseRequestURI(u)


the problem was eagerly parsing here (not done in dapr where some logic was copied from). This breaks relative paths like file://../../foo.wasm. I noticed this running make profile

ah right. This was not caught by other tests because they were eagerly converting to an absolute file:///

codefromthecrypt · 2023-08-01T02:35:18Z

scheduler/plugin/wasm_test.go

+
+func Test_httpGet(t *testing.T) {
+	wasmBinary := wasmMagicNumber
+	wasmBinary = append(wasmBinary, 0x00, 0x00, 0x00, 0x00)


ps I have no idea why we are appending 4 bytes as this was copied from before.. possibly to give some more data to compress?

I think it was padded to make it a valid wasmBinary, but this could be misguided. I didn't think of verifying without the padding.

codefromthecrypt · 2023-08-02T06:44:30Z

added polish, mostly README

sanposhiho

Thanks, left small nits.

/approve

scheduler/plugin/wasm.go

sanposhiho · 2023-08-05T04:17:32Z

scheduler/plugin/wasm.go

+// such as "file://../../path/to/plugin.wasm"
+func getURL(ctx context.Context, url string) ([]byte, error) {
+	if url == "" {
+		return nil, errors.New("missing url")


This error is going to appear to users. So, can we make it a bit more fancy like "GuestURL is required in the plugin configuration"?

I moved the missing one higher up and backfilled a test, so that they are like the others (prefixed wasm: some error)

guest/plugin/plugin.go

This adds `plugin.Set` which makes it easier to write plugins. This also makes the example that uses it simpler by avoiding nottinygo. This is simpler because it reduces the amount of flags used, as well a flat package is easier to write. However, this can't be tested with tinygo anymore, and is slower. Hence, this adds an "advanced" example with the former logic, which is more verbose and tricky, but faster. This also fixes some bugs in URL parsing due to some logic not copied over from dapr. Notably, the original logic handled relative paths and better error messages. This also simplifies the logic further as we were parsing into URL type, just to convert it back to a string! Signed-off-by: Adrian Cole <[email protected]>

codefromthecrypt · 2023-08-05T12:42:38Z

squashed and ready, thanks for the look @sanposhiho!

sanposhiho

/approve
/lgtm

Thanks!

sanposhiho · 2023-08-05T12:55:38Z

scheduler/plugin/plugin.go

-	guestBin, err := readFromURI(ctx, config.GuestURL)
+	url := config.GuestURL
+	if url == "" {
+		return nil, errors.New("wasm: guestURL is required")


k8s-ci-robot · 2023-08-05T12:56:33Z

[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

codefromthecrypt mentioned this pull request Aug 1, 2023

Add klog.Info/Error support #67

Merged

codefromthecrypt commented Aug 1, 2023

View reviewed changes

codefromthecrypt force-pushed the simple branch from 7aa9455 to 0c5daae Compare August 1, 2023 03:04

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 1, 2023

codefromthecrypt mentioned this pull request Aug 1, 2023

config: add support to file:// and http(s):// URIs #68

Merged

codefromthecrypt force-pushed the simple branch 2 times, most recently from 1ff6e36 to e91d24d Compare August 2, 2023 06:41

sanposhiho approved these changes Aug 5, 2023

View reviewed changes

codefromthecrypt force-pushed the simple branch from e91d24d to 35f33e5 Compare August 5, 2023 12:41

sanposhiho approved these changes Aug 5, 2023

View reviewed changes

k8s-ci-robot assigned sanposhiho Aug 5, 2023

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

k8s-ci-robot merged commit 293aa59 into kubernetes-sigs:main Aug 5, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

guest: add plugin.Set for simpler example and fixes URL parsing #69

guest: add plugin.Set for simpler example and fixes URL parsing #69

codefromthecrypt commented Aug 1, 2023 •

edited

Loading

codefromthecrypt Aug 1, 2023

codefromthecrypt Aug 1, 2023

codefromthecrypt Aug 1, 2023

evacchi Aug 1, 2023

codefromthecrypt Aug 1, 2023

evacchi Aug 1, 2023

codefromthecrypt commented Aug 2, 2023

sanposhiho left a comment

sanposhiho Aug 5, 2023

codefromthecrypt Aug 5, 2023

codefromthecrypt commented Aug 5, 2023

sanposhiho left a comment

sanposhiho Aug 5, 2023

k8s-ci-robot commented Aug 5, 2023

guest: add plugin.Set for simpler example and fixes URL parsing #69

guest: add plugin.Set for simpler example and fixes URL parsing #69

Conversation

codefromthecrypt commented Aug 1, 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Aug 2, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Aug 5, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 5, 2023

codefromthecrypt commented Aug 1, 2023 •

edited

Loading