Implement configurable failure policy. #537

jedwins1998 · 2024-04-19T22:50:19Z

This pull request is to implement configurable failure policy.

There is one difference to note from the KEP. I added a new field to the JobSetStatus that tracks the number of restarts which count towards the restart limit. I then use this variable to allow some restarts to not count towards the maximum number of restarts.

This resolves #262.

k8s-ci-robot · 2024-04-19T22:50:28Z

Hi @jedwins1998. 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/test-infra repository.

netlify · 2024-04-19T22:50:38Z

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	`6a1ac21`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/664bd459b5d8bd00088bff49

jedwins1998 · 2024-04-19T22:52:18Z

There is one TODO left for an additional test I would like to add and I still need to implement Webhook validation for OnJobFailureReasons. Besides that, I consider the code ready for review.

danielvegamyhre · 2024-04-20T00:02:15Z

/ok-to-test

danielvegamyhre

Can we move all the helpers in jobset_controller.go specific to failure policies to a failure_policy.go file, and add unit tests for any important ones in failure_policy_test.go? Same as success_policy.go and success_policy_test.go.

I'll take a deeper look next week. Thanks for working on this!

jedwins1998 · 2024-04-22T18:46:05Z

Can we move all the helpers in jobset_controller.go specific to failure policies to a failure_policy.go file, and add unit tests for any important ones in failure_policy_test.go? Same as success_policy.go and success_policy_test.go.

I'll take a deeper look next week. Thanks for working on this!

Can do.

api/jobset/v1alpha2/jobset_types.go

danielvegamyhre

Did a quick pass while you are working on the refactor, looks good so far!

pkg/controllers/jobset_controller.go

pkg/webhooks/jobset_webhook.go

jedwins1998 · 2024-04-26T21:32:15Z

Can we move all the helpers in jobset_controller.go specific to failure policies to a failure_policy.go file, and add unit tests for any important ones in failure_policy_test.go? Same as success_policy.go and success_policy_test.go.

I'll take a deeper look next week. Thanks for working on this!

This is now done.

ahg-g

I didn't look at the integration tests yet

api/jobset/v1alpha2/jobset_types.go

pkg/controllers/jobset_controller.go

pkg/controllers/failure_policy.go

pkg/webhooks/jobset_webhook.go

ahg-g · 2024-04-30T06:51:01Z

pls don't amend the commits, it makes it hard to review because we can't tell the diff

jedwins1998 · 2024-04-30T18:02:52Z

pls don't amend the commits, it makes it hard to review because we can't tell the diff

@ahg-g, there is a "Compare" button next to each amended commit to see the diff. Does that not do what you are looking for or are you referring to something else?

In addition, I am using amend as I am trying to avoid what happened when I merged pull request 487 and all the individual commits were included.

ahg-g · 2024-05-01T19:27:46Z

pls don't amend the commits, it makes it hard to review because we can't tell the diff

@ahg-g, there is a "Compare" button next to each amended commit to see the diff. Does that not do what you are looking for or are you referring to something else?

It is hard to find, more importantly it doesn't allow the reviewer to add comments while comparing the two diffs.

In addition, I am using amend as I am trying to avoid what happened when I merged pull request 487 and all the individual commits were included.

We can squash at the end of the review.

jedwins1998 · 2024-05-01T20:23:50Z

pls don't amend the commits, it makes it hard to review because we can't tell the diff

@ahg-g, there is a "Compare" button next to each amended commit to see the diff. Does that not do what you are looking for or are you referring to something else?

It is hard to find, more importantly it doesn't allow the reviewer to add comments while comparing the two diffs.

In addition, I am using amend as I am trying to avoid what happened when I merged pull request 487 and all the individual commits were included.

We can squash at the end of the review.

Is it possible to denote/mark the PR as intended as a squash merge? I would like to do it now so there is not a chance of forgetting later.

@danielvegamyhre had mentioned I can use a squash label, but I do not see a way to apply it. I tried following the instructions at [1], but do not see anything called Labels that I can click as suggested in the instructions.

[1] https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels#applying-a-label

ahg-g · 2024-05-01T20:27:31Z

simply hold the PR, and once you get the all clear from the reviewers, send a squashed commit and cancel the hold.

/hold

ahg-g

Thanks

pkg/controllers/jobset_controller.go

pkg/controllers/failure_policy.go

pkg/controllers/jobset_controller.go

danielvegamyhre

Reviewed the tests

pkg/controllers/failure_policy.go

pkg/controllers/jobset_controller_test.go

pkg/webhooks/jobset_webhook_test.go

pkg/controllers/failure_policy_test.go

test/integration/controller/jobset_controller_test.go

…indFirstFailedPolicyRuleAndJob.

…nditionAndOpts.

…to be the first failure policy rule test.

…case names more clear.

I added `[failure policy]` to the begin of the name of each test related to failure policies so that it is easier to select only those tests to run. I also updated tests to check that `RestartsCountTowardsMax` is incrementing only when expected.

danielvegamyhre

LGTM after a few final comments are addressed. In a follow-up PR, we should add an example JobSet spec to the examples/ folder showcasing how the feature works, which we can use to do manual testing as well.

pkg/constants/constants.go

pkg/controllers/failure_policy.go

pkg/webhooks/jobset_webhook.go

pkg/controllers/failure_policy.go

danielvegamyhre · 2024-05-17T23:55:39Z

api/jobset/v1alpha2/jobset_types.go

+	RestartJobSetAndIgnoreMaxRestarts FailurePolicyAction = "RestartJobSetAndIgnoreMaxRestarts"
+)
+
+// FailurePolicyRule defines a FailurePolicyAction to be executed if a child job


(This is a general comment unrelated to this file) Now that feature gate support has been merged in #557 I think we should add a feature gate (default on) for this feature. If the feature is not enabled, fall back to the current behavior.

We can do this in a follow up PR.

I want to avoid a scenario where we publish the v0.6.0 release and an important customer is using this feature, then they encounter a bug that slipped through the cracks, and we can't simply downgrade to v0.5.0 to mitigate because their JobSet spec (often defined in Python/Go code checked into their codebase) is using fields which only exist in v0.6.0 - thus requiring some emergency rollout on their end to revert their Python/Go code to a spec usable by JobSet v0.5.0, and then downgrade JobSet deployment to v0.5.0.

pkg/controllers/jobset_controller_test.go

pkg/controllers/failure_policy.go

test/integration/controller/jobset_controller_test.go

…onReason value.

danielvegamyhre · 2024-05-20T23:31:40Z

/lgtm

Thanks for working on this! Will leave approval for @ahg-g

ahg-g · 2024-05-20T23:42:39Z

/approve

k8s-ci-robot · 2024-05-20T23:42:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ahg-g · 2024-05-21T01:14:06Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 19, 2024

k8s-ci-robot requested review from ahg-g and kannon92 April 19, 2024 22:50

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 19, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 20, 2024

danielvegamyhre reviewed Apr 20, 2024

View reviewed changes

kannon92 reviewed Apr 22, 2024

View reviewed changes

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

danielvegamyhre reviewed Apr 25, 2024

View reviewed changes

danielvegamyhre reviewed Apr 26, 2024

View reviewed changes

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

danielvegamyhre reviewed Apr 26, 2024

View reviewed changes

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

ahg-g reviewed Apr 28, 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 May 1, 2024

ahg-g reviewed May 5, 2024

View reviewed changes

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

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

danielvegamyhre reviewed May 6, 2024

View reviewed changes

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

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

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

danielvegamyhre reviewed May 7, 2024

View reviewed changes

Justin Edwins added 2 commits May 17, 2024 17:53

Implement configurable failure policy.

f038a33

Refactor findFirstFailedPolicyRuleAndJob, ruleIsApplicable, and TestF…

a3cecb1

…indFirstFailedPolicyRuleAndJob.

Justin Edwins added 9 commits May 17, 2024 17:53

Create function parseFailJobOpts to remove closure in jobWithFailedCo…

9af5c06

…nditionAndOpts.

Refactor TestJobSetDefaulting.

ac8c3ea

Refactor TestValidateCreate.

e91b6b0

Add logging when a matching failure policy rule is found.

4de124b

In TestValidateCreate, move the test for a valid failure policy rule …

c5ae410

…to be the first failure policy rule test.

Add comment defining TestValidateCreate.

9f0d08b

Refactor TestFindFirstFailedPolicyRuleAndJob to make individual test …

5b8f55d

…case names more clear.

Add check for value of .status.restarts field.

a8354ff

jedwins1998 requested review from kannon92, danielvegamyhre and ahg-g May 17, 2024 20:45

danielvegamyhre reviewed May 19, 2024

View reviewed changes

Justin Edwins added 6 commits May 20, 2024 18:23

Remove 'Reason' from the end of RestartJobSetAndIgnoreMaxRestartsActi…

bf03055

…onReason value.

Change the ordering of functions in failure_policy.go.

715777a

Remove parentheses around '[]int'.

3fee011

Rename 'failurePolicyRule' to 'matchingFailurePolicyRule'.

92fe2f7

Combine function function call and error check into one line.

6bd209f

Change 'gomega.BeComparableTo' to 'gomega.Equal'.

6a1ac21

k8s-ci-robot assigned danielvegamyhre May 20, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 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 May 21, 2024

k8s-ci-robot merged commit ac8cd93 into kubernetes-sigs:main May 21, 2024
12 checks passed

danielvegamyhre mentioned this pull request May 24, 2024

Relax validation rules enforcing immutability of replicated jobs #579

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

Implement configurable failure policy. #537

Implement configurable failure policy. #537

jedwins1998 commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

netlify bot commented Apr 19, 2024 •

edited

Loading

jedwins1998 commented Apr 19, 2024 •

edited

Loading

danielvegamyhre commented Apr 20, 2024

danielvegamyhre left a comment •

edited

Loading

jedwins1998 commented Apr 22, 2024

danielvegamyhre left a comment

jedwins1998 commented Apr 26, 2024 •

edited

Loading

ahg-g left a comment

ahg-g commented Apr 30, 2024

jedwins1998 commented Apr 30, 2024

ahg-g commented May 1, 2024

jedwins1998 commented May 1, 2024

ahg-g commented May 1, 2024

ahg-g left a comment

danielvegamyhre left a comment

danielvegamyhre left a comment •

edited

Loading

danielvegamyhre May 17, 2024 •

edited

Loading

danielvegamyhre commented May 20, 2024

ahg-g commented May 20, 2024

k8s-ci-robot commented May 20, 2024

ahg-g commented May 21, 2024

Implement configurable failure policy. #537

Implement configurable failure policy. #537

Conversation

jedwins1998 commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

netlify bot commented Apr 19, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

jedwins1998 commented Apr 19, 2024 • edited Loading

danielvegamyhre commented Apr 20, 2024

danielvegamyhre left a comment • edited Loading

Choose a reason for hiding this comment

jedwins1998 commented Apr 22, 2024

danielvegamyhre left a comment

Choose a reason for hiding this comment

jedwins1998 commented Apr 26, 2024 • edited Loading

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g commented Apr 30, 2024

jedwins1998 commented Apr 30, 2024

ahg-g commented May 1, 2024

jedwins1998 commented May 1, 2024

ahg-g commented May 1, 2024

ahg-g left a comment

Choose a reason for hiding this comment

danielvegamyhre left a comment

Choose a reason for hiding this comment

danielvegamyhre left a comment • edited Loading

Choose a reason for hiding this comment

danielvegamyhre May 17, 2024 • edited Loading

Choose a reason for hiding this comment

danielvegamyhre commented May 20, 2024

ahg-g commented May 20, 2024

k8s-ci-robot commented May 20, 2024

ahg-g commented May 21, 2024

netlify bot commented Apr 19, 2024 •

edited

Loading

jedwins1998 commented Apr 19, 2024 •

edited

Loading

danielvegamyhre left a comment •

edited

Loading

jedwins1998 commented Apr 26, 2024 •

edited

Loading

danielvegamyhre left a comment •

edited

Loading

danielvegamyhre May 17, 2024 •

edited

Loading