Keep adjusted workload resources in sync with limitRanges and runtimeClasses #653

trasc · 2023-03-20T13:16:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Keeps in sync the adjusted workload resource needs (which are computed based on the workload's spec, potential cluster resourceClass definition and potential namespace limitRange definition) , with the cluster context (limitRanges and runtimeClasses).

In addition, now, the admission status struct keeps track of the workload's resource needs at the admission time, this values being the source for computing the resource needs for the admitted workloads.

Which issue(s) this PR fixes:

Fixes #611

Special notes for your reviewer:

netlify · 2023-03-20T13:16:47Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`a956f88`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/641c6b8849b9bd0008234cc1

k8s-ci-robot · 2023-03-20T13:16:48Z

Hi @trasc. 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:52:52Z

/ok-to-test

trasc · 2023-03-20T19:40:55Z

/cc @alculquicondor
/cc @mwielgus

apis/kueue/v1beta1/workload_types.go

alculquicondor · 2023-03-21T13:53:13Z

apis/kueue/v1beta1/workload_types.go

@@ -71,6 +71,12 @@ type PodSetFlavors struct {

 	// Flavors are the flavors assigned to the workload for each resource.
 	Flavors map[corev1.ResourceName]ResourceFlavorReference `json:"flavors,omitempty"`


we are on time to change PodSetFlavors by something else, given that it's no longer just about flavors.

Maybe PodSetAssignments is a bit more generic?

And we could have a single list holding both the quantity and the flavor, instead of two maps. I'm not to sure about this one. How does the calculation of workload.Info change if we do this?

For the first part done.

For the second one ... , it will imply adding two addition conversion methods ... but it will look strange. Also since ResourceList is "standard", I'd say to keep it as is. (also the time that will take to re-re-work the tests might not be negligible )

apis/kueue/v1beta1/workload_types.go

pkg/workload/workload.go

pkg/controller/core/workload_controller.go

pkg/controller/core/indexer/indexer.go

pkg/cache/snapshot_test.go

pkg/scheduler/preemption/preemption_test.go

test/integration/controller/core/workload_controller_test.go

trasc · 2023-03-22T07:49:43Z

/retest

pkg/controller/core/workload_controller.go

test/integration/controller/core/workload_controller_test.go

test/util/util.go

test/integration/scheduler/workload_controller_test.go

alculquicondor · 2023-03-22T20:03:35Z

just nits

alculquicondor

/approve

Just couple of typos

test/integration/scheduler/workload_controller_test.go

k8s-ci-robot · 2023-03-23T13:35:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-03-23T14:17:05Z

/label tide/merge-method-squash
/lgtm

alculquicondor · 2023-03-23T14:19:45Z

/lgtm cancel

It looks like this is introducing a flaky test. Please investigate

alculquicondor · 2023-03-23T14:21:03Z

https://testgrid.k8s.io/sig-scheduling#periodic-kueue-test-integration-main testgrid looks fine, so I don't think this is a problem in the current test suite.

alculquicondor · 2023-03-23T14:34:13Z

test/integration/scheduler/workload_controller_test.go

+			gomega.Expect(k8sClient.Create(ctx, localQueue)).To(gomega.Succeed())
+		})
+		ginkgo.AfterEach(func() {
+			ginkgo.By("Resource consumption should be 0", func() {


Suggested change

ginkgo.By("Resource consumption should be 0", func() {

ginkgo.By("Resource consumption should be 0 and no pending workloads", func() {

alculquicondor · 2023-03-23T14:37:59Z

apis/kueue/v1beta1/workload_types.go

+	//
+	// Beside what is provided in podSet's specs, this calculation takes into account
+	// the LimitRange defaults and RuntimeClass overheads at the moment of admission.
+	ResourceUsage corev1.ResourceList `json:"resourceUsage,omitempty"`


Can you make a PR that includes the field name changes without adding ResourceUsage?

I would like to merge that before cutting the release.

If this PR ends up taking longer, we can leave it to the next release.

Set both the flavor and the request values in a single call.

…er tests

alculquicondor · 2023-03-23T15:25:15Z

passed 1/3

/test pull-kueue-test-integration-main

alculquicondor · 2023-03-23T15:42:16Z

passed 2/3

/test pull-kueue-test-integration-main

/milestone v0.3

alculquicondor · 2023-03-23T16:01:04Z

passed 3/3

/lgtm

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 Mar 20, 2023

k8s-ci-robot requested review from ahg-g and kerthcet March 20, 2023 13:16

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 20, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels 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

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch from 0490283 to ff53e85 Compare March 20, 2023 19:39

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 20, 2023

trasc marked this pull request as ready for review March 20, 2023 19:39

k8s-ci-robot removed 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 a review from alculquicondor March 20, 2023 19:39

k8s-ci-robot requested a review from mwielgus March 20, 2023 19:40

alculquicondor reviewed Mar 21, 2023

View reviewed changes

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch 2 times, most recently from ff0c7e3 to 36f230d Compare March 21, 2023 15:02

alculquicondor reviewed Mar 21, 2023

View reviewed changes

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch 2 times, most recently from f2a40c7 to 7c4469a Compare March 22, 2023 07:43

alculquicondor reviewed Mar 22, 2023

View reviewed changes

[api/workload] Keep track of the resource needs at the admission time.

7b7fb66

trasc added 3 commits March 23, 2023 11:24

[api/workload] Rename PodSetFlavors to PodSetAssignments

465321a

[test/integration] Move resource specific workload tests in scheduler

f1b5e17

[test/util] Don't check the status of nil Flavors

cd991fd

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch from 77073b5 to 39c14be Compare March 23, 2023 09:39

alculquicondor reviewed Mar 23, 2023

View reviewed changes

test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved

test/integration/scheduler/workload_controller_test.go Outdated Show resolved Hide resolved

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

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch from 39c14be to f9017ca Compare March 23, 2023 14:15

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

k8s-ci-robot assigned alculquicondor Mar 23, 2023

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

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2023

alculquicondor reviewed Mar 23, 2023

View reviewed changes

trasc added 3 commits March 23, 2023 17:08

[util/testing] Set assignments in one call

89e5eb8

Set both the flavor and the request values in a single call.

[test/integration] Add update limits near workload delete

754e5e4

[flaky tests] Wait for flavor and queue deletion in workload controll…

a956f88

…er tests

trasc force-pushed the keep_adjusted_workload_resources_in_sync branch from f9017ca to a956f88 Compare March 23, 2023 15:08

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

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

k8s-ci-robot merged commit db8e38f into kubernetes-sigs:main Mar 23, 2023

trasc mentioned this pull request Mar 29, 2023

Record original node selectors #660

Merged

trasc mentioned this pull request Apr 19, 2023

REQUEST: New membership for trasc kubernetes/org#4168

Closed

9 tasks

trasc deleted the keep_adjusted_workload_resources_in_sync branch April 20, 2023 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep adjusted workload resources in sync with limitRanges and runtimeClasses #653

Keep adjusted workload resources in sync with limitRanges and runtimeClasses #653

trasc commented Mar 20, 2023

netlify bot commented Mar 20, 2023 •

edited

Loading

k8s-ci-robot commented Mar 20, 2023

alculquicondor commented Mar 20, 2023

trasc commented Mar 20, 2023

alculquicondor Mar 21, 2023

trasc Mar 21, 2023

trasc commented Mar 22, 2023

alculquicondor commented Mar 22, 2023

alculquicondor left a comment

k8s-ci-robot commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor Mar 23, 2023

alculquicondor Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

		@@ -71,6 +71,12 @@ type PodSetFlavors struct {

		// Flavors are the flavors assigned to the workload for each resource.
		Flavors map[corev1.ResourceName]ResourceFlavorReference `json:"flavors,omitempty"`

	ginkgo.By("Resource consumption should be 0", func() {
	ginkgo.By("Resource consumption should be 0 and no pending workloads", func() {

Keep adjusted workload resources in sync with limitRanges and runtimeClasses #653

Keep adjusted workload resources in sync with limitRanges and runtimeClasses #653

Conversation

trasc commented Mar 20, 2023

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:

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

trasc commented Mar 20, 2023

alculquicondor Mar 21, 2023

Choose a reason for hiding this comment

trasc Mar 21, 2023

Choose a reason for hiding this comment

trasc commented Mar 22, 2023

alculquicondor commented Mar 22, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor Mar 23, 2023

Choose a reason for hiding this comment

alculquicondor Mar 23, 2023

Choose a reason for hiding this comment

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

alculquicondor commented Mar 23, 2023

netlify bot commented Mar 20, 2023 •

edited

Loading