kubernetes-sigs / kube-scheduler-wasm-extension

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

feature(guest): implement framework interface which is quite similar to the scheduling framework #19

Closed sanposhiho closed 11 months ago

sanposhiho commented 11 months ago

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR defines the interface which is very similar to the scheduling framework and allows users to implement the guest plugin in a similar way to what they do in native Go scheduler plugins.

Which issue(s) this PR fixes:

Fixes #15

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Improve the guest interface exposed to users so that users can implement the guest plugin in a similar way to what they do in native Go scheduler plugins.
sanposhiho commented 11 months ago

cc @codefromthecrypt

sanposhiho commented 11 months ago

/unhold Thanks! @kerthcet mistake 😓

k8s-ci-robot commented 11 months ago

[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](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/blob/main/OWNERS)~~ [sanposhiho] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
sanposhiho commented 11 months ago

it would be slower and allocate more

I wouldn't say allocating framework.Status isn't an expensive cost, it's reasonable to take it and make the interface the same as the scheduler's one.

while also adding confusion

If you mean nil / pointer staff which you commented, they're not confusing to people usually working around the scheduler plugins.

codefromthecrypt commented 11 months ago

it's reasonable to take it and make the interface the same as the scheduler's one.

I think eventually you will decide that the interface won't be the same as the host side, but now you don't see this. I may be wrong on this, so just defer my comments until things like what I said about context make sense to you. It is better that we proceed than get hung up on things that can be learned later.

codefromthecrypt commented 11 months ago

p.s. feel free to merge this ahead of #22 as I can rebase it

sanposhiho commented 11 months ago

It's quite related to the user experience. Regarding not only this Filter function, but all functions in all extension points, I'd like to keep the original scheduling framework's interface as much as possible. (As I do pod PodFn, nodeInfo NodeInfoFn, I don't disagree with the interface change that must need)

I know it makes sense that it's faster to return raw values instead of returning them in one fancy struct (framework.Status). But, on the other hand, we've already had many future potential wasm-extension users in the world, those creating the native Golang scheduler plugins. And, making many unnecessary differences between our interface and the scheduling framework will directly be the migration costs for users.

just defer my comments until things like what I said about context make sense to you.

Yes, then I'd be happy to have a corresponding change there. Currently I don't see any strong-enough reasons.

sanposhiho commented 11 months ago

/close

https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/19#discussion_r1210982138

k8s-ci-robot commented 11 months ago

@sanposhiho: Closed this PR.

In response to [this](https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/19#issuecomment-1569349169): >/close > >https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/19#discussion_r1210982138 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.