Verify that Trials were successfully deleted

[andreyvelich]

When I tried to update parallelTrialCount to less value, I got errors in controller.

In deleteTrials we should verify that Trials were successfully deleted before running reconcile loop again and update suggestion requests and status.suggestions.

Otherwise, updateTrialsSummary is resynchronized with Trial list and addCount for ReconcileSuggestion is incorrect.

/assign @gaocegege @johnugeorge

[kubeflow-bot]

This change is 

[andreyvelich]

I updated test, unit tests locally works good, I don't know why it fails sometime in CI.

@gaocegege @johnugeorge @sperlingxx Do you have any ideas why we have 2 Travis in our pre-submits and post-submits?
First is on travis-ci.org https://travis-ci.org/github/kubeflow/katib/builds.
Second is on travis-ci.com https://travis-ci.com/github/kubeflow/katib/builds.

[andreyvelich]

Should we disable Katib on travis-ci.org since it is outdated: https://blog.travis-ci.com/2018-05-02-open-source-projects-on-travis-ci-com-with-github-apps ?

[gaocegege]

Should we disable Katib on travis-ci.org since it is outdated: https://blog.travis-ci.com/2018-05-02-open-source-projects-on-travis-ci-com-with-github-apps ?

SGTM

BTW, I am not sure if I understand the PR, can you give me an example about why we need it?

[andreyvelich]

@gaocegege If you tried to edit Experiment, e.g change parallelTrialCount from 4 to 2, 2 Trial has to be deleted.
Here we execute Delete:

katib/pkg/controller.v1beta1/experiment/experiment_controller.go

Line 389 in 9cf4544

if err := r.Delete(context.TODO(), &trialSlice[i]); err != nil {

After that, k8s objet will not be deleted immediately, finalizers run after Trial deletion, because of that I added check before finish Experiment reconciler.
If we don't do it, on the next Experiment reconciler loop List:

katib/pkg/controller.v1beta1/experiment/experiment_controller.go

Line 266 in 9cf4544

if err := r.List(context.TODO(), lo, trials); err != nil {

returns Trial list with 2 deleting Trials.
Then, UpdateExperimentStatus overrides Experiment status incorrect:

katib/pkg/controller.v1beta1/experiment/experiment_controller.go

Line 271 in 9cf4544

if err := util.UpdateExperimentStatus(r.collector, instance, trials); err != nil {

.
And addCount will be not correct:

katib/pkg/controller.v1beta1/experiment/experiment_controller.go

Line 315 in 9cf4544

addCount := requiredActiveCount - activeCount

.

Suggestion status also needs to be updated since we store Trials assignment there.

[andreyvelich]

@jlewi Could you help us to disable travis-ci.org for Katib, please?
I don't have permission to disable travis-ci.org from here: https://travis-ci.org/organizations/kubeflow/repositories, or from Katib repo settings.

[andreyvelich]

/retest

[gaocegege]

/lgtm

[gaocegege]

/lgtm

[andreyvelich]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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:

• OWNERS [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andreyvelich]

/retest

[andreyvelich]

/retest