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

Fix: All pods is existing when restart count exceed max retry #1719

Merged
merged 1 commit into from Nov 24, 2021
Merged

Fix: All pods is existing when restart count exceed max retry #1719

merged 1 commit into from Nov 24, 2021

Conversation

LuBingtan
Copy link
Contributor

Fix: #1718

@volcano-sh-bot
Copy link
Contributor

Welcome @LuBingtan!

It looks like this is your first PR to volcano-sh/volcano 馃帀.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 4, 2021
@LuBingtan
Copy link
Contributor Author

/assign @shinytang6

@LuBingtan LuBingtan force-pushed the fix/delete_running_pods_on_last_retry branch from 2e98a97 to 91a26be Compare September 4, 2021 04:40
@Thor-wl Thor-wl requested review from jasonliu747, shinytang6 and william-wang and removed request for hzxuzhonghu and zen-xu September 4, 2021 06:39
@k82cn
Copy link
Member

k82cn commented Sep 7, 2021

/cc @wpeng102

@Thor-wl
Copy link
Member

Thor-wl commented Sep 7, 2021

I think this issue should be discussed in the community. I'll take it in the weekly meeting.

@Thor-wl
Copy link
Member

Thor-wl commented Sep 13, 2021 •
edited

When I tried to reproduce the phenomenon mentioning at #1736, I found that the mpi master was compeleted but workers were still running when maxRetry is 1.

@Thor-wl Thor-wl mentioned this pull request Sep 13, 2021
Closed
@william-wang
Copy link
Member

@LuBingtan Thanks for your pr. Is there e2e case to track this case? If not, please add the e2e or add your testing results to make sure it playing well with the case in pr 1657.

@Thor-wl
Copy link
Member

Thor-wl commented Sep 13, 2021

/cc @wpeng102 Please take a look.

@LuBingtan
Copy link
Contributor Author

@LuBingtan Thanks for your pr. Is there e2e case to track this case? If not, please add the e2e or add your testing results to make sure it playing well with the case in pr 1657.

Ok, I will add an e2e testing case

@igormishsky
Copy link

Can someone see why it is not merged?
We also have this issue

@jasonliu747
Copy link
Member

jasonliu747 commented Nov 2, 2021 •
edited

Can someone see why it is not merged? We also have this issue

Waiting for more test results.
@LuBingtan Any updates?

@volcano-sh-bot volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 3, 2021
@LuBingtan LuBingtan force-pushed the fix/delete_running_pods_on_last_retry branch from a2865ec to 2bea7b9 Compare November 3, 2021 09:04
@volcano-sh-bot volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2021
@LuBingtan LuBingtan force-pushed the fix/delete_running_pods_on_last_retry branch from 2bea7b9 to 1c87728 Compare November 3, 2021 09:05
@LuBingtan
Fix kill job, do not delete failed/succeeded pods at last retry, add …
083f2f9 
…e2e test for job retry

Signed-off-by: lubingtan <lubingtan@126.com>
@LuBingtan LuBingtan force-pushed the fix/delete_running_pods_on_last_retry branch from 1c87728 to 083f2f9 Compare November 3, 2021 09:20
@LuBingtan
Copy link
Contributor Author

@jasonliu747 @william-wang @Thor-wl

Hi, I have updated this pr with an e2e test for job retry.
Please take a look

@Thor-wl
Copy link
Member

Thor-wl commented Nov 4, 2021

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@LuBingtan
Copy link
Contributor Author

@jasonliu747 @william-wang @Thor-wl
Hi, is there any suggestions? I wander how this PR could be merged?

@Thor-wl
Copy link
Member

Thor-wl commented Nov 22, 2021

I'm OK about current PR.
/cc @shinytang6 @william-wang @jasonliu747 Pls take a review.

jasonliu747
jasonliu747 reviewed Nov 22, 2021
View reviewed changes
Copy link
Member

@jasonliu747 jasonliu747 left a comment

Choose a reason for hiding this comment

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

/lgtm

@LuBingtan
Copy link
Contributor Author

@shinytang6 @william-wang
Hi~ Could you help me to take a look ?

william-wang
william-wang approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2021
@volcano-sh-bot volcano-sh-bot merged commit c2765de into volcano-sh:master Nov 24, 2021
@kye308 kye308 mentioned this pull request Jan 27, 2022
Closed
@william-wang william-wang mentioned this pull request Jan 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonliu747 jasonliu747 jasonliu747 left review comments

@william-wang william-wang william-wang approved these changes

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@wpeng102 wpeng102 Awaiting requested review from wpeng102

Assignees

@william-wang william-wang

@shinytang6 shinytang6

@jasonliu747 jasonliu747

@Thor-wl Thor-wl

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

All pods is existing when restart count exceed max retry
8 participants
@LuBingtan @volcano-sh-bot @k82cn @Thor-wl @william-wang @igormishsky @jasonliu747 @shinytang6