Swap admitted condition in the same API call as admission or eviction #654

mcariatm · 2023-03-20T13:20:41Z

What type of PR is this?

To reduce complexity in the Workload controller and reduce API usage.

What this PR does / why we need it:

Set the Admitted condition value in the same API call that sets .status.admission.

Which issue(s) this PR fixes:

Fixes #603

Special notes for your reviewer:

linux-foundation-easycla · 2023-03-20T13:20:47Z

The committers listed above are authorized under a signed CLA.

✅ login: mcariatm (b880723, e4adb5f, 7bc140d)

netlify · 2023-03-20T13:20:50Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`c0ccdb4`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/642c56060980440008516735

k8s-ci-robot · 2023-03-20T13:20:52Z

Hi @mcariatm. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

alculquicondor · 2023-03-20T13:53:10Z

/ok-to-test

mcariatm · 2023-03-20T13:56:57Z

/ok-to-test

I'm still updating integration tests.

pkg/controller/core/workload_controller.go

alculquicondor · 2023-03-20T18:16:29Z

pkg/scheduler/preemption/preemption.go

+				Message:            fmt.Sprintf("Preempted by ClusterQueue %s", target.Obj.Status.Admission.ClusterQueue),
+			}
+		}
+		err := p.applyPreemption(ctx, workload.AdmissionPatch(target.Obj))


Here is the beauty of SSA:
You don't need to find the old condition. You can just send a patch that includes the condition and the server would find the old one and replace it.

alculquicondor · 2023-03-20T18:18:43Z

I like what you are doing: keeping the admission field and changing the controller to just use the condition.

This is going to help towards issues like this one #510, where after eviction we need to know what the assigned flavors were.

Naturally, a lot of tests will need to change to check for the condition instead of empty admission field. I think you can use ExpectWorkloadsToBePending

kueue/test/util/util.go

Line 155 in 17fbec9

    
           func ExpectWorkloadsToBePending(ctx context.Context, k8sClient client.Client, wls ...*kueue.Workload) {

alculquicondor · 2023-03-21T19:55:27Z

/ok-to-test

mcariatm · 2023-03-22T12:49:19Z

/easycla

mcariatm · 2023-03-22T14:46:42Z

/retest

alculquicondor · 2023-03-23T15:41:17Z

/milestone v0.3

kerthcet · 2023-03-28T09:22:04Z

Hi, any progress? New release is blocking on this.

mcariatm · 2023-03-28T09:32:30Z

I'm still struggling with this error when I run the tests.
2023-03-28T12:30:00.004214789+03:00 ERROR controller/controller.go:329 Reconciler error {"controller": "workload", "controllerGroup": "kueue.x-k8s.io", "controllerKind": "Workload", "Workload": {"name":"prod1","namespace":"podsready-mgfbp"}, "namespace": "podsready-mgfbp", "name": "prod1", "reconcileID": "c487bb3d-e222-45e9-a9dc-25fb00735e6f", "error": "Workload.kueue.x-k8s.io \"prod1\" is invalid: status: Invalid value: \"null\": status in body must be of type object: \"null\""} sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler /home/mcaria/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem /home/mcaria/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:274 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2 /home/mcaria/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:235 2023-03-28T12:30:00.009476515+03:00 LEVEL(-2) core/workload_controller.go:142 Reconciling Workload {"controller": "workload", "controllerGroup": "kueue.x-k8s.io", "controllerKind": "Workload", "Workload": {"name":"prod1","namespace":"podsready-mgfbp"}, "namespace": "podsready-mgfbp", "name": "prod1", "reconcileID": "2f12b924-c203-4547-89ea-26f9ba61cb67", "workload": {"name":"prod1","namespace":"podsready-mgfbp"}}

mcariatm · 2023-04-03T07:53:18Z

/retest

mcariatm · 2023-04-03T08:09:25Z

/retest

mcariatm · 2023-04-04T12:19:51Z

/test pull-kueue-test-integration-main

trasc · 2023-04-04T12:32:49Z

/test pull-kueue-test-integration-main

alculquicondor · 2023-04-04T13:54:03Z

when rerunning the test suite, please indicate how many times it has passed already

mcariatm · 2023-04-04T14:03:18Z

Passed 2/3
/test pull-kueue-test-integration-main

pkg/cache/cache.go

pkg/scheduler/scheduler.go

mcariatm · 2023-04-04T14:32:09Z

Passed 3/10
/test pull-kueue-test-integration-main

mcariatm · 2023-04-04T14:57:04Z

Passed 4/10
/test pull-kueue-test-integration-main

mcariatm · 2023-04-04T15:08:30Z

Passed 5/10
/test pull-kueue-test-integration-main

alculquicondor · 2023-04-04T15:21:34Z

Passed 6/10
/test pull-kueue-test-integration-main

alculquicondor · 2023-04-04T15:35:31Z

Passed 7/10
/test pull-kueue-test-integration-main

mcariatm · 2023-04-04T15:50:59Z

Passed 8/10
/test pull-kueue-test-integration-main

mcariatm · 2023-04-04T16:07:48Z

Passed 9/10
/test pull-kueue-test-integration-main

mcariatm · 2023-04-04T16:17:52Z

Passed 10/10

alculquicondor

/lgtm
/approve
/label tide/merge-method-squash

pkg/cache/cache_test.go

k8s-ci-robot · 2023-04-04T16:56:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mcariatm

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~~ [alculquicondor]

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

alculquicondor · 2023-04-04T21:44:00Z

/retitle Swap admitted condition in the same API call as admission or eviction

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023

k8s-ci-robot requested review from alculquicondor and kerthcet March 20, 2023 13:20

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 20, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 20, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 20, 2023

alculquicondor reviewed Mar 20, 2023

View reviewed changes

mcariatm force-pushed the 603_admitted_condition_swap_complexity branch 2 times, most recently from e8df75d to e4adb5f Compare March 21, 2023 14:36

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 22, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 22, 2023

alculquicondor added this to the v0.3 milestone Mar 23, 2023

alculquicondor mentioned this pull request Mar 23, 2023

Release v0.3.0 #659

Closed

20 tasks

Notify Wl update changes

f4bc8e1

mcariatm force-pushed the 603_admitted_condition_swap_complexity branch from 1a3e16d to f4bc8e1 Compare April 3, 2023 07:53

Added admission checking on nominate

3622045

mcariatm force-pushed the 603_admitted_condition_swap_complexity branch from 0bb02b0 to 3622045 Compare April 4, 2023 10:58

alculquicondor reviewed Apr 4, 2023

View reviewed changes

pkg/cache/cache.go Show resolved Hide resolved

pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved

added unit test for workload IsAssumedOrAdmitted in cache

c0ccdb4

alculquicondor reviewed Apr 4, 2023

View reviewed changes

pkg/cache/cache_test.go Show resolved Hide resolved

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 4, 2023

k8s-ci-robot assigned alculquicondor Apr 4, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2023

k8s-ci-robot merged commit 5227f60 into kubernetes-sigs:main Apr 4, 2023

k8s-ci-robot changed the title ~~603 admitted condition swap complexity~~ Swap admitted condition in the same API call as admission or eviction Apr 4, 2023

trasc deleted the 603_admitted_condition_swap_complexity branch October 24, 2023 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap admitted condition in the same API call as admission or eviction #654

Swap admitted condition in the same API call as admission or eviction #654

mcariatm commented Mar 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 20, 2023 •

edited

Loading

netlify bot commented Mar 20, 2023 •

edited

Loading

k8s-ci-robot commented Mar 20, 2023

alculquicondor commented Mar 20, 2023

mcariatm commented Mar 20, 2023

alculquicondor Mar 20, 2023

alculquicondor commented Mar 20, 2023 •

edited

Loading

alculquicondor commented Mar 21, 2023

mcariatm commented Mar 22, 2023

mcariatm commented Mar 22, 2023

alculquicondor commented Mar 23, 2023

kerthcet commented Mar 28, 2023

mcariatm commented Mar 28, 2023

mcariatm commented Apr 3, 2023

mcariatm commented Apr 3, 2023

mcariatm commented Apr 4, 2023

trasc commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

alculquicondor left a comment

k8s-ci-robot commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

Swap admitted condition in the same API call as admission or eviction #654

Swap admitted condition in the same API call as admission or eviction #654

Conversation

mcariatm commented Mar 20, 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:

linux-foundation-easycla bot commented Mar 20, 2023 • edited Loading

netlify bot commented Mar 20, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Mar 20, 2023

alculquicondor commented Mar 20, 2023

mcariatm commented Mar 20, 2023

alculquicondor Mar 20, 2023

Choose a reason for hiding this comment

alculquicondor commented Mar 20, 2023 • edited Loading

alculquicondor commented Mar 21, 2023

mcariatm commented Mar 22, 2023

mcariatm commented Mar 22, 2023

alculquicondor commented Mar 23, 2023

kerthcet commented Mar 28, 2023

mcariatm commented Mar 28, 2023

mcariatm commented Apr 3, 2023

mcariatm commented Apr 3, 2023

mcariatm commented Apr 4, 2023

trasc commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

mcariatm commented Apr 4, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

mcariatm commented Mar 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 20, 2023 •

edited

Loading

netlify bot commented Mar 20, 2023 •

edited

Loading

alculquicondor commented Mar 20, 2023 •

edited

Loading