-
Notifications
You must be signed in to change notification settings - Fork 51
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
Rename JobSet status to Complete #723
Comments
it makes sense to me, I am willing to make this change. 🤔 |
I tend to prefer "Succeeded" as it matches terminal states like 'Suspended' and 'Failed'. I do think it unfortunate that Job uses complete. I don't really have a strong feeling about the renaming but I think this should be considered a breaking change so we should probably introduce "Complete" and leave "Succeeded" as deprecated. And then maybe in a release or two we can drop "Succeeded". But I don't really feel that this renaming is worth the effort. |
We discussed before with @tenzen-y, that it is tricky to define what does Succeeded mean for Job or JobSet given all of the Success and Failure policies that we have: kubeflow/training-operator#2298.
I feel like if we want to be consistent across all of our Batch Jobs CRDs, we should not introduce such terminology for the Job status. Especially, if we want to match our Jobs counters with the actual status of the Job: https://github.com/kubernetes-sigs/jobset/blob/main/api/jobset/v1alpha2/jobset_types.go#L180. |
This is practically part of the api now, so we need to bump the api version for this change, we could do this only when we graduate to v1 |
What would you like to be added:
@tenzen-y and I noticed that JobSet uses the
Completed
status rather thanComplete
.However, Kubernetes Job uses the
Complete
status.We also use the
Complete
status in the TrainJob to be consistent with the Batch/Job.Additionally, I would propose that we rename
Succeeded
ReplicatedJobStatus toComplete
, since there is no such status asSucceeded
on the Job level. E.g.Any thoughts @tenzen-y @kannon92 @danielvegamyhre @ahg-g ?
Why is this needed:
Being consistent across various Kubernetes resources.
This enhancement requires the following artifacts:
The artifacts should be linked in subsequent comments.
The text was updated successfully, but these errors were encountered: