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.

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

fix: tfjob with restartPolicy=ExitCode not work #1562

Conversation

cheimu
Copy link
Member

@cheimu cheimu commented Mar 23, 2022

What this PR does / why we need it:
As describe in issues linked below, the tfjob with restartPolicy=ExitCode won’t work. If workers/evaluators got exited, the whole job will be set to failed instead of restarting, which is not expected.

Need for more comments! Thank you in advance! :D

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1560, kubeflow/common#186

Checklist:

  • Docs included if any changes are user facing

@aws-kf-ci-bot
Copy link
Contributor

Hi @cheimu. Thanks for your PR.

I'm waiting for a kubeflow 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.

@cheimu cheimu changed the title fix tfjob with restartPolicy=exitCode not work fix: tfjob with restartPolicy=ExitCode not work Mar 23, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2030402854

  • 14 of 16 (87.5%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 37.061%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controller.v1/tensorflow/tfjob_controller.go 14 16 87.5%
Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/tensorflow/tfjob_controller.go 1 71.04%
Totals Coverage Status
Change from base Build 2014597889: 0.2%
Covered Lines: 2290
Relevant Lines: 6179

💛 - Coveralls

@richardsliu
Copy link
Contributor

/ok-to-test

// the restarting condition will be removed from jobStatus by commonv1.filterOutCondition(),
// so we need to append the restarting condition back to jobStatus.
if existingRestartingCondition != nil {
err := commonutil.UpdateJobConditions(jobStatus, commonv1.JobRestarting, existingRestartingCondition.Reason, existingRestartingCondition.Message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we reset the value of existingRestartingCondition after this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @richardsliu, I don't know if we could reset it. I record the existingRestartingCondition outside the loop, which is the condition and result for current reconcile(). Inside the loop, when iterate over all pods, if there are multiple workers or masters .etc, are running, and only one worker failed, by the logic inside of updateJobConditions(), restarting + running combination will make running condition overwrite restarting.

Copy link

Choose a reason for hiding this comment

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

@richardsliu is this the right way to deal with it? If one of the pods is restarting, do we mark the whole job as restarting ?

Copy link

Choose a reason for hiding this comment

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

If we do something along the lines of, would it achieve the same thing?

if condition.Type == commonv1.JobRunning || condition.Type == commonv1.JobRestarting {
  restart = true

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @pavanky, I would like to hear more advices: ) For now, I just follow the current convention. See https://github.com/kubeflow/training-operator/blob/master/pkg/controller.v1/tensorflow/tfjob_controller.go#L762 . Current strategy is that if one of the pods failed, then update the whole job status to failed : /

Copy link

@pavanky pavanky Mar 25, 2022

Choose a reason for hiding this comment

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

Wouldn't that technically mean we are updating the job status to restart and incrementing the count two times for the same failure ?

Copy link
Member Author

@cheimu cheimu Mar 25, 2022

Choose a reason for hiding this comment

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

IMO, we could see it from 2 points of views.

  1. If jobStatus is running, and one of roles got failed pods, then the whole job will be updated to failed, then in next reconcile, the pods will be deleted, so jobstatus should have a restarting (don't know if I understand the code base correctly, we don't have specific restarting logic but use general reconcile logic)
  2. Technically we just set jobstatus's field twice without sending request to apiServer, so in fact, it didn't get really updated. But yeah, current one is not elegant. 🤔

@cheimu
Copy link
Member Author

cheimu commented Mar 24, 2022

/retest

@gaocegege
Copy link
Member

cc @rllin @pavanky

@richardsliu
Copy link
Contributor

/lgtm

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

LGTM 👍
/lgtm

@cheimu
Copy link
Member Author

cheimu commented May 5, 2022

Hi @richardsliu , what should we do next for this pr/issue?

@emenendez
Copy link

Hi all! Just wanted to ask if there is intent to merge this PR -- our users are definitely impacted by this issue and we'd really benefit from the fix. Thanks so much!

@gaocegege
Copy link
Member

We get two LGTMs here. I think we should merge it.

Sorry for the late reply.

/approve.

@gaocegege
Copy link
Member

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheimu, gaocegege

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:

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

@google-oss-prow google-oss-prow bot merged commit 9cc1cc9 into kubeflow:master Jul 25, 2022
@emenendez
Copy link

Thanks so much for the speedy merge!

djwhatle pushed a commit to djwhatle/training-operator that referenced this pull request Jul 26, 2022
djwhatle added a commit to djwhatle/training-operator that referenced this pull request Jul 26, 2022
…le-cherrypick-exitcode-fix

fix: tfjob with restartPolicy=ExitCode not work (kubeflow#1562)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When restartPolicy is ExitCode and a pod is deleted (137), the entire TFJob will still be marked as failed.
7 participants