-
Notifications
You must be signed in to change notification settings - Fork 439
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
Custom CRD: Support dynamic Trial's jobs conditions #1307
Custom CRD: Support dynamic Trial's jobs conditions #1307
Conversation
@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: czheng94, nielsmeima. Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
2df1462
to
fa7811a
Compare
@gaocegege @johnugeorge @sperlingxx This PR is ready for review. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign @johnugeorge
} | ||
|
||
// TODO (andreyvelich): Can be deleted after custom CRD is implemented | ||
func (r *ReconcileTrial) UpdateTrialStatusConditionDeprecated(instance *trialsv1beta1.Trial, deployedJob *unstructured.Unstructured, jobCondition *commonv1.JobCondition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaocegege We can delete it after we finish Custom CRD implementation.
Controller still runs this function if successCondition and failureCondition is not set here:
r.UpdateTrialStatusConditionDeprecated(instance, deployedJob, jobCondition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Please keep track of the cleanup required to remove the deprecated functions. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge 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 |
409b3a2
to
37393cc
Compare
/lgtm |
/retest |
1 similar comment
/retest |
Related: #1214.
WIP until we merge this: #1305, after that I will generate API clients, etc, but it would be great if you can start to review it.
I changed a bit API from proposal to support various success and failure conditions in CRD.
As I can see not all CRDs are using the same status approach with Conditions list as Kubeflow operators or BatchJobs.
For example, Argo uses
.status.phase
to define whether Node is succeeded or failed.To handle all of these cases, I proposed approach when user defines
SuccessCondition
andFailureCondition
in Experiment.This condition is
gjson
expression which can directly point when job is succeeded or failed.For example, for TFJob:
For Argo Workflow:
I think this approach is extensible, since user can define any expression there when they want to fail Trial.
For example, when TFJob reaches 3 failed replicas.
status.replicaStatuses.master.[@this].#(failed=="3")
For Kubeflow operators or BatchJobs we can define these conditions in advance if we don't want to let user manually add them.
I renamed
UpdateTrialStatusCondition
toUpdateTrialStatusConditionDeprecated
, since I had to refactorUpdateTrialStatusCondition
.In job_util.go I created
TrialJobStatus
which represents current Job status.I also transfer Message and Reason from deployed Job to Trial if it is possible.
/assign @gaocegege @johnugeorge @sperlingxx
/cc @czheng94 @nielsmeima