Dynamically reclaiming resources #756

trasc · 2023-05-09T10:17:34Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add the support for dynamic resources reclaim.

Which issue(s) this PR fixes:

Fixes #78

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add the support for dynamic resources reclaim.

netlify · 2023-05-09T10:17:43Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`cf30176`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/645e7cf2febc69000831fabc

trasc · 2023-05-09T13:50:00Z

/cc @alculquicondor

apis/kueue/v1beta1/workload_types.go

apis/kueue/webhooks/workload_webhook.go

apis/kueue/v1beta1/workload_types.go

pkg/workload/workload.go

test/integration/controller/core/clusterqueue_controller_test.go

pkg/controller/jobframework/reconciler.go

test/integration/controller/job/job_controller_test.go

test/util/util.go

apis/kueue/webhooks/workload_webhook.go

pkg/controller/jobframework/interface.go

apis/kueue/webhooks/workload_webhook.go

pkg/controller/jobframework/reconciler.go

pkg/controller/jobs/job/job_controller.go

pkg/workload/workload.go

pkg/controller/jobs/job/job_controller.go

apis/kueue/v1beta1/workload_types.go

apis/kueue/webhooks/workload_webhook.go

pkg/controller/jobframework/reconciler.go

pkg/workload/workload.go

alculquicondor · 2023-05-11T18:09:49Z

apis/kueue/v1beta1/workload_types.go

+	// +optional
+	// +listType=map
+	// +listMapKey=name
+	ReclaimablePods []ReclaimablePod `json:"reclaimablePods,omitempty"`


I'm having second thoughts here.

If we assume that the admission section can be modified after admission (to update Count and usage), then I prefer we have reclaimableCount inside the admission struct.

I think this might be a better idea as we move towards elastic jobs. @tenzen-y ?

then I prefer we have reclaimableCount inside the admission struct.

Does this mean we have reclaimableCount inside the admission instead of having this ReclaimablePods (removing ReclaimablePods )?

yes, that's what I'm thinking.

IIRC, we selected ReclaimablePods instead of reclaimableCount since it could be misleading when the job is suspended.

#742 (comment)

However, as @alculquicondor says, we can ignore that concern if we have reclaimableCount inside the Admission.

I think this might be a better idea as we move towards elastic jobs.

right. The Admission has the reclaimableCount would be more natural for the elastic job.

@trasc Are you concerned about having reclaimableCount inside the Admission?

If we assume that the admission section can be modified after admission (to update Count and usage), then I prefer we have reclaimableCount inside the admission struct.

The admission is not changed when reclaimable changes.

@trasc Are you concerned about having reclaimableCount inside the Admission?

SSA conflicts. We should keep them separated.

Let's say we have elastic jobs. Then the kueue scheduler+preemption could update count.

However, this is generally independent from the concept of reclaiming pods.

Ok, let's keep it like this.

Any reason why not make this map[string]int32?

just to look similar to flavors and usage

alculquicondor

A few nits

apis/kueue/v1beta1/workload_types.go

alculquicondor · 2023-05-12T13:58:01Z

apis/kueue/v1beta1/workload_types.go

+	// +optional
+	// +listType=map
+	// +listMapKey=name
+	ReclaimablePods []ReclaimablePod `json:"reclaimablePods,omitempty"`


Let's say we have elastic jobs. Then the kueue scheduler+preemption could update count.

However, this is generally independent from the concept of reclaiming pods.

Ok, let's keep it like this.

Any reason why not make this map[string]int32?

pkg/controller/jobframework/reconciler.go

alculquicondor · 2023-05-12T14:14:19Z

pkg/workload/workload.go

@@ -288,3 +341,30 @@ func GetQueueOrderTimestamp(w *kueue.Workload) *metav1.Time {
 func IsAdmitted(w *kueue.Workload) bool {
 	return apimeta.IsStatusConditionTrue(w.Status.Conditions, kueue.WorkloadAdmitted)
 }
+
+// UpdateReclaimablePods updates the ReclaimablePods list for the workload wit SSA.
+func UpdateReclaimablePods(ctx context.Context, c client.Client, w *kueue.Workload, reclaimablePods []kueue.ReclaimablePod) error {


Suggested change

func UpdateReclaimablePods(ctx context.Context, c client.Client, w *kueue.Workload, reclaimablePods []kueue.ReclaimablePod) error {

func ApplyReclaimablePods(ctx context.Context, c client.Client, w *kueue.Workload, reclaimablePods []kueue.ReclaimablePod) error {

Let's keep "Apply" , for when we only push parts of a workload to the api server.

Which this does?

reclaimablePods []kueue.ReclaimablePod is not part of the workload now, w *kueue.Workload is used to identify the target.

vs func ApplyAdmissionStatus(ctx context.Context, c client.Client, w *kueue.Workload, strict bool) error where w has the content set

I want to be able to quickly identify which function is using Update (PUT), versus SSA (PATCH). For that reason, I find Apply very obvious.

But we can clean this up later.

alculquicondor · 2023-05-12T14:14:48Z

pkg/workload/workload.go

+		r[name] /= f
+	}
+}
+
 // UpdateStatus updates the condition of a workload with ssa,
 // filelManager being set to managerPrefix + "-" + conditionType
 func UpdateStatus(ctx context.Context,


Can you rename to ApplyStatus?

Let's keep "Apply" , for when we only push parts of a workload to the api server.

pkg/controller/jobframework/reconciler.go

pkg/workload/workload_test.go

tenzen-y · 2023-05-12T18:04:32Z

test/integration/controller/job/job_controller_test.go

+			gomega.Eventually(func() []kueue.ReclaimablePod {
+				gomega.Expect(k8sClient.Get(ctx, wlKey, wl)).Should(gomega.Succeed())
+				return wl.Status.ReclaimablePods
+


Suggested change

Reverting this empty line would be good.

tenzen-y · 2023-05-12T18:06:37Z

Totally LGTM.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2023-05-12T18:30:59Z

[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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels May 9, 2023

k8s-ci-robot requested review from kerthcet and tenzen-y May 9, 2023 10:17

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

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 9, 2023

trasc force-pushed the dyn-reclaim branch from c2d17d3 to 814bb9e Compare May 9, 2023 10:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 9, 2023

trasc force-pushed the dyn-reclaim branch from 814bb9e to c2d17d3 Compare May 9, 2023 10:45

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2023

k8s-ci-robot requested a review from alculquicondor May 9, 2023 13:50

alculquicondor reviewed May 9, 2023

View reviewed changes

trasc force-pushed the dyn-reclaim branch from c2d17d3 to 0d84005 Compare May 10, 2023 07:35

trasc mentioned this pull request May 10, 2023

Allow partial admission of jobs #420

Closed

3 tasks

tenzen-y reviewed May 10, 2023

View reviewed changes

trasc force-pushed the dyn-reclaim branch 2 times, most recently from de1ca9e to 5f30896 Compare May 11, 2023 07:00

tenzen-y reviewed May 11, 2023

View reviewed changes

pkg/controller/jobs/job/job_controller.go Outdated Show resolved Hide resolved

trasc force-pushed the dyn-reclaim branch from 5f30896 to b7899e3 Compare May 11, 2023 11:41

alculquicondor reviewed May 11, 2023

View reviewed changes

trasc added 3 commits May 12, 2023 08:14

[api][workload] Add ReclaimablePods in status

d4de946

[api][workload] Track the count of pods used during admission

fab9584

[workload] Track the number of pods used during adimssion

60871b0

trasc force-pushed the dyn-reclaim branch from b7899e3 to c640cf0 Compare May 12, 2023 08:08

alculquicondor reviewed May 12, 2023

View reviewed changes

trasc force-pushed the dyn-reclaim branch from c640cf0 to d60b39c Compare May 12, 2023 16:22

tenzen-y reviewed May 12, 2023

View reviewed changes

pkg/workload/workload_test.go Outdated Show resolved Hide resolved

trasc added 4 commits May 12, 2023 20:52

[workload] Account for reclaimable pods

d894851

[workload] Account for reclaimable pod - integration tests

3720ecd

[jobframework] Track reclaimable job counts

1d57739

[webhook][workload] Validate reclaimable pods

cf30176

trasc force-pushed the dyn-reclaim branch from d60b39c to cf30176 Compare May 12, 2023 17:52

tenzen-y reviewed May 12, 2023

View reviewed changes

alculquicondor reviewed May 12, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor May 12, 2023

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

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

k8s-ci-robot merged commit 60cec2f into kubernetes-sigs:main May 12, 2023

trasc deleted the dyn-reclaim branch May 15, 2023 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically reclaiming resources #756

Dynamically reclaiming resources #756

trasc commented May 9, 2023

netlify bot commented May 9, 2023 •

edited

Loading

trasc commented May 9, 2023

alculquicondor May 11, 2023

tenzen-y May 11, 2023

alculquicondor May 11, 2023

tenzen-y May 11, 2023

trasc May 12, 2023

alculquicondor May 12, 2023

trasc May 12, 2023

alculquicondor left a comment

alculquicondor May 12, 2023

alculquicondor May 12, 2023

trasc May 12, 2023

alculquicondor May 12, 2023

trasc May 12, 2023 •

edited

Loading

alculquicondor May 12, 2023

alculquicondor May 12, 2023

trasc May 12, 2023

tenzen-y May 12, 2023

tenzen-y commented May 12, 2023

alculquicondor left a comment

k8s-ci-robot commented May 12, 2023

	func UpdateReclaimablePods(ctx context.Context, c client.Client, w *kueue.Workload, reclaimablePods []kueue.ReclaimablePod) error {
	func ApplyReclaimablePods(ctx context.Context, c client.Client, w *kueue.Workload, reclaimablePods []kueue.ReclaimablePod) error {

Dynamically reclaiming resources #756

Dynamically reclaiming resources #756

Conversation

trasc commented May 9, 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:

Does this PR introduce a user-facing change?

netlify bot commented May 9, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

trasc commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trasc May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented May 12, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 12, 2023

netlify bot commented May 9, 2023 •

edited

Loading

trasc May 12, 2023 •

edited

Loading