KEP-2170: Add manifests for Kubeflow Training V2 #2289

andreyvelich · 2024-10-17T23:13:23Z

I added manifests for Training Operator V2. I renamed the manager image to:

docker.io/kubeflow/training-operator-v2

That will allow users to use latest version for both V1 and V2 version of Training Operator.
In the future, we can deprecate the old version of Training Operator.

For now, I install JobSet using the release manifests in the Kustomize overlay. Let's discuss with @kubeflow/wg-manifests-leads what is the better approach long-term.

Additionally, I fixed the invalid validating webhook configuration name in our cert generator.

/assign @kubeflow/wg-training-leads
/hold for review

Signed-off-by: Andrey Velichkevich <[email protected]>

coveralls · 2024-10-18T15:38:08Z

Pull Request Test Coverage Report for Build 11413026138

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

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

Totals
Change from base Build 11390874125:	0.0%
Covered Lines:	73
Relevant Lines:	73

💛 - Coveralls

tenzen-y

Thank you for adding manifests!

tenzen-y · 2024-10-18T20:57:01Z

pkg/controller.v2/trainjob_controller.go

+//+kubebuilder:rbac:groups=jobset.x-k8s.io,resources=jobsets,verbs=get;list;watch;create
+//+kubebuilder:rbac:groups=scheduling.x-k8s.io,resources=podgroups,verbs=get;list;watch;create


Could you move these markers to the each job pipeline frame work plugins?

https://github.com/kubeflow/training-operator/tree/master/pkg/runtime.v2/framework/plugins

tenzen-y · 2024-10-18T20:59:23Z

pkg/controller.v2/trainjob_controller.go

+//+kubebuilder:rbac:groups=kubeflow.org,resources=trainingruntimes,verbs=get;list;watch
+//+kubebuilder:rbac:groups=kubeflow.org,resources=clustertrainingruntimes,verbs=get;list;watch


Could you move these markers to each job pipeline framework series?

https://github.com/kubeflow/training-operator/tree/master/pkg/runtime.v2/core

tenzen-y · 2024-10-18T21:01:01Z

cmd/training-operator.v1/main.go

@@ -52,6 +52,8 @@ import (
 const (
 	// EnvKubeflowNamespace is an environment variable for namespace when deployed on kubernetes
 	EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"
+
+	webhookConfigurationName = "validator.training-operator.kubeflow.org"


IIUC, we need to refine the webhook controller-gen marker as well:

training-operator/pkg/webhooks/pytorch/pytorchjob_webhook.go

Line 51 in 6965c1a

// +kubebuilder:webhook:path=/validate-kubeflow-org-v1-pytorchjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=pytorchjobs,verbs=create;update,versions=v1,name=validator.pytorchjob.training-operator.kubeflow.org,admissionReviewVersions=v1

I don't think we should change it, since those parameters defines the particular webhook configuration, not the name of validation webhook configuration object. For example:

training-operator/manifests/v2/base/webhook/manifests.yaml

Line 35 in 4bab32a

name: validator.trainingruntime.kubeflow.org

Ah, you're right.
This will be used in

- op: replace path: /metadata/name value: validator.training-operator-v2.kubeflow.org

tenzen-y · 2024-10-18T21:01:33Z

cmd/training-operator.v2alpha1/main.go

@@ -44,6 +44,10 @@ import (
 	webhookv2 "github.com/kubeflow/training-operator/pkg/webhook.v2"
 )

+const (
+	webhookConfigurationName = "validator.training-operator-v2.kubeflow.org"


ditto:

training-operator/pkg/webhook.v2/trainjob_webhook.go

Line 42 in 6965c1a

// +kubebuilder:webhook:path=/validate-kubeflow-org-v2alpha1-trainjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=kubeflow.org,resources=trainjobs,verbs=create;update,versions=v2alpha1,name=validator.trainjob.kubeflow.org,admissionReviewVersions=v1

tenzen-y · 2024-10-18T21:02:26Z

manifests/v2/base/manager/manager.yaml

+metadata:
+  name: training-operator-v2
+  labels:
+    training.kubeflow.org/component: manager


Instead of these, could we use the kubernetes recommended labels?

https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y

Thanks!
/lgtm
/approve

google-oss-prow · 2024-10-21T19:16:09Z

[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

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

andreyvelich · 2024-10-21T19:32:28Z

/hold cancel

* KEP-2170: Add manifests for Kubeflow Training V2 Signed-off-by: Andrey Velichkevich <[email protected]> * Fix invalid name for webhook config in cert Signed-off-by: Andrey Velichkevich <[email protected]> * Fix integration tests Signed-off-by: Andrey Velichkevich <[email protected]> * Move kubebuilder markers to runtime framework Signed-off-by: Andrey Velichkevich <[email protected]> * Use Kubernetes recommended labels Signed-off-by: Andrey Velichkevich <[email protected]> --------- Signed-off-by: Andrey Velichkevich <[email protected]>

* KEP-2170: Add manifests for Kubeflow Training V2 Signed-off-by: Andrey Velichkevich <[email protected]> * Fix invalid name for webhook config in cert Signed-off-by: Andrey Velichkevich <[email protected]> * Fix integration tests Signed-off-by: Andrey Velichkevich <[email protected]> * Move kubebuilder markers to runtime framework Signed-off-by: Andrey Velichkevich <[email protected]> * Use Kubernetes recommended labels Signed-off-by: Andrey Velichkevich <[email protected]> --------- Signed-off-by: Andrey Velichkevich <[email protected]> Signed-off-by: sailesh duddupudi <[email protected]>

andreyvelich added 2 commits October 17, 2024 23:31

KEP-2170: Add manifests for Kubeflow Training V2

647abba

Signed-off-by: Andrey Velichkevich <[email protected]>

Fix invalid name for webhook config in cert

6a58c51

Signed-off-by: Andrey Velichkevich <[email protected]>

google-oss-prow bot added the do-not-merge/hold label Oct 17, 2024

google-oss-prow bot requested review from jinchihe and kuizhiqing October 17, 2024 23:13

google-oss-prow bot added the size/L label Oct 17, 2024

Fix integration tests

4bab32a

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y reviewed Oct 18, 2024

View reviewed changes

Move kubebuilder markers to runtime framework

cc72871

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich force-pushed the issue-2208-v2-manifests branch from 97663fe to cc72871 Compare October 19, 2024 00:20

Use Kubernetes recommended labels

1e24175

Signed-off-by: Andrey Velichkevich <[email protected]>

tenzen-y reviewed Oct 21, 2024

View reviewed changes

google-oss-prow bot assigned tenzen-y Oct 21, 2024

google-oss-prow bot added the lgtm label Oct 21, 2024

google-oss-prow bot added the approved label Oct 21, 2024

google-oss-prow bot removed the do-not-merge/hold label Oct 21, 2024

google-oss-prow bot merged commit fd4d102 into kubeflow:master Oct 21, 2024
41 checks passed

andreyvelich deleted the issue-2208-v2-manifests branch October 21, 2024 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Add manifests for Kubeflow Training V2 #2289

KEP-2170: Add manifests for Kubeflow Training V2 #2289

andreyvelich commented Oct 17, 2024

coveralls commented Oct 18, 2024 •

edited

Loading

tenzen-y left a comment

tenzen-y Oct 18, 2024

tenzen-y Oct 18, 2024

tenzen-y Oct 18, 2024

andreyvelich Oct 19, 2024

tenzen-y Oct 21, 2024

tenzen-y Oct 18, 2024

tenzen-y Oct 18, 2024

tenzen-y left a comment

google-oss-prow bot commented Oct 21, 2024

andreyvelich commented Oct 21, 2024

		//+kubebuilder:rbac:groups=jobset.x-k8s.io,resources=jobsets,verbs=get;list;watch;create
		//+kubebuilder:rbac:groups=scheduling.x-k8s.io,resources=podgroups,verbs=get;list;watch;create

		//+kubebuilder:rbac:groups=kubeflow.org,resources=trainingruntimes,verbs=get;list;watch
		//+kubebuilder:rbac:groups=kubeflow.org,resources=clustertrainingruntimes,verbs=get;list;watch

KEP-2170: Add manifests for Kubeflow Training V2 #2289

KEP-2170: Add manifests for Kubeflow Training V2 #2289

Conversation

andreyvelich commented Oct 17, 2024

coveralls commented Oct 18, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11413026138

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Oct 18, 2024

Choose a reason for hiding this comment

tenzen-y Oct 18, 2024

Choose a reason for hiding this comment

tenzen-y Oct 18, 2024

Choose a reason for hiding this comment

andreyvelich Oct 19, 2024

Choose a reason for hiding this comment

tenzen-y Oct 21, 2024

Choose a reason for hiding this comment

tenzen-y Oct 18, 2024

Choose a reason for hiding this comment

tenzen-y Oct 18, 2024

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 21, 2024

andreyvelich commented Oct 21, 2024

coveralls commented Oct 18, 2024 •

edited

Loading