fix restart policy bug in mpi job UpdateJobConditions #2344
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Pull Request Test Coverage Report for Build 12176368662Details
💛 - Coveralls |
There was a problem hiding this comment.
I was not sure if this is really bugs.
Could you add tests to prove the bug situations?
https://github.com/kubeflow/training-operator/blob/master/pkg/controller.v1/mpi/mpijob_controller_test.go
| @@ -597,7 +597,7 @@ func (jc *MPIJobReconciler) UpdateJobStatus(job interface{}, replicas map[kubefl | |||
| } | |||
| } | |||
| if failed > 0 { | |||
| if spec.RestartPolicy == kubeflowv1.RestartPolicyExitCode { | |||
| if spec.RestartPolicy != kubeflowv1.RestartPolicyNever { | |||
There was a problem hiding this comment.
@fyxemmmm Sorry for the late reply, please can you submit this fix to the release-1.9 branch?
There was a problem hiding this comment.
@tenzen-y I think, it is a bug, here is similar code in PyTorchJob controller: https://github.com/kubeflow/trainer/blob/release-1.9/pkg/controller.v1/pytorch/pytorchjob_controller.go#L429
There was a problem hiding this comment.
I do not think MPI v1 equals PyTorch since MPI does not handle Job RestartPolicy directly:
There was a problem hiding this comment.
Oh, I see. Shouldn't we set it as follows then:
if spec.RestartPolicy != kubeflowv1.RestartPolicyNever && if spec.RestartPolicy != kubeflowv1.RestartPolicyExitCodeThere was a problem hiding this comment.
I'm not sure if the conditional checking works well because MPI v1 doesn't use Generic PodReconciler. MPI v1 has a dedicated Pod Reconciler.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
What this PR does / why we need it:
This PR fixes an issue in the MPIJob controller logic related to the handling of failed replicas. 8000 Specifically, the condition for determining the restart policy was incorrect.
Previously, the code used:
if spec.RestartPolicy == kubeflowv1.RestartPolicyExitCode
This condition was problematic because it failed to handle cases where the restart policy should be based on other valid configurations. The incorrect logic could lead to unexpected behavior, such as jobs failing instead of restarting.
The updated code uses:
if spec.RestartPolicy != kubeflowv1.RestartPolicyNever
This ensures that jobs are restarted appropriately unless explicitly configured not to restart, aligning the behavior with the intended design and user expectations.