Add Coordinator field to JobSet spec #618

danielvegamyhre · 2024-07-15T19:56:15Z

Part of #617. I plan on adding validation logic for this new field in a follow up PR, as well as more tests.

Add coordinator field to JobSet spec, allowing the user to define the ReplicatedJob, JobIndex, and PodIndex of the pod they wish to be the global coordinator.
Add a new label/annotation, jobset.sigs.k8s.io/coordinator, which will store the stable network endpoint where the coordinator pod can be reached, when the jobset.spec.coordinator field is defined.
Minor refactoring of test code so it is more flexible, allowing us to do a "set or merge" operation when adding labels/annotations to Job or Pod wrappers.

k8s-ci-robot · 2024-07-15T19:56:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielvegamyhre

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

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

netlify · 2024-07-15T19:56:33Z

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	`8a1b2ee`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/66a400f9a531ec0008bc2f4f

googs1025 · 2024-07-16T01:27:15Z

api/jobset/v1alpha2/jobset_types.go

@@ -44,6 +44,11 @@ const (
 	// JobSetControllerName is the reserved value for the managedBy field for the built-in
 	// JobSet controller.
 	JobSetControllerName = "jobset.sigs.k8s.io/jobset-controller"
+
+	// CoordinatorKey is used as an annotation and label on Jobs and Pods. If the JobSet spec
+	// defines the .spec.coordinator field, this annotation/label will be added to store a stable


Hello, I have a small question, why do we need to put this tag on both the annotation and the label? Is it not enough to just use the label?

Yes but inconsistency between labels vs annotations and Job labels/annotations vs Pod labels/annotations has led to bugs in the past, as well as confusion for users. So while the API is still in alpha I've just been adding everything as both a label and annotation to both Jobs and Pods, so there is full consistency across Jobs, pods, labels, and annotations. Then for graduation to v1 I plan on reviewing these and assigning them more thoughtfully, and documenting them in the JobSet docsite.

Understood, thanks for your reply

googs1025 · 2024-07-20T16:22:13Z

I have a small question: Does this feature require the use of featureGate?

danielvegamyhre · 2024-07-20T23:19:30Z

I have a small question: Does this feature require the use of featureGate?

i don't think we'll add a feature gate for this one since it is isn't a risky feature. It just sets a new annotation and label on jobs and pods if the field is set.

danielvegamyhre · 2024-07-25T18:10:40Z

cc @kannon92 would you ming taking a look at this if you have time? Abdullah is OOO currently.

Happy to discuss the feature/context further if you'd like.

api/jobset/v1alpha2/jobset_types.go

kannon92 · 2024-07-25T19:10:21Z

pkg/controllers/jobset_controller.go

@@ -746,6 +746,12 @@ func labelAndAnnotateObject(obj metav1.Object, js *jobset.JobSet, rjob *jobset.R
 	annotations[jobset.JobIndexKey] = strconv.Itoa(jobIdx)
 	annotations[jobset.JobKey] = jobHashKey(js.Namespace, jobName)

+	// Apply coordinator annotation/label if a coordinator is defined in the JobSet spec.
+	if js.Spec.Coordinator != nil {
+		labels[jobset.CoordinatorKey] = coordinatorEndpoint(js)


One subtle bug here would be that if someone specifies these labels we would overwrite.

Do we want a warning if someone uses these labels?

Yep, I have a follow up PR with webhook changes ready to go after this is merged. Will tag you on it shortly.

kannon92 · 2024-07-25T19:57:50Z

sdk/python/docs/JobsetV1alpha2Coordinator.md

+------------ | ------------- | ------------- | -------------
+**job_index** | **int** | JobIndex is the index of Job which contains the coordinator pod (i.e., for a ReplicatedJob with N replicas, there are Job indexes 0 to N-1). Defaults to 0 if unset. | [optional] 
+**pod_index** | **int** | PodIndex is the Job completion index of the coordinator pod. Defaults to 0 if unset. | [optional] 
+**replicated_job** | **str** | ReplicatedJob is the name of the ReplicatedJob which contains the coordinator pod. | [default to '']


Where do we get "default to ''" from?

I'm not sure, I think because the replicatedJob field does not have "omitempty" so it will default to empty string if unset. In the follow up PR with webhook changes I will validate it set to a valid replicated job name, though.

kannon92 · 2024-07-26T11:14:45Z

/hold

/lgtm

Code looks good. I have some minor nits. Feel free to unhold.

danielvegamyhre · 2024-07-26T20:04:04Z

@kannon92 can you reapply LGTM when you have a sec please? I made changes to address the comments and it was removed

kannon92 · 2024-07-26T20:15:25Z

/hold cancel
/lgtm

danielvegamyhre added 2 commits July 12, 2024 02:56

update wrappers

345cc92

initial commit for coordinator support

95fc11b

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 15, 2024

k8s-ci-robot requested review from ahg-g and kannon92 July 15, 2024 19:56

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2024

danielvegamyhre force-pushed the multicell branch from a9b40a6 to 95fc11b Compare July 15, 2024 20:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 15, 2024

googs1025 reviewed Jul 16, 2024

View reviewed changes

danielvegamyhre mentioned this pull request Jul 25, 2024

WIP: Used fixed API for Job Pod Failure Policy #620

Closed

kannon92 reviewed Jul 25, 2024

View reviewed changes

api/jobset/v1alpha2/jobset_types.go Outdated Show resolved Hide resolved

kannon92 reviewed Jul 25, 2024

View reviewed changes

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

k8s-ci-robot assigned kannon92 Jul 26, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 26, 2024

remove comments about defaults

8a1b2ee

danielvegamyhre force-pushed the multicell branch from 18b12de to 8a1b2ee Compare July 26, 2024 20:03

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 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2024

k8s-ci-robot merged commit a1bbff3 into kubernetes-sigs:main Jul 26, 2024
13 checks passed

This was referenced Jul 26, 2024

Validation for Coordinator field #627

Merged

Support for defining a global coordinator pod in the JobSet spec #617

Closed

danielvegamyhre mentioned this pull request Aug 19, 2024

Release v0.6.0 #655

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Coordinator field to JobSet spec #618

Add Coordinator field to JobSet spec #618

danielvegamyhre commented Jul 15, 2024

k8s-ci-robot commented Jul 15, 2024

netlify bot commented Jul 15, 2024 •

edited

Loading

googs1025 Jul 16, 2024

danielvegamyhre Jul 16, 2024 •

edited

Loading

googs1025 Jul 16, 2024

googs1025 commented Jul 20, 2024

danielvegamyhre commented Jul 20, 2024

danielvegamyhre commented Jul 25, 2024 •

edited

Loading

kannon92 Jul 25, 2024

danielvegamyhre Jul 26, 2024

kannon92 Jul 25, 2024

danielvegamyhre Jul 26, 2024

kannon92 commented Jul 26, 2024

danielvegamyhre commented Jul 26, 2024

kannon92 commented Jul 26, 2024

Add Coordinator field to JobSet spec #618

Add Coordinator field to JobSet spec #618

Conversation

danielvegamyhre commented Jul 15, 2024

k8s-ci-robot commented Jul 15, 2024

netlify bot commented Jul 15, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

googs1025 Jul 16, 2024

Choose a reason for hiding this comment

danielvegamyhre Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

googs1025 Jul 16, 2024

Choose a reason for hiding this comment

googs1025 commented Jul 20, 2024

danielvegamyhre commented Jul 20, 2024

danielvegamyhre commented Jul 25, 2024 • edited Loading

kannon92 Jul 25, 2024

Choose a reason for hiding this comment

danielvegamyhre Jul 26, 2024

Choose a reason for hiding this comment

kannon92 Jul 25, 2024

Choose a reason for hiding this comment

danielvegamyhre Jul 26, 2024

Choose a reason for hiding this comment

kannon92 commented Jul 26, 2024

danielvegamyhre commented Jul 26, 2024

kannon92 commented Jul 26, 2024

netlify bot commented Jul 15, 2024 •

edited

Loading

danielvegamyhre Jul 16, 2024 •

edited

Loading

danielvegamyhre commented Jul 25, 2024 •

edited

Loading