/hold

Hi @toshiiw. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/assign @YujiOshima

/ok-to-test

@YujiOshima I was asking if we know the reason behind the error. Else, this error will be hidden with this fix.

I think we can't know the exactly reason.
But because of mutex I believe the reasons of this error are only completion of Worker, creation of job from MetricsCollector cronjob, and Fail of Worker.
Since these updates are not controled by studyjob operator.
And they can handle at the next reconcile.

The reason why we need this is avoiding endless retry.
When we use a heavy suggestion service which take longer than one minute for creating a suggestion, the metrics collector must create a new job during studyjob operator wait for the suggestion.
Then the studyjob will fail to update and retry.
The studyjob won't update any more.

I think we can't know the exactly reason.
But because of mutex I believe the reasons of this error are only completion of Worker, creation of job from MetricsCollector cronjob, and Fail of Worker.

@YujiOshima Can you please explain more on this situation? Why will this error happen during these conditions? Error says that the object got modified before the update call.

Since these updates are not controled by studyjob operator.
And they can handle at the next reconcile.

@johnugeorge When any updates on a resource that is corresponding to a studyjob, the operator can't update the studyjob.

A studyjob named "A" has a worker "A-w1" and a metrics collector cron-job "A-mc1".
First, The operator get instance by r.Get here
And the operator waiting for some process (e.g. GetSuggestion).
During the operator waiting, "A-mc1" create a job. Since "A-mc1" is a cron-job, it will create a job every minute.
At this time, the studyjob CR is modified from the operator getted.
So the operator get error when call r.Update here

At this time, the studyjob CR is modified from the operator getted.
@YujiOshima In the above example, i didn't understand how studyJobCR is getting modified after mc job is created. Can you point out to the code? AFAIK, error happens when update is called on a object that already got modified from the initial Get

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yujioshima

If they are not already assigned, you can assign the PR to them by writing /assign @yujioshima in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/hold cancel
Seems to work in my local env.

Related: kubernetes-sigs/controller-runtime#343

Since we are watching on job pods, there are higher chances of Reconcile functions called continuously. Immediate Get after Update can have stale entries. This will cause the next Update to fail. However, this should not be a problem since we can handle this during the next Reconcile.

@johnugeorge Thanks. So we should not believe values in CR, believe values in DB.
I think it would be a problem for currently Katib. For example, when fail to update after getSuggestion, the suggestionCount will be inconsistent between DB and CR.
Should we fix that bug and other related bugs now or think about this after v2API? WDYT?

For example, when fail to update after getSuggestion, the suggestionCount will be inconsistent between DB and CR.

This patch treats the suggestionCount in the DB as the single source of the truth. Inconsistency with this should be resolved in the next reconcile run.

@toshiiw When the operator get stale version of CR, operator won't set false to nextSuggestionSchedule here even the GetSuggestion is alrealy called in previous Reconcile.
Then GetSuggestion is called duplicatedly and create workers but update will be fail. So the workers will be orphan.

It is not accurate to say that values in CR should not be believed. From the k8s philosophy, We have to make sure that design is level triggered based than edge triggered based. If it is a level triggered design, reconcile function will make sure that actual state moves towards the desired state.

i think, we should think after v2API because there will be significant code changes. We have to ensure this.

@johnugeorge Agreed. The current Reconcile() is written somewhat in a edge-triggered way. We might need to put some worker related info in CR or DB to ensure a level-triggered design works.

/close

Thanks for the PR. But we already dropped v1alpha1. We can't merge it since v1alpha1 is deprecated.

@gaocegege: Closed this PR.