Custom CRD: Set dynamic watch from controller flags

[andreyvelich]

Related: #1214.

I added possibility to set trial controller watchers from controller flags.
In controller deployment flags should be in format Kind.version.group.

For example:

args:
  - "-trial-resource=TFJob.v1.kubeflow.org".
  - "-trial-resource=Workflow.v1alpha1.argoproj.io"
  - "-trial-resource=Job.v1.batch"

/assign @gaocegege @sperlingxx @johnugeorge

[kubeflow-bot]

This change is 

[gaocegege]

/lgtm

[sperlingxx]

Shall we check whether there exists any overlap between dynamic GVKs and GVKs of SupportedJobList?

[andreyvelich]

Shall we check whether there exists any overlap between dynamic GVKs and GVKs of SupportedJobList?

I think after complete custom CRD implementation we can remove SupportedJobList and by default add few trial-resources in Katib controller deployment for testing/examples.
What do you think @sperlingxx @gaocegege @johnugeorge ?

[sperlingxx]

Shall we check whether there exists any overlap between dynamic GVKs and GVKs of SupportedJobList?

I think after complete custom CRD implementation we can remove SupportedJobList and by default add few trial-resources in Katib controller deployment for testing/examples.
What do you think @sperlingxx @gaocegege @johnugeorge ?

Maybe we can keep SupportedJobList as a legacy and make current custom CRD implementation as default approach to watch and renconcile in trial level? In this way, old-fashioned SupportedJobList could also be supported as a non-default option. In short, trial-controller will either watch SupportedJobList or custom CRDs, and the later one is default option.

[gaocegege]

Maybe we can keep SupportedJobList as a legacy and make current custom CRD implementation as default approach to watch and renconcile in trial level? In this way, old-fashioned SupportedJobList could also be supported as a non-default option. In short, trial-controller will either watch SupportedJobList or custom CRDs, and the later one is default option.

SGTM

[andreyvelich]

@sperlingxx Do you mean left SupportedJobList to not specify additional parameters in Trial template for default jobs (e.g. PrimaryPodLabel, PrimaryContainerName, SucceededCondition) ?
Do we have any other cases when SupportedJobList is useful ?

After implementation, Job provider can be used only for custom MutateJob function as we discussed here: #1273 (comment), other functions can be omitted.

[sperlingxx]

@sperlingxx Do you mean left SupportedJobList to not specify additional parameters in Trial template for default jobs (e.g. PrimaryPodLabel, PrimaryContainerName, SucceededCondition) ?
Do we have any other cases when SupportedJobList is useful ?

After implementation, Job provider can be used only for custom MutateJob function as we discussed here: #1273 (comment), other functions can be omitted.

Yes, I supppose, due to MutateJob function, we should keep the whole stack of SupportedJobList?

[andreyvelich]

@sperlingxx For MutateJob we don't actually need SupportedJobList.
I think for custom MutateJob function you can just check the Kind of unstructured Job and make some changes if it is needed.
What do you think ?

[sperlingxx]

@sperlingxx For MutateJob we don't actually need SupportedJobList.
I think for custom MutateJob function you can just check the Kind of unstructured Job and make some changes if it is needed.
What do you think ?

Sry for slow reply. But @andreyvelich how do we know which MutateJob function need to be invoked given an unstructured Job ?

[andreyvelich]

@sperlingxx We invoke MutateJob with desiredJob object and we can get Kind of it.
Can we refactor Provider to have MutateJob per Kind?
In that case, users can re-use provider interface to define their own MutateJob function.
I believe all other functionality to handle Trial can be done without Provider.

[sperlingxx]

@sperlingxx We invoke MutateJob with desiredJob object and we can get Kind of it.
Can we refactor Provider to have MutateJob per Kind?
In that case, users can re-use provider interface to define their own MutateJob function.
I believe all other functionality to handle Trial can be done without Provider.

SGTM

[andreyvelich]

@gaocegege @johnugeorge @sperlingxx The test passed, can you approve it again, please?

[johnugeorge]

After this change, Is supportedJobList required?

[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