KEP-2170: Implement skeleton webhook servers #2251

tenzen-y · 2024-09-06T16:39:28Z

What this PR does / why we need it:
I implemented the skeleton webhook servers for v2 operator.

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 #2209

Checklist:

Docs included if any changes are user facing

coveralls · 2024-09-06T16:43:42Z

Pull Request Test Coverage Report for Build 10778925774

Details

21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 10739885471:	0.0%
Covered Lines:	66
Relevant Lines:	66

💛 - Coveralls

tenzen-y · 2024-09-06T16:44:03Z

/assign @kubeflow/wg-training-leads

tenzen-y · 2024-09-06T16:51:54Z

I will delegate the actual validation implementations to other contributors who are interested in this project.

andreyvelich

Thanks for this @tenzen-y!
I left a few comments.

andreyvelich · 2024-09-06T20:51:59Z

manifests/v2/base/service.yaml

+    prometheus.io/port: "8080"
+  labels:
+    app: training-operator
+  name: training-operator


I think, we discussed before that we want to allow to deploy V1 and V2 operator, so I suggest we name it as:

Suggested change

name: training-operator

name: training-operator-v2

The Service was removed based on #2251 (comment) discussion.

andreyvelich · 2024-09-06T20:52:59Z

manifests/v2/base/service.yaml

+  labels:
+    app: training-operator


Let's use the appropriate label here:

Suggested change

labels:

app: training-operator

labels:

training.kubeflow.org/component: controller

I make it similar to Katib: https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/components/controller/controller.yaml#L17

Actually, I was not supposed to add this file in this PR. I would like to add Service and Deployment manifests in #2208.

So, could we remove Service and Deployment from this PR so that we can focus on setting up skeleton webhook servers?

Sounds good, we can add them in a separate PR.

andreyvelich · 2024-09-06T20:58:31Z

manifests/v2/base/service.yaml

+      protocol: TCP
+      targetPort: 9443
+  selector:
+    control-plane: kubeflow-training-operator


Suggested change

control-plane: kubeflow-training-operator

training.kubeflow.org/component: controller

The Service was removed based on #2251 (comment) discussion.

andreyvelich · 2024-09-06T21:01:07Z

manifests/v2/base/webhook/manifests.yaml

+apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  name: mutating-webhook-configuration


I am wondering for what are we planning to use Mutating Webhook ?

IIUC, we need to default .spec.mlPolicy.numNodes to 1, right?

Yeah, but can we set these values via CEL or we require Mutating Webhook in any case if we want to add default values?

That makes sense. In that case, let's decide if we should introduce the webhook mutator during #2209.

So, we can remove the mutator for now.

andreyvelich · 2024-09-06T21:03:06Z

manifests/v2/base/webhook/manifests.yaml

+apiVersion: admissionregistration.k8s.io/v1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: validating-webhook-configuration


Can we call it similar to Katib webhooks ?

Suggested change

name: validating-webhook-configuration

name: training.kubeflow.org

This is auto-generated name by controller-gen.
So, we can not change this name.

I remember that we discussed before that we can rename it via Kustomize patch: https://github.com/kubeflow/training-operator/blob/master/manifests/base/webhook/patch.yaml.

Yes, that's right. But we do not add any manifests in this PR because I added only manifests generated by controller-gen so that we can start up the webhook servers during envtest.

In other words, these manifests are at least manifests to launch webhook servers during integration testing.

So, I think #2208 has the responsibility to add manifests not generated by controller-tools. Any thoughts?

Sounds good. @tenzen-y Please can you add the comment in #2208 that we should add the Kustomize patch to update those names ?

Sure. I will describe the detailed task in #2208.

andreyvelich · 2024-09-06T21:04:49Z

manifests/v2/base/webhook/manifests.yaml

+      namespace: system
+      path: /validate-kubeflow-org-v2alpha1-clustertrainingruntime
+  failurePolicy: Fail
+  name: vclustertrainingruntime.training-operator.kubeflow.org


Should we simplify it ?

Suggested change

name: vclustertrainingruntime.training-operator.kubeflow.org

name: clustertrainingruntime.training.kubeflow.org

To avoid the conflict between the defaulter and the validator, let's use the "validator" prefix.
Additionally, could we use the "training-operaotr.kubeflow.org" domain since we have 2 components, training-operator and mpi-operator for the training?

To avoid the conflict between the defaulter and the validator, let's use the "validator" prefix

What do you think about the full names, like we did for Katib webhooks:

validator.clustertrainingruntime.kubeflow.org defaulter.clustertrainingruntime.kubeflow.org

Additionally, could we use the "training-operaotr.kubeflow.org" domain since we have 2 components, training-operator and mpi-operator for the training?

Should we use mpi.kubeflow.org for MPI Operator webhooks for now ?

Sure, it could work fine now.

The TainJob and TrainingRuntime are still in the alpha stage. So, it is easy to change the name once we face conflicts with other kubeflow projects.

Indeed, the MPIOperator does not have any webhooks now.

andreyvelich · 2024-09-06T21:05:08Z

manifests/v2/base/webhook/manifests.yaml

+      namespace: system
+      path: /validate-kubeflow-org-v2alpha1-trainingruntime
+  failurePolicy: Fail
+  name: vtrainingruntime.training-operator.kubeflow.org


Suggested change

name: vtrainingruntime.training-operator.kubeflow.org

name: trainingruntime.training.kubeflow.org

andreyvelich · 2024-09-06T21:05:22Z

manifests/v2/base/webhook/manifests.yaml

+      namespace: system
+      path: /validate-kubeflow-org-v2alpha1-trainjob
+  failurePolicy: Fail
+  name: vtrainjob.training-operator.kubeflow.org


Suggested change

name: vtrainjob.training-operator.kubeflow.org

name: trainjob.training.kubeflow.org

andreyvelich · 2024-09-06T21:19:06Z

pkg/webhook.v2/setup.go

+		return "TrainingRuntime", err
+	}
+	if err := setupWebhookForTrainJob(mgr); err != nil {
+		return "TranJob", err


Can we move those Kind types to constants ?

Suggested change

return "TranJob", err

return "TrainJob", err

andreyvelich · 2024-09-06T21:22:05Z

manifests/v2/base/service.yaml

@@ -0,0 +1,22 @@
+apiVersion: v1


Do you want to create deployment for controller as well ?
Also, what do you think about moving service and controller under /base/controller or /base/manager folder ?

The Service was removed based on #2251 (comment) discussion.

tenzen-y · 2024-09-09T16:44:32Z

@andreyvelich I addressed the comments. The only remaining point is #2251 (comment).

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

tenzen-y · 2024-09-09T18:18:52Z

@andreyvelich I addressed all comments. PTAL.

andreyvelich · 2024-09-09T19:04:48Z

pkg/webhook.v2/setup.go

+		return "ClusterTrainingRuntime", err
+	}
+	if err := setupWebhookForTrainingRuntime(mgr); err != nil {
+		return "TrainingRuntime", err
+	}
+	if err := setupWebhookForTrainJob(mgr); err != nil {
+		return "TrainJob", err
+	}


@tenzen-y Do you return these names only to handle this error: https://github.com/kubeflow/training-operator/blob/master/cmd/training-operator.v2alpha1/main.go#L150C5-L152 ?
Do you think it is necessary ?
If some of the webhook will fail, we will get this info since we traceback error from the underlying file: ClusterTrainingRuntime, TrainingRuntime, or TrainJob.

Yes, I believe that this name is helpful because this is not for validation error, this is for set up error.
This error can be seen when the startup webhook server failed.

That's right, but without this name, can we see the file traceback of this error during the webhook setup ?

I guess that the traceback will show the path where the error occurs. However, I still believe this helps understand the components in which failure occurs since this component name (ex. TrainJob) can be shown as a top-level log, and this top-level log allows us to avoid digging into nested traceback logs.

Alright, sounds good!

pkg/webhook.v2/trainingruntime_webhook.go

andreyvelich

Thank you for this @tenzen-y!
/lgtm
/approve

google-oss-prow · 2024-09-09T20:11:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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~~ [andreyvelich]

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-09-09T21:01:18Z

Thank you for this @tenzen-y! /lgtm /approve

Thank you!

google-oss-prow bot requested review from jinchihe and kuizhiqing September 6, 2024 16:39

google-oss-prow bot added the size/XL label Sep 6, 2024

andreyvelich reviewed Sep 6, 2024

View reviewed changes

tenzen-y force-pushed the implement-sckelton-webhook-servers branch from a74d4db to 967b68d Compare September 9, 2024 16:42

google-oss-prow bot added size/L and removed size/XL labels Sep 9, 2024

KEP-2170: Implement skeleton webhook servers

3538acd

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

tenzen-y force-pushed the implement-sckelton-webhook-servers branch from 967b68d to 3538acd Compare September 9, 2024 18:18

andreyvelich reviewed Sep 9, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Sep 9, 2024

google-oss-prow bot added the lgtm label Sep 9, 2024

google-oss-prow bot added the approved label Sep 9, 2024

google-oss-prow bot merged commit 7c8d4df into kubeflow:master Sep 9, 2024
39 checks passed

tenzen-y deleted the implement-sckelton-webhook-servers branch September 9, 2024 21:01

	control-plane: kubeflow-training-operator
	training.kubeflow.org/component: controller

	name: validating-webhook-configuration
	name: training.kubeflow.org

	name: vclustertrainingruntime.training-operator.kubeflow.org
	name: clustertrainingruntime.training.kubeflow.org

	name: vtrainingruntime.training-operator.kubeflow.org
	name: trainingruntime.training.kubeflow.org

	name: vtrainjob.training-operator.kubeflow.org
	name: trainjob.training.kubeflow.org

KEP-2170: Implement skeleton webhook servers #2251

KEP-2170: Implement skeleton webhook servers #2251

Conversation

tenzen-y commented Sep 6, 2024 • edited Loading

coveralls commented Sep 6, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10778925774

Details

💛 - Coveralls

tenzen-y commented Sep 6, 2024

tenzen-y commented Sep 6, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Sep 9, 2024

tenzen-y commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Sep 9, 2024

tenzen-y commented Sep 9, 2024

tenzen-y commented Sep 6, 2024 •

edited

Loading

coveralls commented Sep 6, 2024 •

edited

Loading

andreyvelich Sep 9, 2024 •

edited

Loading