[WIP] feat: port the NodeUnschedulable in-tree plugin to wasm #123

dejanzele · 2024-11-05T22:20:10Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Ports the in-tree NodeUnschedulable plugin

Which issue(s) this PR fixes:

Relates #101

Special notes for your reviewer:

This is a work in progress, I have made it work locally, but this is not finished yet. I have created this PR to get early feedback as I am not sure some things need to be handled the way I handled them.

I need to add more tests, will do that in the next couple of days.

Does this PR introduce a user-facing change?

NONE

What are the benchmark results of this change?

k8s-ci-robot · 2024-11-05T22:20:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dejanzele
Once this PR has been reviewed and has the lgtm label, please assign codefromthecrypt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

dejanzele · 2024-11-05T22:21:29Z

Makefile

 %/main.wasm: %/main.go
 	@(cd $(@D); tinygo build -o main.wasm -scheduler=none --no-debug -target=wasi .)

 .PHONY: build-tinygo
-build-tinygo: examples/nodenumber/main.wasm examples/advanced/main.wasm examples/imagelocality/main.wasm guest/testdata/cyclestate/main.wasm guest/testdata/filter/main.wasm guest/testdata/score/main.wasm \
+build-tinygo: examples/nodenumber/main.wasm examples/advanced/main.wasm examples/imagelocality/main.wasm examples/nodeunschedulable/main.wasm guest/testdata/cyclestate/main.wasm guest/testdata/filter/main.wasm guest/testdata/score/main.wasm \


I do not understand why the internal/e2e/scheduler_perf/wasm/nodeunschedulable can be compiled successfully with the %/main.wasm but the examples/nodeunschedulable one which is almost identical fails and needs the -gc=custom -tags=custommalloc flags.

dejanzele · 2024-11-05T22:24:52Z

guest/api/framework.go

@@ -1,5 +1,16 @@
 package api

+type QueueingHint int


This refers to https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/types.go#L256-L263. Where should this be placed?

Why do we need this in the first place? We don't support QueueingHint in the extension yet.

dejanzele · 2024-11-05T22:27:16Z

guest/api/helper/helpers.go

+)
+
+// TolerationsTolerateTaint checks if taint is tolerated by any of the tolerations.
+func TolerationsTolerateTaint(tolerations []*protoapi.Toleration, taint *protoapi.Taint) bool {


This refers to this function https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-helpers/scheduling/corev1/helpers.go#L63-L70.
Is there a better place?

You should put it (along with ToleratesTaint(...)) inside examples/nodeunschedulable.
I don't think we should maintain this function as the part of the guest SDK.

dejanzele · 2024-11-05T22:27:40Z

guest/api/tolerations.go

+//  3. Empty toleration.key means to match all taint keys.
+//     If toleration.key is empty, toleration.operator must be 'Exists';
+//     this combination means to match all taint values and all taint keys.
+func ToleratesTaint(toleration *v1.Toleration, taint *v1.Taint) bool {


This refers to this function https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/toleration.go#L38-L57.
As the kubernetes folder is sparsely checked out and I guess should not be modified, is there a better approach how to use this function?

Yes, kubernetes folder is only for generating the object files. Also, we cannot import the things from kubernetes because of the limitation coming from TinyGo.
So, yes, it's the only option that we almost do a copy like you do here, at least until we move to Golang SDK.

dejanzele · 2024-11-05T22:30:19Z

internal/e2e/scheduler_perf/config/sched-config-all-enabled.yaml

@@ -5,11 +5,11 @@ profiles:
      multiPoint:
        enabled:
          - name: wasm
-          - name: NodeNumber
+          - name: NodeUnschedulable


This was edited so I can test does it work.
I'll fix it with a better config.

dejanzele · 2024-11-05T22:31:08Z

/cc @sanposhiho @Gekko0114 @utam0k

dejanzele · 2024-11-05T22:33:49Z

Here is an AI generated summary of the benchmark report (should be taken with a bit of reservation):

Report on Scheduler Performance: Go In-tree vs. WASM Plugin

Scheduling Throughput (pods/s):

Default Go In-tree: Generally has a higher throughput (e.g., ~183.8 pods/s).
WASM Plugin: Lower scheduling throughput compared to the Go in-tree version (e.g., ~55.6 pods/s for some configurations).
Analysis: The Go in-tree scheduler demonstrates better throughput, indicating it can schedule more pods per second. This is a key metric for scenarios requiring high scalability and rapid scheduling.

Scheduling Attempt Duration (ms):

Go In-tree: Shows shorter average and percentile durations (e.g., ~20.97 ms for Average, 20.8 ms for Perc50).
WASM Plugin: Exhibits longer attempt durations (e.g., ~25.9 ms for Average and higher 99th percentile values like 563.2 ms).
Analysis: The Go in-tree scheduler performs better in terms of scheduling attempt duration, suggesting it is quicker at attempting to schedule individual pods compared to the WASM-based plugin.

Pod Scheduling Duration (ms):

Go In-tree: More consistent with shorter average pod scheduling times (e.g., ~2430.5 ms for Average).
WASM Plugin: Longer average scheduling times with higher percentile spikes (e.g., ~8030.8 ms Average and significant jumps at Perc90 and Perc99 levels).
Analysis: The Go in-tree scheduler provides more stable and quicker scheduling times, indicating a more predictable performance under load. The WASM plugin shows greater variability and higher latency, particularly under stress.

Extension Point Durations (ms):

PreFilter, Filter, Score, Bind, etc.:
- Go In-tree: Generally faster across extension points (e.g., averages in the sub-millisecond range).
- WASM Plugin: Shows more variability and higher values in specific extension points (e.g., Filter averaging at ~15.98 ms compared to Go's sub-millisecond figures).
Analysis: The Go scheduler demonstrates better performance across the different extension points of the scheduling process, which contributes to overall scheduling efficiency.

High Percentile Analysis (Perc99):

Go In-tree: Tends to have lower 99th percentile values, indicating more consistent performance even under heavy load.
WASM Plugin: Often displays higher 99th percentile times, suggesting potential bottlenecks or slower edge-case performance.
Implication: High 99th percentile values in the WASM plugin imply that, in scenarios with peak demand, performance can degrade significantly, leading to higher scheduling latency.

sanposhiho · 2024-11-06T16:35:09Z

Thanks for the PR @dejanzele.
FYI though, I'm away for Kubecon and most likely cannot visit here until the week after next. Pre-apologize for that!

dejanzele · 2024-11-06T20:39:40Z

I am also heading to KubeCon, we can discuss it there :)

sanposhiho · 2024-11-11T15:14:51Z

@Gekko0114 @utam0k can you take a look as well?

k8s-ci-robot · 2024-11-11T15:15:00Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sanposhiho · 2024-11-12T22:53:50Z

guest/api/framework.go

@@ -1,5 +1,16 @@
 package api

+type QueueingHint int


Why do we need this in the first place? We don't support QueueingHint in the extension yet.

sanposhiho · 2024-11-12T22:54:57Z

guest/api/helper/helpers.go

+)
+
+// TolerationsTolerateTaint checks if taint is tolerated by any of the tolerations.
+func TolerationsTolerateTaint(tolerations []*protoapi.Toleration, taint *protoapi.Taint) bool {


You should put it (along with ToleratesTaint(...)) inside examples/nodeunschedulable.
I don't think we should maintain this function as the part of the guest SDK.

sanposhiho · 2024-11-12T22:57:33Z

guest/api/tolerations.go

+//  3. Empty toleration.key means to match all taint keys.
+//     If toleration.key is empty, toleration.operator must be 'Exists';
+//     this combination means to match all taint values and all taint keys.
+func ToleratesTaint(toleration *v1.Toleration, taint *v1.Taint) bool {


Yes, kubernetes folder is only for generating the object files. Also, we cannot import the things from kubernetes because of the limitation coming from TinyGo.
So, yes, it's the only option that we almost do a copy like you do here, at least until we move to Golang SDK.

sanposhiho · 2024-11-12T22:59:00Z

internal/e2e/scheduler_perf/wasm/nodeunschedulable/main.go

Why do we need this? I mean, can we just use the wasm built from examples/nodeunschedulable?

sanposhiho · 2024-11-12T23:00:16Z

internal/e2e/scheduler_perf/config/sched-config-all-enabled.yaml

Please revert all the changes in scheduler-perf test cases.
If you want to include the nodeunschedulable plugin in the scheduler-perf, it's better to create a new test case, not replacing the existing test cases to use nodeunschedulable instead of nodenumber.
nodenumber is the simplest plugin which is good to measure the overhead of wasm extension.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 5, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2024

k8s-ci-robot requested a review from codefromthecrypt November 5, 2024 22:20

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2024

dejanzele commented Nov 5, 2024

View reviewed changes

port the NodeUnschedulable in-tree plugin to wasm

ac49ac2

dejanzele force-pushed the feat/port-nodeunschedulable-plugin branch from 094402b to ac49ac2 Compare November 5, 2024 22:23

dejanzele commented Nov 5, 2024

View reviewed changes

k8s-ci-robot requested review from Gekko0114, sanposhiho and utam0k November 5, 2024 22:31

sanposhiho changed the title ~~[WIP} feat: port the NodeUnschedulable in-tree plugin to wasm~~ [WIP] feat: port the NodeUnschedulable in-tree plugin to wasm Nov 11, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2024

sanposhiho reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: port the NodeUnschedulable in-tree plugin to wasm #123

[WIP] feat: port the NodeUnschedulable in-tree plugin to wasm #123

dejanzele commented Nov 5, 2024

k8s-ci-robot commented Nov 5, 2024

dejanzele Nov 5, 2024

dejanzele Nov 5, 2024

sanposhiho Nov 12, 2024

dejanzele Nov 5, 2024 •

edited

Loading

sanposhiho Nov 12, 2024

dejanzele Nov 5, 2024

sanposhiho Nov 12, 2024

dejanzele Nov 5, 2024

dejanzele commented Nov 5, 2024

dejanzele commented Nov 5, 2024

sanposhiho commented Nov 6, 2024

dejanzele commented Nov 6, 2024

sanposhiho commented Nov 11, 2024

k8s-ci-robot commented Nov 11, 2024

sanposhiho Nov 12, 2024

sanposhiho Nov 12, 2024

sanposhiho Nov 12, 2024

sanposhiho Nov 12, 2024

sanposhiho Nov 12, 2024

[WIP] feat: port the NodeUnschedulable in-tree plugin to wasm #123

Are you sure you want to change the base?

[WIP] feat: port the NodeUnschedulable in-tree plugin to wasm #123

Conversation

dejanzele commented Nov 5, 2024

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 Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejanzele Nov 5, 2024 • edited Loading

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

dejanzele commented Nov 5, 2024

dejanzele commented Nov 5, 2024

Report on Scheduler Performance: Go In-tree vs. WASM Plugin

Scheduling Throughput (pods/s):

Scheduling Attempt Duration (ms):

Pod Scheduling Duration (ms):

Extension Point Durations (ms):

High Percentile Analysis (Perc99):

sanposhiho commented Nov 6, 2024

dejanzele commented Nov 6, 2024

sanposhiho commented Nov 11, 2024

k8s-ci-robot commented Nov 11, 2024

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

dejanzele Nov 5, 2024 •

edited

Loading