Implement webhook validation for the TFJob by tenzen-y · Pull Request #2051 · kubeflow/trainer

tenzen-y · 2024-04-10T17:27:56Z

What this PR does / why we need it:
I implemented webhook validations for the TFJob.
Additionally, I didn't add any additional validations. The traininig-operator has the same validations the same as before.

Although we potentially commonize podTemplate validations, I leave such commonize in the follow-ups.

This PR depends on #2035

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

Checklist:

Docs included if any changes are user facing

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

google-oss-prow · 2024-04-10T17:28:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

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

tenzen-y · 2024-04-10T17:28:37Z

/hold
for review

tenzen-y · 2024-04-10T18:25:14Z

/assign @andreyvelich @johnugeorge

This is the next PR to introduce webhook validations. PTAL, thanks.

johnugeorge · 2024-04-10T18:39:32Z

Some tests are timing out

tenzen-y · 2024-04-10T18:43:39Z

Some tests are timing out

I guess that some tests reach GHA limitations...

tenzen-y · 2024-04-10T18:45:23Z

Some tests are timing out

I guess that some tests reach GHA limitations...

This error seems to be related it: docker/buildx#841

johnugeorge · 2024-04-10T19:21:15Z

/lgtm

andreyvelich

Thanks for this @tenzen-y!
A few comments.

andreyvelich · 2024-04-10T19:32:11Z

pkg/apis/kubeflow.org/v1/tensorflow_types.go

+func IsChiefOrMaster(typ ReplicaType) bool {
+	return typ == TFJobReplicaTypeChief || typ == TFJobReplicaTypeMaster
+}
+


@tenzen-y Does it make sense to move this function in webhooks/tensorflow/tfjob_webhook.go since we use it there and it will help us to avoid creation of additional file named tensorflow_types_test.go?

Indeed. I'm wondering if we can put this function in the webhook package.
However, I decided to put this here since this function is not only for the webhooks like this: https://github.com/search?q=repo%3Akubeflow%2Ftraining-operator%20IsChiefOrMaster&type=code

@tenzen-y Do we really need this check in the controller if we already verify if TFJob contains Chief or Master here: https://github.com/kubeflow/training-operator/blob/c20422067e3ef81df39d03c6f285353344d8f77d/pkg/controller.v1/tensorflow/tfjob_controller.go#L452C6-L452C31 ?

Yes, we need it since the webhook verifies only if the master/chief role replica is 0 or 1.
When TFJob doesn't have any master/chief role, the training operator switches there based on the check.

Alright, since we need additional check to verify if rtype is Chief of Master

andreyvelich · 2024-04-10T19:35:36Z

pkg/webhooks/tensorflow/tfjob_webhook.go

+	if errors := apimachineryvalidation.NameIsDNS1035Label(job.Name, false); len(errors) != 0 {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), job.Name, fmt.Sprintf("should match: %v", strings.Join(errors, ","))))
+	}


Should we move this validation to the helper function in /webhooks/utils/utils.go since this check is common to all Framework ?

Yes, we can commonize these functions, but as I mentioned in the description, commonizing in a separate PR would be better. Because the minimum validation change could prove that webhook validations would not bring any bugs/breaks.

Although we potentially commonize podTemplate validations, I leave such commonize in the follow-ups.

Sure, that sounds good @tenzen-y!
We might need to track it in the separate issue.

Created: #2054

andreyvelich · 2024-04-10T19:37:44Z

pkg/webhooks/tensorflow/tfjob_webhook.go

+		for idx, container := range rSpec.Template.Spec.Containers {
+			if container.Image == "" {
+				allErrs = append(allErrs, field.Required(containerPath.Index(idx).Child("image"), "must be required"))
+			}
+			if container.Name == trainingoperator.TFJobDefaultContainerName {
+				defaultContainerPresent = true
+			}
+		}


Similar to above comment maybe that could be moved to the helper function in which we pass container name for the check.

andreyvelich

Thank you @tenzen-y!
/lgtm

andreyvelich · 2024-04-10T20:30:08Z

pkg/apis/kubeflow.org/v1/tensorflow_types.go

+func IsChiefOrMaster(typ ReplicaType) bool {
+	return typ == TFJobReplicaTypeChief || typ == TFJobReplicaTypeMaster
+}
+


Alright, since we need additional check to verify if rtype is Chief of Master

tenzen-y · 2024-04-10T20:34:05Z

Thanks all!

/hold cancel

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Implement webhook validation for the TFJob

7a08604

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

google-oss-prow bot requested review from jinchihe and kuizhiqing April 10, 2024 17:28

google-oss-prow bot added approved size/XL labels Apr 10, 2024

tenzen-y mentioned this pull request Apr 10, 2024

Implement webhook validation for the TFJob #2036

Closed

1 task

google-oss-prow bot added the do-not-merge/hold label Apr 10, 2024

tenzen-y mentioned this pull request Apr 10, 2024

Introduce Webhook Validation #1993

Closed

6 tasks

google-oss-prow bot assigned andreyvelich and johnugeorge Apr 10, 2024

google-oss-prow bot added the lgtm label Apr 10, 2024

andreyvelich reviewed Apr 10, 2024

View reviewed changes

tenzen-y mentioned this pull request Apr 10, 2024

Commonize webhook validations at the some points #2054

Closed

andreyvelich reviewed Apr 10, 2024

View reviewed changes

google-oss-prow bot removed the do-not-merge/hold label Apr 10, 2024

google-oss-prow bot merged commit 1a73cf1 into kubeflow:master Apr 10, 2024

tenzen-y deleted the implement-tfjob-webhook-validations branch April 10, 2024 20:35

johnugeorge pushed a commit to johnugeorge/training-operator that referenced this pull request Apr 28, 2024

Implement webhook validation for the TFJob (kubeflow#2051)

f469162

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

johnugeorge pushed a commit to johnugeorge/training-operator that referenced this pull request Apr 28, 2024

Implement webhook validation for the TFJob (kubeflow#2051)

c99d1f6

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Conversation

tenzen-y commented Apr 10, 2024

Uh oh!

google-oss-prow bot commented Apr 10, 2024

Uh oh!

tenzen-y commented Apr 10, 2024

Uh oh!

tenzen-y commented Apr 10, 2024

Uh oh!

johnugeorge commented Apr 10, 2024

Uh oh!

tenzen-y commented Apr 10, 2024

Uh oh!

tenzen-y commented Apr 10, 2024

Uh oh!

johnugeorge commented Apr 10, 2024

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tenzen-y commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants