support elastic jobset #622

kannon92 · 2024-07-22T18:29:14Z

Fixes #463

Add elastic jobset support (downscale/upscaling replicas of a Replicated Job).

Before this change, we treated ReplicateJobs (in JobSet.Spec) as immutable. This relaxes that restriction.

I choose to validate that names cannot be changed and the job templates must not change during updates. I don't see a reason for names to change and we also don't allow order change of the replicated job list so we can detect differences easily.

I am reopening this to resolve the rebase labels..

netlify · 2024-07-22T18:29:33Z

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	`58ca914`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/66c0acbe016c860008d463a0

kannon92 · 2024-07-24T12:22:18Z

/assign @danielvegamyhre

@mimowo wouls you be open for a review also?

kannon92 · 2024-07-25T13:02:38Z

/cc @tenzen-y

kannon92 · 2024-07-27T00:22:37Z

/hold

Missed the conflicts..

mimowo · 2024-07-29T07:18:39Z

@mimowo wouls you be open for a review also?

Ack, I will try this week, but don't wait for my review unnecessarily if you get enough input from others.

k8s-ci-robot · 2024-07-29T13:43:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kannon92
Once this PR has been reviewed and has the lgtm label, please ask for approval from danielvegamyhre. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

kannon92 · 2024-07-29T13:49:59Z

@danielvegamyhre do you know how to remove the rebase label? I did resolve conflicts in my branch and pushes those up. I think the prow removal of it is not working.

ref: https://kubernetes.slack.com/archives/C09QZ4DQB/p1708018595891739

Looks like rebase label can have issues. So I think removing is fine. It also doesn't block the merge so this is ready for review.

kannon92 · 2024-07-29T19:10:28Z

/remove needs-rebase

danielvegamyhre · 2024-07-29T20:56:28Z

@danielvegamyhre do you know how to remove the rebase label? I did resolve conflicts in my branch and pushes those up. I think the prow removal of it is not working.

ref: https://kubernetes.slack.com/archives/C09QZ4DQB/p1708018595891739

Looks like rebase label can have issues. So I think removing is fine. It also doesn't block the merge so this is ready for review.

I removed it via the github UI, the "labels" button on the top right.

kannon92 · 2024-07-29T21:24:18Z

I think since you have admin rights to the repo you have access to that.

Thank you though!

kannon92 · 2024-07-30T13:54:02Z

/hold cancel

mimowo · 2024-07-30T14:25:23Z

pkg/webhooks/jobset_webhook.go

+	// Comparing job templates can be slow
+	// Only do it if we detect a difference.
+	if !equality.Semantic.DeepEqual(mungedSpec.ReplicatedJobs, oldJS.Spec.ReplicatedJobs) {
+		if err := validateReplicatedJobsUpdate(mungedSpec.ReplicatedJobs, oldJS.Spec.ReplicatedJobs); err != nil {


It seems this allows to update the number of replicas for an unsuspended JobSet. Is this desired?
I this would not work if there is no mechanism to recreate the Jobs. Maybe it is better to only support mutability of replicas when the JobSet is suspended?

I was thinking that one would want to add more replicated jobs as a major use case for Elastic JobSet.

We debated about whether or not we should modify the indexed job or the JobSet. I think we went with scaling up/down on the replicas of a ReplicatedJob.

Are you thinking that elasticity should only occur on a suspended object?

I think it is ok to support elasticity for running Jobs, but you may test e2e if this really works in this PR (I doubt).

IIUC this requires an update to the underlying Job object no? IIUC you only update the JobSet object, but this does not impact the underlying Jobs which are running, but I might be missing something.

Also, I think changing paralellism only works for IndexedJobs where completions = parallelism.

If my thinking is correct, then your PR works ok, by only if combined with #625, and only if the JobSet is updated while suspended.

Overall it would be good to demonstrate this feature works with an e2e test.

Sorry, I have probably mixed a single Job mutability and the number of Job replicas.

In this PR you only let to update the Job replicas count, right?

Still, it is not clear to me if we already have code to wcale up and down the number of replicas.

I did a e2e test and scaling up works. I create a JobSet and then I edit the replicated jobs of a worker jobset and I can confirm that new jobs are created.

I change the replicated jobs. I should check on scaling down actually.

I create a JobSet and then I edit the replicated jobs of a worker jobset and I can confirm that new jobs are created.

Great!

I change the replicated jobs. I should check on scaling down actually.

Please share the results of the testing.

Downscaling doesn’t seem to work.

I think I’ll add some e2e tests for this as it “works” for integration tests so there may be some subtleties

Thanks for the update!

mimowo · 2024-07-30T14:26:29Z

pkg/webhooks/jobset_webhook_test.go

@@ -1599,7 +1599,7 @@ func TestValidateUpdate(t *testing.T) {
 			}.ToAggregate(),
 		},
 		{
-			name: "replicated job pod template can be updated for suspended jobset",
+			name: "replicated jobs updates can change replicas",


Make sure there are two versions of the test: for suspended JobSet and unsuspended.

Would that be relevant once #625 merges?

sorry, I see what you mean.

pushed up a change.

mimowo · 2024-08-01T09:51:22Z

test/integration/controller/jobset_controller_test.go

+				},
+				{
+					checkJobCreation: func(js *jobset.JobSet) {
+						expectedStarts := 7


It will be good to add a comment why 7 is expected. It is a bit magical number here. Maybe also "numJobs" is a better name? Not sure why this is called expectedStarts while it is checked against NumJobs.

mimowo · 2024-08-01T09:51:36Z

test/integration/controller/jobset_controller_test.go

+				},
+				{
+					checkJobCreation: func(js *jobset.JobSet) {
+						expectedStarts := 2


Similarly here. Please add a comment why 2 is expected.

kannon92 · 2024-08-04T14:14:03Z

Pushed up some e2e tests for this. I need to work on the downscale case.

What should be the behavior for elastic jobs on a downscale?

If more jobs exists, do we delete the extra jobs only? Or should we delete and recreate all the jobs?

On upscale, the extra jobs are created and I think the original ones are left alone.

On downscale, I guess we could delete the extra replicas.

danielvegamyhre · 2024-08-06T17:20:34Z

@kannon92 Looks like e2e tests are failing, can you take a look?

kannon92 · 2024-08-06T17:33:45Z

Yes, my hope is to get some clarity on the design/code.

Downscaling doesn't work for sure. What should we do on a downscale of replicas in ReplicatedJobs? Do we delete all the jobs and recreate? Or do we terminate the jobs that are not in the list of replicas?

pkg/webhooks/jobset_webhook.go

danielvegamyhre · 2024-08-07T17:41:27Z

Yes, my hope is to get some clarity on the design/code.

Downscaling doesn't work for sure. What should we do on a downscale of replicas in ReplicatedJobs? Do we delete all the jobs and recreate? Or do we terminate the jobs that are not in the list of replicas?

Hmm, I think if replicatedJob.replicas is downscaled from N to N-2, we should just delete 2 jobs so the state matches the spec.

kannon92 · 2024-08-08T02:36:54Z

Yes, my hope is to get some clarity on the design/code.
Downscaling doesn't work for sure. What should we do on a downscale of replicas in ReplicatedJobs? Do we delete all the jobs and recreate? Or do we terminate the jobs that are not in the list of replicas?

Hmm, I think if replicatedJob.replicas is downscaled from N to N-2, we should just delete 2 jobs so the state matches the spec.

Maybe I’m overthinking it.

Say I have 4 jobs and I downscale to 2. Should I only delete the 3rd and 4th replicated job via index? Say I want to keep the 1st and 2nd job running and I remove the excess ones?

danielvegamyhre · 2024-08-08T04:07:54Z

Yes, my hope is to get some clarity on the design/code.
Downscaling doesn't work for sure. What should we do on a downscale of replicas in ReplicatedJobs? Do we delete all the jobs and recreate? Or do we terminate the jobs that are not in the list of replicas?

Hmm, I think if replicatedJob.replicas is downscaled from N to N-2, we should just delete 2 jobs so the state matches the spec.

Maybe I’m overthinking it.

Say I have 4 jobs and I downscale to 2. Should I only delete the 3rd and 4th replicated job via index? Say I want to keep the 1st and 2nd job running and I remove the excess ones?

If I recall correctly the Job controller works this way for Elastic Indexed Jobs, removing indexes from largest to smallest until there are no more excess pods, so it would make sense to follow that pattern here for consistency I think.

kannon92 · 2024-08-17T14:31:15Z

@danielvegamyhre @mimowo

I got the downscale to work so I think this PR is ready for review.

danielvegamyhre · 2024-09-08T16:51:45Z

pkg/webhooks/jobset_webhook_test.go

+			want: fmt.Errorf("updates can not change job names or reorder the jobs"),
+		},
+		{
+			name: "updates on replicated job templates are not allowed",


Why do we want to disallow job template updates? This would prevent us from using Elastic Indexed Jobs, which I'm hoping could be used in a more efficient implementation of exclusive job placement (see #482)

See this comment: #463 (comment).

I wasn’t sure if there was a use case for this so I omitted support for it.

@ahg-g would this be a strong enough usecase to add support for ElasticIndexedJobs.

k8s-ci-robot requested review from danielvegamyhre and moficodes July 22, 2024 18:29

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 22, 2024

k8s-ci-robot assigned danielvegamyhre Jul 24, 2024

k8s-ci-robot requested a review from tenzen-y July 25, 2024 13:02

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 27, 2024

kannon92 force-pushed the elastic-jobset-reopen branch from 086a389 to 3f3a2ac Compare July 29, 2024 13:43

danielvegamyhre removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2024

mimowo reviewed Jul 30, 2024

View reviewed changes

mimowo reviewed Aug 1, 2024

View reviewed changes

danielvegamyhre reviewed Aug 7, 2024

View reviewed changes

pkg/webhooks/jobset_webhook.go Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2024

kannon92 force-pushed the elastic-jobset-reopen branch 2 times, most recently from f4e6034 to b266034 Compare August 13, 2024 00:18

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed 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. labels Aug 13, 2024

kannon92 force-pushed the elastic-jobset-reopen branch from 2c8ef3a to fcd1bd1 Compare August 17, 2024 03:31

support elastic jobset

58ca914

kannon92 force-pushed the elastic-jobset-reopen branch from fcd1bd1 to 58ca914 Compare August 17, 2024 13:59

kannon92 requested review from danielvegamyhre and mimowo August 17, 2024 14:30

danielvegamyhre added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 8, 2024

danielvegamyhre reviewed Sep 8, 2024

View reviewed changes

support elastic jobset #622

Are you sure you want to change the base?

support elastic jobset #622

Conversation

kannon92 commented Jul 22, 2024

netlify bot commented Jul 22, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

kannon92 commented Jul 24, 2024

kannon92 commented Jul 25, 2024

kannon92 commented Jul 27, 2024

mimowo commented Jul 29, 2024

k8s-ci-robot commented Jul 29, 2024

kannon92 commented Jul 29, 2024 • edited Loading

kannon92 commented Jul 29, 2024

danielvegamyhre commented Jul 29, 2024

kannon92 commented Jul 29, 2024 • edited Loading

kannon92 commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

mimowo Aug 1, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Aug 1, 2024 • 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

kannon92 commented Aug 4, 2024

danielvegamyhre commented Aug 6, 2024

kannon92 commented Aug 6, 2024

danielvegamyhre commented Aug 7, 2024

kannon92 commented Aug 8, 2024

danielvegamyhre commented Aug 8, 2024 • edited Loading

kannon92 commented Aug 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jul 22, 2024 •

edited

Loading

kannon92 commented Jul 29, 2024 •

edited

Loading

kannon92 commented Jul 29, 2024 •

edited

Loading

mimowo Jul 30, 2024 •

edited

Loading

kannon92 Jul 30, 2024 •

edited

Loading

mimowo Aug 1, 2024 •

edited

Loading

mimowo Aug 1, 2024 •

edited

Loading

danielvegamyhre commented Aug 8, 2024 •

edited

Loading