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 Reserve plugin #82

Closed utam0k closed 4 months ago

utam0k commented 5 months ago

What type of PR is this?

/kind feature /kind api-change

What this PR does / why we need it:

Support Reserve plugin

Which issue(s) this PR fixes:

Part of https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/issues/72

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support postBind plugin

What are the benchmark results of this change?

utam0k commented 5 months ago

I'll fix it tomorrow.

utam0k commented 5 months ago

@sanposhiho @codefromthecrypt This PR is ready for your review 🙏

sanposhiho commented 5 months ago

/assign /assign @Gekko0114

Gekko0114 commented 5 months ago

@utam0k Overall looks good! Left minor comments.

utam0k commented 4 months ago

hmm I'll check failure in CI

Gekko0114 commented 4 months ago

I guess because within Test_guestPool_bindingCycles, Filter is not called. But permit function expects to have called Filter in cyclestate's wasm. I also stacked this part before :)

utam0k commented 4 months ago

I guess because within Test_guestPool_bindingCycles, Filter is not called. But permit function expects to have called Filter in cyclestate's wasm. I also stacked this part before :)

@Gekko0114 Thanks for your comment. But this is an error message. It seems we need to call PreFilter...? 🤔 We have already called it before calling PreBind, right? Do you know the cause of this error?

$ go test -run 'Test_guestPool_bindingCycles$' ./plugin -count=1
--- FAIL: Test_guestPool_bindingCycles (0.13s)
    plugin_test.go:129: prebind failed: wasm: prebind error: panic: didn't cache pod from score

        wasm error: unreachable
        wasm stack trace:
                .runtime._panic(i32,i32)
                .prebind() i32
FAIL
FAIL    sigs.k8s.io/kube-scheduler-wasm-extension/scheduler/plugin      0.153s
FAIL
utam0k commented 4 months ago

It seems podSpec in wasm became nil after calling Permit 🤔 Why... ?

Gekko0114 commented 4 months ago

I guess because within Test_guestPool_bindingCycles, Filter is not called. But permit function expects to have called Filter in cyclestate's wasm. I also stacked this part before :)

@Gekko0114

Thanks for your comment. But this is an error message. It seems we need to call PreFilter...? 🤔 We have already called it before calling PreBind, right? Do you know the cause of this error?


$ go test -run 'Test_guestPool_bindingCycles$' ./plugin -count=1

--- FAIL: Test_guestPool_bindingCycles (0.13s)

    plugin_test.go:129: prebind failed: wasm: prebind error: panic: didn't cache pod from score

        wasm error: unreachable

        wasm stack trace:

                .runtime._panic(i32,i32)

                .prebind() i32

FAIL

FAIL    sigs.k8s.io/kube-scheduler-wasm-extension/scheduler/plugin      0.153s

FAIL

I see. I thought it because of 'make test's error log below. Did you get error even after fixing this issue?

E1213 13:22:36.656076 16802 guest.go:236] wasm: unreserve error: panic: didn't propagate state from pre-filter

Gekko0114 commented 4 months ago

Or maybe this function causes this issue...?? I will take a look when I have time, sorry for not having a clear answer

pl.pool.freeFromBinding(pod.UID)

Gekko0114 commented 4 months ago

I guess this failure might be caused by my previous PR (these parts might not be necessary). https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/blob/main/guest/testdata/cyclestate/main.go#L180-L182

I am asking to @sanposhiho about this point. BTW if you are in the slack of kubernetes workplace, could you ping me? Sometimes discussing on slack might be useful.

Gekko0114 commented 4 months ago

sanposhiho fixed the issue. So this failure will be fixed. https://github.com/kubernetes-sigs/kube-scheduler-wasm-extension/pull/86

utam0k commented 4 months ago

@Gekko0114 Don't worry, thanks for your look 🙏

Or maybe this function causes this issue...?? I will take a look when I have time, sorry for not having a clear answer

Gekko0114 commented 4 months ago

@utam0k Thanks! Left minor comments.

Gekko0114 commented 4 months ago

@utam0k Overall looks good! Sorry, I left one comment about typo.

utam0k commented 4 months ago

@Gekko0114 Did you forget to submit your review? I didn't find your comment 😭

Sorry, I left one comment about typo.

Gekko0114 commented 4 months ago

@utam0k So sorry, seems I didn't submit my comments. Please let me know if you can't see it even now.

k8s-ci-robot commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanposhiho, utam0k

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
Gekko0114 commented 4 months ago

/lgtm

Thank you so much!