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 ready!

Name	Link
🔨 Latest commit	`bf458c6`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/67000c7d0434c600085e7192
😎 Deploy Preview	https://deploy-preview-622--kubernetes-sigs-jobset.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

pkg/controllers/elastic_jobset.go

danielvegamyhre · 2024-09-27T16:04:29Z

pkg/controllers/jobset_controller.go

@@ -520,6 +527,22 @@ func (r *JobSetReconciler) reconcileReplicatedJobs(ctx context.Context, js *jobs
 	return nil
 }

+// We need to check if the replicas of a replicated job are mismatching
+// If they are, we should delete the extra jobs
+func (r *JobSetReconciler) downscaleElasticJobs(ctx context.Context, js *jobset.JobSet, replicatedJobStatus []jobset.ReplicatedJobStatus) error {


This can also be moved to the elastic_jobset.go file

I don't think it can. We keep anything related to the JobSetReconciler in jobset_controller.

ElasticJob and other utilites seem to be pure functions that can easily be tested with units. This does rely on the reconcile class to talk to the client.

That's what we've done so far but I think it makes more sense for all functions and struct methods that are feature-specific to be colocated in the same feature file, rather than group them based on whether or not the function has a pointer receiver. I think this is a useful pattern for organizing the methods of large structs with a growing number of methods.

For now we can leave this as is though, since this refactor I'm describing isn't really in the scope of this PR. I'll work on this refactor in a separate PR later and we can discuss the pros/cons of it in the PR.

Yea, in theory that sounds like a good idea. I tend to like separating code from controller logic as unit testing controllers is quite painful.

I will eagerly await your PR.

danielvegamyhre · 2024-09-27T16:18:19Z

pkg/controllers/elastic_jobset.go

+				if err != nil {
+					return nil, fmt.Errorf("unable get integer from job index key")
+				}
+				if jobIndex >= int(countOfJobsToDelete) {


So if we have 4 jobs (indexes 0,1,2,3) and we want to delete 1 job, then countOfJobsToDelete=1 and we may end up deleting any of job indexes 1,2,3 - so it is non-deterministic and may leave us with an non-contiguous range of job indexes (remaining jobs indexes 0,2,3).

Rather than this, I think we should delete jobs from largest job index to smallest, so the remaining jobs still occupy a contiguous index range. This follows the same pattern as how the Job controller behaves for elastic indexed jobs, deleting pods from largest to smallest completion indexes.

Good point. I went ahead and added this change.

@danielvegamyhre , keeping the indices unchanged could be desirable in cases where we don't want to restart the entire jobset right?

@danielvegamyhre , keeping the indices unchanged could be desirable in cases where we don't want to restart the entire jobset right?

Can you clarify what you mean? In this thread we are discussing which jobs to delete if the JobSet is scaled down (e.g., number of replicas in a replicatedJob reduced from 4 to 3) - not restarting the JobSet.

danielvegamyhre · 2024-09-27T16:20:51Z

pkg/controllers/elastic_jobset.go

+		countOfJobsToDelete := status.Ready - replicatedJob.Replicas
+		if countOfJobsToDelete > 0 {
+			jobsWeDeleted := 0
+			for _, val := range jobItems {


Ii think it would be more optimal (although perhaps not provide a big perf improvement in practice) to store the jobs in a map of "replicatedJobName -> [slice of jobs for that replicatedJob]". That way we don't need to iterate over every job in the jobset, for every replicatedJob we are downscaling.

The O(jobs) cost of constructing the map is preferable to the O(replicatedJobs * jobs) cost of this approach.

I ended up doing this to help with making sure I delete the right jobs.

danielvegamyhre · 2024-09-27T16:22:49Z

pkg/controllers/elastic_jobset.go

+					jobsToDelete = append(jobsToDelete, &val)
+				}
+				if jobsWeDeleted == int(countOfJobsToDelete) {
+					continue


we can break here rather than continue right, since there's no more jobs to delete in this inner loop?

kannon92 · 2024-10-14T12:29:40Z

@ahg-g id thinking we should clear use cases with this in mind. I am not a ML engineer so I’m not really able to give clear concrete cases for this. Let me ask around and see. Otherwise I may close this and wait until we have a customer ask for this.

kannon92 · 2024-10-14T15:06:42Z

/close

We need more design/use cases on this. Going to close for now.

k8s-ci-robot · 2024-10-14T15:06:48Z

@kannon92: Closed this PR.

In response to this:

/close

We need more design/use cases on this. Going to close for now.

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-sigs/prow repository.

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 2 times, most recently 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

danielvegamyhre reviewed Sep 27, 2024

View reviewed changes

kannon92 added 2 commits October 4, 2024 09:48

support elastic jobset

1b89010

daniel comments

bf458c6

kannon92 force-pushed the elastic-jobset-reopen branch from 58ca914 to bf458c6 Compare October 4, 2024 15:40

danielvegamyhre mentioned this pull request Oct 5, 2024

Support Elastic JobSets #463

Open

kannon92 requested a review from danielvegamyhre October 13, 2024 18:24

k8s-ci-robot closed this Oct 14, 2024

support elastic jobset #622

support elastic jobset #622

Conversation

kannon92 commented Jul 22, 2024

netlify bot commented Jul 22, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset ready!

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

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

danielvegamyhre Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielvegamyhre Oct 6, 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

kannon92 commented Oct 14, 2024

kannon92 commented Oct 14, 2024

k8s-ci-robot commented Oct 14, 2024

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

danielvegamyhre Sep 27, 2024 •

edited

Loading

danielvegamyhre Oct 6, 2024 •

edited

Loading