Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy #309

danielvegamyhre · 2023-09-26T19:19:04Z

Fixes #310

This PR implements the 2nd option described in the linked issue. Specifically, it adds a pod mutating and admission webhook which does the following:

For leader pods, inject exclusive affinities and allow them to be created.
For follower pods, reject admission attempts until the leader pod has been scheduled, then inject a nodeSelector for the same topology as the leader is scheduled on.

The pod reconciler is minimal in this PR, and only has the bare minimum logic needed to setup the indexes used by the webhook to look up pods by name and look up all pods for a job.

Once this PR is submitted, I'll create a follow-up PR with the pod reconciler changes necessary to handle race conditions that can arise in this webhook based exclusive placement strategy.

k8s-ci-robot · 2023-09-26T19:19:10Z

[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

main.go

pkg/util/names/names.go

pkg/controllers/pod_controller.go

api/jobset/v1alpha2/jobset_webhook.go

api/jobset/v1alpha2/jobset_webhook_test.go

config/components/webhook/manifests.yaml

pkg/controllers/pod_controller.go

api/jobset/v1alpha2/pod_webhook.go

pkg/controllers/pod_controller.go

config/components/webhook/manifests.yaml

danielvegamyhre · 2023-09-27T00:34:11Z

/retest

kannon92 · 2023-09-27T01:44:14Z

pkg/controllers/jobset_controller.go

 	// if Suspend is set, then we assume all jobs will be suspended also.
 	jobsetSuspended := js.Spec.Suspend != nil && *js.Spec.Suspend
 	job.Spec.Suspend = ptr.To(jobsetSuspended)

 	return job, nil
 }

-// Appends pod affinity/anti-affinity terms to the job pod template spec,


I remember a discussion with @vsoch around these annotations. Should we implement the new way of placements in case of users using these annotations rather than deleting this entirely?

I'm not familiar with the context of this PR, but I think a feature of jobset definitely should be to make it easy to ask for common configurations for affinity (or other similar needs for placement). In my case, I did try using one of the defaults here, but it was slightly off for what I needed (so I couldn't use this strategy) and had to implement an entire function akin to the below, with tweaks. I would say it would make sense to:

Define a list of common use cases for affinity / anti-affinity

Associate them with names the user can ask for in a section of the spec

Implement so it's ridiculously easy to just request a named set!

And in this case, it would make sense to only deprecate the existing when an alternative is available. It might be unlikely that someone is using them, in which case maybe OK to delete first, but perhaps adding the new sets first makes sense so if anyone is using them they aren't really angry :)

pkg/webhooks/pod_webhook.go

sdk/python/docs/JobsetV1alpha2PodAnnotator.md

pkg/controllers/pod_controller.go

pkg/webhooks/pod_mutating_webhook.go

ahg-g · 2023-10-05T00:20:56Z

You can use make build on your local environment to check for such compiler errors

pkg/webhooks/pod_admission_webhook.go

pkg/webhooks/pod_mutating_webhook.go

pkg/webhooks/pod_admission_webhook.go

danielvegamyhre · 2023-11-19T03:16:53Z

/retest

pkg/controllers/jobset_controller_test.go

main.go

ahg-g · 2023-11-22T17:55:57Z

pkg/util/shared/shared.go

+	return fmt.Sprintf("%s-%s-%d", jsName, rjobName, jobIndex)
+}
+
+func IsLeaderPod(pod *corev1.Pod) bool {


util/shared sounds repetitive, shared is just another way of saying util.

Do we really need this function here? We probably only need it in the webhook, so lets just define it there and keep this pkg named "names"

I ended up needing to use this function in the pod reconciler, to check if a follower pod's nodeSelector topology value does not match the leader pod's (and check if the leader pod even exists / was killed), so that's why I placed it in this shared package. This logic was removed from this PR to simplify reviewing, but it will be in a subsequent PR.

Yeah naming this package is hard, but I do think we do need a package of shared functions used by both webhooks and controllers, to serve as a single source of truth and avoid code duplication of course.

util by definition is a pkg of "shared" functions, may be rename the pkg to placement to indicate that those functions are for exclusive placement.

you can do that in the followup PR

pkg/webhooks/pod_admission_webhook.go

pkg/webhooks/pod_mutating_webhook.go

ahg-g · 2023-11-22T20:30:47Z

/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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 26, 2023

k8s-ci-robot requested a review from ahg-g September 26, 2023 19:19

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 26, 2023

ahg-g reviewed Sep 26, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

danielvegamyhre force-pushed the scheduling branch from 4b21dc0 to 4151402 Compare September 26, 2023 20:11

ahg-g reviewed Sep 26, 2023

View reviewed changes

pkg/util/names/names.go Outdated Show resolved Hide resolved

danielvegamyhre force-pushed the scheduling branch from af40366 to 64e4c75 Compare September 26, 2023 21:32

danielvegamyhre changed the title ~~[WIP] Mutating webhook for new exclusive job placement strategy~~ [WIP] Migrate to "leader pod" exclusive job placement strategy Sep 26, 2023

danielvegamyhre changed the title ~~[WIP] Migrate to "leader pod" exclusive job placement strategy~~ Add mutating webhook and pod reconciler for new "leader pod" exclusive job placement strategy Sep 26, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 26, 2023

danielvegamyhre force-pushed the scheduling branch from 329f1b7 to 0150620 Compare September 26, 2023 22:11

danielvegamyhre commented Sep 26, 2023

View reviewed changes

pkg/controllers/pod_controller.go Outdated Show resolved Hide resolved

ahg-g reviewed Sep 26, 2023

View reviewed changes

danielvegamyhre force-pushed the scheduling branch 3 times, most recently from a76cbd1 to db2c6ec Compare September 26, 2023 23:17

ahg-g reviewed Sep 27, 2023

View reviewed changes

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

kannon92 reviewed Sep 27, 2023

View reviewed changes

ahg-g reviewed Sep 27, 2023

View reviewed changes

pkg/webhooks/pod_webhook.go Outdated Show resolved Hide resolved

kannon92 reviewed Sep 27, 2023

View reviewed changes

sdk/python/docs/JobsetV1alpha2PodAnnotator.md Outdated Show resolved Hide resolved

ahg-g reviewed Sep 27, 2023

View reviewed changes

pkg/controllers/pod_controller.go Outdated Show resolved Hide resolved

ahg-g reviewed Sep 27, 2023

View reviewed changes

pkg/controllers/pod_controller.go Outdated Show resolved Hide resolved

ahg-g reviewed Oct 4, 2023

View reviewed changes

pkg/webhooks/pod_mutating_webhook.go Outdated Show resolved Hide resolved

danielvegamyhre force-pushed the scheduling branch from 76860eb to 278d01a Compare October 4, 2023 23:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023

ahg-g reviewed Oct 5, 2023

View reviewed changes

pkg/webhooks/pod_admission_webhook.go Outdated Show resolved Hide resolved

pkg/webhooks/pod_mutating_webhook.go Show resolved Hide resolved

ahg-g reviewed Oct 5, 2023

View reviewed changes

pkg/webhooks/pod_mutating_webhook.go Outdated Show resolved Hide resolved

ahg-g reviewed Oct 5, 2023

View reviewed changes

pkg/webhooks/pod_admission_webhook.go Outdated Show resolved Hide resolved

danielvegamyhre force-pushed the scheduling branch 4 times, most recently from feed6dd to 1317dbd Compare November 18, 2023 01:04

danielvegamyhre changed the title ~~Add mutating webhook and pod reconciler for new "leader pod" exclusive job placement strategy~~ Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy Nov 18, 2023

danielvegamyhre force-pushed the scheduling branch 2 times, most recently from 8860b76 to 3c08157 Compare November 19, 2023 02:57

add pod webhook and pod reconciler for new exclusive placement strategy

f09783e

danielvegamyhre force-pushed the scheduling branch from 3c08157 to f09783e Compare November 19, 2023 03:24

moving pod reconciler logic to separate PR

2f474b0

danielvegamyhre force-pushed the scheduling branch from 7d61dba to 2f474b0 Compare November 20, 2023 20:44

remove unnecessary helper method in pod webhook

ddac87b

ahg-g reviewed Nov 22, 2023

View reviewed changes

address comments

7ef0e1d

danielvegamyhre force-pushed the scheduling branch from 2fe62ec to 7ef0e1d Compare November 22, 2023 20:13

k8s-ci-robot assigned ahg-g Nov 22, 2023

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

k8s-ci-robot merged commit e876d9d into kubernetes-sigs:main Nov 22, 2023
10 checks passed

danielvegamyhre mentioned this pull request Nov 27, 2023

Update pod reconciler to handle race conditions in exclusive placement #342

Merged

danielvegamyhre mentioned this pull request Dec 12, 2023

Release v0.3.0 #347

Closed

20 tasks

danielvegamyhre mentioned this pull request Jan 4, 2024

A different API to express colocation and exclusiveness #75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy #309

Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy #309

danielvegamyhre commented Sep 26, 2023 •

edited

Loading

k8s-ci-robot commented Sep 26, 2023

danielvegamyhre commented Sep 27, 2023

kannon92 Sep 27, 2023

vsoch Sep 27, 2023

ahg-g commented Oct 5, 2023

danielvegamyhre commented Nov 19, 2023

ahg-g Nov 22, 2023

danielvegamyhre Nov 22, 2023 •

edited

Loading

ahg-g Nov 22, 2023

ahg-g Nov 22, 2023

ahg-g commented Nov 22, 2023

Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy #309

Add pod webhook + reconciler for new "leader pod" exclusive job placement strategy #309

Conversation

danielvegamyhre commented Sep 26, 2023 • edited Loading

k8s-ci-robot commented Sep 26, 2023

danielvegamyhre commented Sep 27, 2023

kannon92 Sep 27, 2023

Choose a reason for hiding this comment

vsoch Sep 27, 2023

Choose a reason for hiding this comment

ahg-g commented Oct 5, 2023

danielvegamyhre commented Nov 19, 2023

ahg-g Nov 22, 2023

Choose a reason for hiding this comment

danielvegamyhre Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

ahg-g Nov 22, 2023

Choose a reason for hiding this comment

ahg-g Nov 22, 2023

Choose a reason for hiding this comment

ahg-g commented Nov 22, 2023

danielvegamyhre commented Sep 26, 2023 •

edited

Loading

danielvegamyhre Nov 22, 2023 •

edited

Loading