Add validation for verifying that the CustomJob (e.g., TFJob) name meets DNS1035

[tenzen-y]

Signed-off-by: Yuki Iwai [email protected]

What this PR does / why we need it:
I added validation for verifying that the CustomJob (e.g., TFJob) name meets DNS1035.
Also, I refactored unit tests for validation.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1745

Checklist:

• Docs included if any changes are user facing

[tenzen-y]

/assign @johnugeorge @zw0610

[coveralls]

Pull Request Test Coverage Report for Build 4056733428

• 54 of 56 (96.43%) changed or added relevant lines in 10 files are covered.
• 10 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.7%) to 39.535%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller.v1/mxnet/mxjob_controller.go	0	1	0.0%
pkg/controller.v1/xgboost/xgboostjob_controller.go	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/apis/kubeflow.org/v1/pytorch_validation.go	2	90.91%
pkg/apis/kubeflow.org/v1/tensorflow_validation.go	2	75.0%
pkg/apis/kubeflow.org/v1/xgboost_validation.go	2	91.84%
pkg/controller.v1/mpi/mpijob_controller.go	4	76.79%

Totals
Change from base Build 4012132598:	0.7%
Covered Lines:	2735
Relevant Lines:	6918

---

💛 - Coveralls

[tenzen-y]

@johnugeorge Do we need to cherry-pick this commit to the v1.6 branch?

[johnugeorge]

Thanks @tenzen-y for this.
Since rc0 manifests have already merged for Kubeflow testing, shall we take this up for next release ? What do you think?We can merge it to master anyways

[tenzen-y]

Thanks @tenzen-y for this. Since rc0 manifests have already merged for Kubeflow testing, shall we take this up for next release ? What do you think?We can merge it to master anyways

I'm ok with taking this up for the next release since #1745 is not the critical bug.

[zw0610]

I could be wrong. The code changes introduced by this pr seems a bit tremendous to me.

Another approach might be: skipping changes under pkg/apis and adding apimachineryvalidation.NameIsDNS1035Label to each job controller files under pkg/controller.v1 for job controllers that will generate Service.

However, I agree with the idea to change from validating job spec to validating the entire job. Maybe after the recent release, we can focus on a refactor version of training operator and introduce this new validating idea.

[tenzen-y]

Another approach might be: skipping changes under pkg/apis and adding apimachineryvalidation.NameIsDNS1035Label to each job controller files under pkg/controller.v1 for job controllers that will generate Service.

Does that mean we want to introduce your another approach to the next training-operator release (v1.6)?

Maybe after the recent release, we can focus on a refactor version of training operator and introduce this new validating idea.

We will not include this improvement in the next training-operator (v1.6) release. So this commit is merged only to the master branch and this becomes the improvement for the long term.

@zw0610

[tenzen-y]

I'm ok with taking this up for the next release since #1745 is not the critical bug.

The above next release meant v1.7, not v1.6.

[zw0610]

Does that mean we want to introduce your another approach to the next training-operator release (v1.6)?

No, what I commented is just to provide another possible to address the naming issue, which I believe could leads to less code changes. But it's just a suggestion and I'm OK with the approach in commit 68b582a

@tenzen-y

[tenzen-y]

Does that mean we want to introduce your another approach to the next training-operator release (v1.6)?

No, what I commented is just to provide another possible to address the naming issue, which I believe could leads to less code changes. But it's just a suggestion and I'm OK with the approach in commit 68b582a

@tenzen-y

@zw0610, I see. I would take my approach since this approach has been more tested and is safer than adding apimachineryvalidation.NameIsDNS1035Label to reconciler.

[zw0610]

Good.
/lgtm

[terrytangyuan]

Thanks!

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y, terrytangyuan

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 [terrytangyuan]

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