Enhancement for Custom CRD

[andreyvelich]

I was testing Custom CRD.
I was able to run Tekton TaskRun as Trial CR. I Will submit PR with few examples soon.
I found these problems while running:

• I disabled MutateJob in controller. We will track it in this issue: Refactoring Supported Job List #1320.

• I disabled validation for Job other than SupportedJobList.

• We should verify PrimaryContainerName in MutateVolume also.

• I changed INSERT in RegisterObservationLog function. It is better to insert all lines in one SQL query, instead of running separate query for each line. It helps to avoid unnecessary Trial updates.

• I added reconcile re-queue if metrics are not available. This use-case happens in Tekton job. Tekton task is succeeded once Training container is finished, but metrics can be not collected, yet. Because of that, controller reconciles Trial without observation and turns Trial status in Metrics Unavailable. To avoid it, I try to re-queue controller for maxRequeueCount times to convert Trial to Succeeded status. @gaocegege @johnugeorge What do you think about this approach, can we do it in other way?

/assign @gaocegege @johnugeorge

[kubeflow-bot]

This change is 

[andreyvelich]

I figure out that we send all Trials to Suggestion. I think we don't need to send Trials with unavailable metrics to Suggestion service since we try to reconcile controller until metrics are reported.
WDYT @gaocegege @johnugeorge ?

[gaocegege]

SGTM, we do not need to send these trials.

[gaocegege]

/lgtm

[andreyvelich]

@gaocegege We were discussing with @johnugeorge about my first solution with re-queuing controller and saving data in map.
It has few disadvantages.

1. We don't want to waste resources with this map and have problems with controller restarts as you mentioned before.
2. It is not a good way to monitor Reconcile restarts. We don't really know how many times Trial controller will be restarted and it depends on user's cluster.

I propose a bit different solution:

• If metrics collector doesn't find objectiveMetricName in the training logs, it reports unavailable value to the DB.
• If Trial CRD Job is succeeded, controller re-queues Trial object until DB records with appropriate trial name are empty.
• If DB has a record with unavailable metric value, Trial changes status to MetricsUnavailable

What do you think about this?

[andreyvelich]

/retest

[andreyvelich]

/retest

[johnugeorge]

/lgtm
/approve

[k8s-ci-robot]

[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:

• OWNERS [johnugeorge]

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