search

kubernetes-sigs / kube-scheduler-wasm-extension

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

Support Permit Plugin #93

Open kazukousen opened 3 months ago

kazukousen commented 3 months ago

What type of PR is this?

/kind feature /kind api-change

What this PR does / why we need it:

Support Permit Plugin

Which issue(s) this PR fixes:

Part of #72

Special notes for your reviewer:

permit must return timeout in addition to the statusCode. The Wasm (specification 1) allows only one return value to be defined, and the maximum size is limited to i64. But the statusCode has already consumed i32. Packing the remaining i32, as in the implementation of score, is difficult because timeout cannot be reduced to a size smaller than i64.

Therefore, the one of remaining solutions is to add a host callback for the timeout, Guest passes a timeout to an imported Host function. This will only add hostcall overhead.

So the suggestion would add set_timeout to the ABI.

Does this PR introduce a user-facing change?

Support permit plugin

What are the benchmark results of this change?

linux-foundation-easycla[bot] commented 3 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

k8s-ci-robot commented 3 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kazukousen Once this PR has been reviewed and has the lgtm label, please assign sanposhiho 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](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/blob/main/OWNERS)** 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 commented 3 months ago

Welcome @kazukousen!

It looks like this is your first PR to kubernetes-sigs/kube-scheduler-wasm-extension 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kube-scheduler-wasm-extension has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

Gekko0114 commented 3 months ago

/ok-to-test

k8s-ci-robot commented 3 months ago

@Gekko0114: No presubmit jobs available for kubernetes-sigs/kube-scheduler-wasm-extension@main

In response to [this](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/93#issuecomment-1880386554): >/test Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Gekko0114 commented 3 months ago

/assign

Gekko0114 commented 3 months ago

/test

k8s-ci-robot commented 3 months ago

@Gekko0114: No presubmit jobs available for kubernetes-sigs/kube-scheduler-wasm-extension@main

In response to [this](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/93#issuecomment-1880387091): >/test Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Gekko0114 commented 3 months ago

Overall it looks good! Thank you. I will add approve label after passing CI checks.

@sanposhiho, Seems that without your approval, this PR can't run CI checks. Could you take a look?

kazukousen commented 3 months ago

Thank you for your kind review. Fixed it 1aa0020. PTAL when you have time.

Gekko0114 commented 3 months ago

Please fix ci failures.

kazukousen commented 3 months ago

@Gekko0114 Sorry, Forced push and then fixed examples/advanced/main.wasm only at 976939b . The change should not affect examples/nodenumber/main.wasm .

I guess that main.wasm is not synchronized with main.go in examples/nodenumber because the test expects FilterPlugin and ScorePlugin interfaces, but the main.go implementation lacks the FilterPlugin interface. https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/actions/runs/7450036829/job/20343211771 I'm not familiar with this codebase, So please point out any mistakes! Thanks,

kazukousen commented 3 months ago

Through conversations with @Gekko0114 in Slack, I've got a handle on what's going on. It seems we should have updated our wasm build and the mask test when ReservePlugin and BindPlugin were added. This can be confirmed by comparing exportFunctions before and after executing make testdata on the main branch.

Addressed it in be83292 .

I suppose the permanent solution is to ensure that the CI fails when there's a discrepancy.

Gekko0114 commented 3 months ago

/label tide/merge-method-squash

kazukousen commented 3 months ago

@Gekko0114 thanks, Fixed it c0fefd0. PTAL

k8s-ci-robot commented 3 months ago

PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kazukousen commented 2 months ago

Sorry for the delay. I will work on it this weekend.