Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add examples for three existing failure policy actions. #601

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add examples for three existing failure policy actions. #601

wants to merge 9 commits into from
+397 −0

Conversation

jedwins1998
Copy link
Contributor

Add examples for each of the following failure policy actions:

  1. FailJobSet,
  2. RestartJobSet,
  3. RestartJobSetAndIgnoreMaxRestarts.

Fixes #600.

@k8s-ci-robot 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 Jun 10, 2024
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 10, 2024
@netlify Netlify
Copy link

netlify bot commented Jun 10, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name Link
🔨 Latest commit d097262
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/66e1adb57580330007cc62ff

@danielvegamyhre
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 Jun 10, 2024
@jedwins1998 jedwins1998 force-pushed the adding-configurable-failure-policy-examples branch from 37f5a92 to c08778c Compare June 13, 2024 22:06
@jedwins1998
Add examples for three existing failure policy actions.
fd29fa0 
Add examples for each of the following failure policy actions:
1. FailJobSet,
2. RestartJobSet,
3. RestartJobSetAndIgnoreMaxRestarts.
@jedwins1998
Add example for configurable failure policy using a rule with onJobFa…
f19acf0 
…ilureReasons present.
@jedwins1998
Correct the name of the jobset in 'examples/failure-policy/onjobfailu…
13bc476 
…rereasons-present.yaml'.
@jedwins1998
Add example using onJobFailureReasons with the selected reason being …
dad8964 
…PodFailurePolicy.
@jedwins1998 jedwins1998 force-pushed the adding-configurable-failure-policy-examples branch from 721c42f to dad8964 Compare June 25, 2024 21:30
@danielvegamyhre danielvegamyhre self-assigned this Jul 1, 2024
danielvegamyhre
danielvegamyhre reviewed Jul 2, 2024
View reviewed changes
examples/failure-policy/failjobset-action.yaml
metadata:
name: failjobset-action-example
spec:
failurePolicy:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a brief comment to the failurePolicy section of each example describing what behavior the user should expect to see when running the example?

examples/failure-policy/restartjobsetandignoremaxrestarts-action.yaml Show resolved Hide resolved
@googs1025 googs1025 mentioned this pull request Jul 27, 2024
Open
@danielvegamyhre danielvegamyhre added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 10, 2024
@ahg-g
Copy link
Contributor

ahg-g commented Aug 31, 2024

Is this ready to merge?

@danielvegamyhre
Copy link
Contributor

Is this ready to merge?

As of last week, Giuseppe (AI Infra team in GKE) was taking over this PR.

@jedwins1998
Copy link
Contributor Author

Is this ready to merge?

As of last week, Giuseppe (AI Infra team in GKE) was taking over this PR.

I spoke with Giuseppe and I'll finish up this PR since I started it.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jedwins1998
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:

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

@jedwins1998
Copy link
Contributor Author

Is this ready to merge?

As of last week, Giuseppe (AI Infra team in GKE) was taking over this PR.

I spoke with Giuseppe and I'll finish up this PR since I started it.

I added an example similar to a host maintenance event. I also added short descriptions of the expected behavior in each example. I consider this PR ready to merge now.

kannon92
kannon92 reviewed Sep 5, 2024
View reviewed changes
examples/failure-policy/host-maintenance-event-model.yaml
- action: RestartJobSetAndIgnoreMaxRestarts
onJobFailureReasons:
- PodFailurePolicy
# The JobSet is restarted as normal when the leader job fails and the above rule is not matched.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for this to restart with maxRestarts 0? It would fail right away right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct. It would fail right away.

kannon92
kannon92 reviewed Sep 5, 2024
View reviewed changes
examples/failure-policy/onjobfailurereasons-present.yaml Outdated
failurePolicy:
maxRestarts: 3
rules:
# The JobSet will restart and unlimited number of times when the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# The JobSet will restart and unlimited number of times when the
# The JobSet will restart an unlimited number of times when the

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

danielvegamyhre
danielvegamyhre reviewed Sep 8, 2024
View reviewed changes
examples/failure-policy/host-maintenance-event-model.yaml
echo "$i"
sleep 1
done
podFailurePolicy:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here explaining this pod failure policy will trigger on host maintenance events when pods are evicted from the affected nodes, thus failing with a condition type of DisruptionTarget?

Pod failure policy is a fairly new, advanced Job API feature that many users won't be familiar with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

examples/failure-policy/restartjobsetandignoremaxrestarts-action.yaml Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kannon92 kannon92 kannon92 left review comments

@googs1025 googs1025 googs1025 left review comments

@danielvegamyhre danielvegamyhre danielvegamyhre left review comments

Assignees

@danielvegamyhre danielvegamyhre

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Examples for Failure Policy Actions
6 participants
@jedwins1998 @k8s-ci-robot @danielvegamyhre @ahg-g @kannon92 @googs1025