[harshrajdebug]

What this PR does / why we need it:

This is option 1 from #4153.

Since OptimizationJob had no rule on metadata.name the API server accepted names the controller cannot use. A leading digit made every trial fail TrainJob validation, a dot made the algorithm Service fail, and a name over 63 characters made the Deployment fail on its trainer.kubeflow.org/optimization-job label. None of these set a Failed condition. This adds the TrainJob name format rule to OptimizationJob, an RFC 1035 label. The length limit is left to the separate change discussed in #4260, so a name over 63 characters is still accepted until then.

GetAlgorithmServiceName cut names longer than 46 characters to 46, so two OptimizationJobs whose names share their first 46 characters got the same Deployment and Service. When a name is cut, it now ends in the first 8 hex characters of a SHA-256 hash of the full name, which keeps it within 63 characters. The issue suggested the UID. I hashed the name instead, because names are already unique within a namespace and the result does not depend on the UID being set. Names of 46 characters or fewer keep the same algorithm service name as before, and OptimizationJob has not been in a release yet, so the new name for longer ones changes nothing for an existing installation.

Tests:

• TestGetAlgorithmServiceName now expects the hashed form for long names and covers two names that share their first 46 characters. The expected hashes were computed outside Go.
• The OptimizationJob webhook table checks that a leading digit, a dot, uppercase letters and a trailing hyphen are rejected, and that a valid name and a 63-character name are accepted.

Against master, the six hashed-name unit cases and the leading-digit and dot entries fail. The valid name, the 63-character name, the uppercase and the trailing-hyphen entries pass on master too, since the API server already rejects uppercase names and names ending in a hyphen. The generated CRDs match controller-gen v0.23.0 with the flags from the make manifests steps, and only the two OptimizationJob CRD files changed.

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

Checklist:

• Docs included if any changes are user facing

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jeffwan for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[harshrajdebug]

@robert-bell could you add /ok-to-test here? The test workflows have been waiting for approval since 7 Oct, so the unit, integration and E2E tests have not run on this PR yet.

[akshaychitneni]

thanks for the fix. please look into the review comments (used ai tools to review)

[andreyvelich]

/ok-to-test

[harshrajdebug]

Thanks @akshaychitneni, I applied all five suggestions in 872b114 and rebased onto master. The rebase also fixes the govulncheck failure: the workflow installs Go from the PR's go.mod, which still said 1.26 on the old base, while master now needs 1.27.

I checked the new tests against the old behavior. With the old algorithm service naming, the new check for two Deployments and two Services fails on every run. The trailing-hyphen entry passes with or without the CEL rule, because the API server's own name validation already rejects a name ending in a hyphen, as it does for the uppercase entry. The leading-digit and 64-character entries are the ones that fail without the rule.

@andreyvelich, on the name limit from #4260: a trial name is the OptimizationJob name plus 15 characters, and each Pod of the trial's JobSet is named after it, followed by the replicated job name, the job and Pod indexes, and a 5-character random suffix, all joined by hyphens. So the longest safe OptimizationJob name depends on the runtime: 33 characters for torch-distributed with up to 10 nodes, and 18 for runtimes with a dataset-initializer job. Longer names give trials whose JobSet is rejected for a Pod name over 63 characters.

I see two ways to add the check here:

1. A fixed maximum taken from the TrainJob limit once TrainJob accepts names that are too long for the JobSet it creates, and the TrainJob then never starts #4260 settles it, which is that limit minus 15.
2. A check in the OptimizationJob webhook that reads the runtime of the TrainJob template and rejects a name whose trial Pod names would go over 63, as @aniket2405 suggested.

I'd go with the second, since it doesn't depend on the TrainJob number, unless you prefer the fixed limit.