Add restart strategy #686

nstogner · 2024-10-18T18:33:27Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #684

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add configurable restart strategy.

k8s-ci-robot · 2024-10-18T18:33:36Z

Welcome @nstogner!

It looks like this is your first PR to kubernetes-sigs/jobset 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/jobset has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-18T18:33:36Z

Hi @nstogner. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-10-18T18:33:43Z

✅ Deploy Preview for kubernetes-sigs-jobset ready!

Name	Link
🔨 Latest commit	`5dce90e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/67192d956b1ce30007cb8800
😎 Deploy Preview	https://deploy-preview-686--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.

ahg-g · 2024-10-18T18:36:18Z

/ok-to-test

ahg-g · 2024-10-18T20:52:11Z

api/jobset/v1alpha2/jobset_types.go

+	// RestartStrategy defines the strategy to use when restarting the JobSet.
+	// Defaults to Recreate.
+	// +optional
+	// +kubebuilder:default=Recreate


Can we do validation here as follows:

// +kubebuilder:validation:Enum:=

Also, if the user was running a jobset and at the same time upgraded the controller, I suspect that this field will not exist for this job. I think this needs to be a reference that we explicitly default in the webhook and in the controller code (default the field if not set) to ensure we don't break running jobsets.

Added the enum validation. Here is the behavior as-is when the CRD is updated (default is applied when JobSet is read after update of CRD):

(base) ➜ jobset.blocking-recreate git:(restart-strategy) kind create cluster using podman due to KIND_EXPERIMENTAL_PROVIDER enabling experimental podman provider Creating cluster "kind" ... ✓ Ensuring node image (kindest/node:v1.29.2) 🖼 ✓ Preparing nodes 📦 ✓ Writing configuration 📜 ✓ Starting control-plane 🕹️ ✓ Installing CNI 🔌 ✓ Installing StorageClass 💾 Set kubectl context to "kind-kind" You can now use your cluster with: kubectl cluster-info --context kind-kind Have a nice day! 👋 (base) ➜ jobset.blocking-recreate git:(restart-strategy) git checkout main Switched to branch 'main' Your branch is up to date with 'origin/main'. (base) ➜ jobset.blocking-recreate git:(main) kubectl apply --server-side -k ./config/components/crd # Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically. customresourcedefinition.apiextensions.k8s.io/jobsets.jobset.x-k8s.io serverside-applied (base) ➜ jobset.blocking-recreate git:(main) kubectl apply -f ./examples/simple/max-restarts.yaml jobset.jobset.x-k8s.io/max-restarts created (base) ➜ jobset.blocking-recreate git:(main) kubectl get jobsets -oyaml | grep -A 3 failurePolicy failurePolicy: maxRestarts: 3 replicatedJobs: - name: leader (base) ➜ jobset.blocking-recreate git:(main) git checkout - Switched to branch 'restart-strategy' Your branch is up to date with 'nstogner/restart-strategy'. (base) ➜ jobset.blocking-recreate git:(restart-strategy) kubectl apply --server-side -k ./config/components/crd # Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically. customresourcedefinition.apiextensions.k8s.io/jobsets.jobset.x-k8s.io serverside-applied (base) ➜ jobset.blocking-recreate git:(restart-strategy) kubectl get jobsets -oyaml | grep -A 3 failurePolicy failurePolicy: maxRestarts: 3 restartStrategy: Recreate # <----- Default is applied replicatedJobs:

Seems like this will not break existing JobSets... WDYT?

sg, can you add the validation, it is not on the last commit.

oh, I see you added it on the enum definition itself, make sense

danielvegamyhre

Looks like a useful change, the idea is to resolve the issue described here correct?

Added a few comments.

danielvegamyhre · 2024-10-19T15:37:06Z

pkg/controllers/jobset_controller.go

+	// If the JobSet is using the BlockingRecreate failure policy, we should not create any new jobs until
+	// all the jobs slated for deletion (i.e. from the last restart index) have been deleted.
+	if len(ownedJobs.previous) > 0 &&
+		js.Spec.FailurePolicy != nil && js.Spec.FailurePolicy.RestartStrategy == jobset.BlockingRecreate {


nit: can we store js.Spec.FailurePolicy != nil && js.Spec.FailurePolicy.RestartStrategy == jobset.BlockingRecreate in a variable to make this big if-statement more concise and readable?

danielvegamyhre · 2024-10-19T15:43:15Z

test/integration/controller/jobset_controller_test.go

+				},
+			},
+		}),
+		// TODO(nstogner): Add test case for BlockingRecreate!!!


We should add this test case for blockingRecreate before merging

danielvegamyhre · 2024-10-19T15:43:48Z

test/integration/controller/jobset_controller_test.go

+				{
+					checkJobSetState: func(js *jobset.JobSet) {
+						ginkgo.By("checking 3/4 jobs were recreated")
+						exp := map[int]int{


Can we rename exp to something a little more descriptive which makes it clear what the keys and values represent, so people reading the code don't need to reverse-engineer it by finding where/how it's used, etc.

nstogner · 2024-10-22T18:58:32Z

Looks like a useful change, the idea is to resolve the issue described here correct?

Added a few comments.

Yep, thats exactly what this is targeting.

danielvegamyhre · 2024-10-22T21:26:52Z

@nstogner you probably need to run make generate to fix the CI error

ahg-g · 2024-10-23T17:31:37Z

/lgtm
/approve

Great, thanks!

k8s-ci-robot · 2024-10-23T17:31:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, nstogner

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~~ [ahg-g]

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 18, 2024

k8s-ci-robot requested review from ahg-g and kannon92 October 18, 2024 18:33

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2024

nstogner changed the title ~~WIP: Add restart strategy to API~~ WIP: Add restart strategy Oct 18, 2024

ahg-g reviewed Oct 18, 2024

View reviewed changes

danielvegamyhre reviewed Oct 19, 2024

View reviewed changes

nstogner changed the title ~~WIP: Add restart strategy~~ Add restart strategy Oct 22, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024

nstogner requested review from ahg-g and danielvegamyhre October 22, 2024 18:46

nstogner added 5 commits October 22, 2024 17:03

Add restart strategy to API

2647152

Remove webhook, replace with openapi defaulting

cb9e85c

Update creation logic to account for BlockingRecreate

a8da08c

Update integration test for existing Recreate strategy

61e305e

Fix integration tests

d6a6aaa

nstogner added 3 commits October 22, 2024 17:03

Document restartStrategy by reusing existing max-restarts example

b1f5b14

Address comment

d24958f

Add enum validation

55c8787

nstogner force-pushed the restart-strategy branch from 61d63ee to 55c8787 Compare October 22, 2024 21:04

make generate

fe225a5

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 Oct 23, 2024

Fix issues with make generate

afa4e9a

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 Oct 23, 2024

Fix example

5dce90e

k8s-ci-robot assigned ahg-g Oct 23, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2024

k8s-ci-robot merged commit f76f2a7 into kubernetes-sigs:main Oct 23, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restart strategy #686

Add restart strategy #686

nstogner commented Oct 18, 2024 •

edited

Loading

k8s-ci-robot commented Oct 18, 2024

k8s-ci-robot commented Oct 18, 2024

netlify bot commented Oct 18, 2024 •

edited

Loading

ahg-g commented Oct 18, 2024

ahg-g Oct 18, 2024 •

edited

Loading

nstogner Oct 22, 2024 •

edited

Loading

nstogner Oct 22, 2024

ahg-g Oct 23, 2024

ahg-g Oct 23, 2024

danielvegamyhre left a comment

danielvegamyhre Oct 19, 2024

nstogner Oct 22, 2024

danielvegamyhre Oct 19, 2024

nstogner Oct 22, 2024

danielvegamyhre Oct 19, 2024

nstogner Oct 22, 2024

nstogner commented Oct 22, 2024

danielvegamyhre commented Oct 22, 2024

ahg-g commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

Add restart strategy #686

Add restart strategy #686

Conversation

nstogner commented Oct 18, 2024 • edited Loading

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?

k8s-ci-robot commented Oct 18, 2024

k8s-ci-robot commented Oct 18, 2024

netlify bot commented Oct 18, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset ready!

ahg-g commented Oct 18, 2024

ahg-g Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

nstogner Oct 22, 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

danielvegamyhre 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstogner commented Oct 22, 2024

danielvegamyhre commented Oct 22, 2024

ahg-g commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

nstogner commented Oct 18, 2024 •

edited

Loading

netlify bot commented Oct 18, 2024 •

edited

Loading

ahg-g Oct 18, 2024 •

edited

Loading

nstogner Oct 22, 2024 •

edited

Loading