Emit events when fails to reconcile all trials #1706
Conversation
|
Hi @henrysecond1. 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 Once the patch is verified, the new status will be reflected by the 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. |
| continue | ||
| } | ||
| trialNames = append(trialNames, trial.Name) | ||
| } | ||
| // Failed to reconcile all trials | ||
| if len(trialNames) == 0 && len(errStrings) != 0 { | ||
| return fmt.Errorf(strings.Join(errStrings, "\n")) |
There was a problem hiding this comment.
Will we retry to create the trial after the error?
/cc @andreyvelich
There was a problem hiding this comment.
Yes, eventually Experiment controller will try to create Trials until we reach the maxFailedTrialCount: https://github.com/kubeflow/katib/blob/master/pkg/controller.v1beta1/experiment/util/status_util.go#L195-L202.
@henrysecond1 I think it is a great enhancement to record this error to the events, but do we want to check errors and returned Trials ? We should have clear requirement "When Trial is Failed".
For example, in case of this error we don't want to stop reconcile. For example, Trial is already exist because of unsynchronised policy of Kubernetes controllers.
I can see the following solution:
- rename
createTrialInstancetogetTrialInstance. - move Trial Creation out of this function.
- if
getTrialInstancefails, return error forcreateTrialsfunction. - if
r.Createfails continue the loop.
Any other suggestion @henrysecond1 @gaocegege @johnugeorge ?
There was a problem hiding this comment.
@andreyvelich Sorry for the late response and thank you for your advice.
I agree with the suggested solution. It seems reasonable to me.
I modified the code according to your suggestion, could you check it?
There was a problem hiding this comment.
@andreyvelich PTAL
Thanks for your time 😄
|
Hi @henrysecond1 Is it ready for another review? |
Oh yes, please review it when you are available :) |
|
Thank you for the updates @henrysecond1! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege, henrysecond1 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 |
What this PR does / why we need it:
The experiment controller is not showing any events when fails to reconcile all trials.
Thus, users not authorized to access the controller logs can not find the reason that why their trials are not created since no events are emitted by the experiment controller.
Please refer #1663 for detail.
Which issue(s) this PR fixes:
Fixes #1663