KEP-2170: Make API specification more restricting #2198

tenzen-y · 2024-08-07T19:45:51Z

What this PR does / why we need it:
I updated the below API definitions for the Training v2 API:

trainingRuntimeRef
1. I removed the namespace field since we should use the TrainingRuntime deployed in the same namespace with the TrainJob. Otherwise, users should define the ClusterTrainingRuntime instead of the namespace-scoped TrainingRuntime resource.
2. I renamed the version field with apiVersion since we are sometimes confused if the version indicates the runtime revision or API version.
managedBy
1. I added limitations and restrictions based on discussions in Add support for the managedBy field #2193. The managedBy field is acceptable only for either an empty value, "training-operator.kubeflow.org/trainjob-controller" or "kueue.x-k8s.io/multikueue".

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

Checklist:

Docs included if any changes are user facing

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

tenzen-y · 2024-08-07T19:46:19Z

/assign @kubeflow/wg-training-leads

coveralls · 2024-08-07T19:50:25Z

Pull Request Test Coverage Report for Build 10307520952

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.008%) to 33.494%

Totals
Change from base Build 10284610088:	0.008%
Covered Lines:	3951
Relevant Lines:	11796

💛 - Coveralls

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

tenzen-y · 2024-08-07T19:55:18Z

cc: @mimowo

tenzen-y · 2024-08-07T19:59:06Z

docs/proposals/2170-kubeflow-training-v2/README.md

-	// Namespace for the runtime. In that case, user should use TrainingRuntime
-	Namespace *string `json:"namespace,omitempty"`
+	// APIVersion is the apiVersion for the runtime.
+	// Defaults to the v2alpha1.


Suggested change

// Defaults to the v2alpha1.

// Defaults to the kubeflow.org/v2alpha1.

I'm wondering if we should align with the YAML apiVersion like this.

APIVersion would indeed look more consistent with how objects are declared in yamls example. OTOH consider if you need a version at all - will you support other versions than the current? Maybe just APIGroup is enough.

@tenzen-y Do we have plans to support different runtime version APIs in TrainJob ? e.g. v2alpha1 and v2beta1

I'm assuming that we need to support multiple versions of TrainingRuntimes when we graduate it from Beta to Stable so that we align with the Kubernetes deprecation policy.

But, I don't think that we need to support multiple versions when we graduate the API from alpha to beta.
So, I'm wondering if we should drop this version field in the alpha and beta stages, and then if we should revisit this discussion to mitigate the GA graduation risks.

@andreyvelich @mimowo WDYT?

Agree, let's do it in the future versions.
What are the benefits you see to add the APIGroup in the runtime reference if that will be always kubeflow.org ?
To give users possibility to build custom APIs for the training runtimes ?

What are the benefits you see to add the APIGroup in the runtime reference if that will be always kubeflow.org ?
To give users possibility to build custom APIs for the training runtimes ?

Yes, once we provide the possibility to specify the apiGroup, vendors and users can implement the CRD instead of TrainingRuntime/ClusterTrainingRuntime and custom controller to build Jobs.

So, I'm thinking that adding the apiGroup in the first iteration would be worth it. @andreyvelich WDYT?

@andreyvelich Oh, I actually still have not added the apiGroup field in the KEP.
So, based on their reactions, I opened the PR to add the apiGroup in the API design.

oh, you are right. Let's add it in the followup PR.

mimowo

Thanks for the update!

mimowo · 2024-08-08T06:50:07Z

docs/proposals/2170-kubeflow-training-v2/README.md

-	// Namespace for the runtime. In that case, user should use TrainingRuntime
-	Namespace *string `json:"namespace,omitempty"`
+	// APIVersion is the apiVersion for the runtime.
+	// Defaults to the v2alpha1.


APIVersion would indeed look more consistent with how objects are declared in yamls example. OTOH consider if you need a version at all - will you support other versions than the current? Maybe just APIGroup is enough.

mimowo · 2024-08-08T06:50:10Z

docs/proposals/2170-kubeflow-training-v2/README.md

+	// 'kueue.x-k8s.io/multikueue'.
+	// The built-in TrainJob controller reconciles TrainJob which don't have this
+	// field at all or the field value is the reserved string
+	// 'training-operator.kubeflow.org/trainjob-controller', but delegates reconciling TrainJobs


training-operator.kubeflow.org/trainjob-controller is a bit long. What about the alternative: kubeflow.org/training-operator-v2?

So, to match the naming of v1 we tentatively go with for v1: kubeflow.org/training-operator (#2193)?

Maybe we should just do kubeflow.org/training-operator or kubeflow.org/trainjob-controller for V2?
Since we are planning to support only managedBy: kueue.x-k8s.io/multikueue in V1 version: #2193 (comment)

Actually, we could probably use kubeflow.org/training-operator both for v1 and v2. It is non-ambigues since API v1 can only be reconciled by the controller in the same version.

TBH, I'm ok with either name kubeflow.org/training-operator or training-operator.kubeflow.org/trainjob-controller because I just wanted to align with the managedBy specifications in the batch/job and jobset.

~~In the batch/job and jobset, we use the ${OPERATOR_NAME}.${API_GROUP}/${CONTROLLER_NAME} like jobset.sigs.k8s.io/jobset-controller and kubernetes.io/job-controller.~~

~~@mimowo Do you know the reason why we selected the above managedBy format in the batch/job and jobset?~~

Oh, I found the jobset and batch/job actually use the ${API_GROUP}/${CONTROLLER_NAME}.
So, never mind the above my comment.

So, let's use the kubeflow.org/training-operator as a reserved name so that we can align with batch/job and jobset.

EDIT: Actually, we reconcile the TrainJob by trainjob-controller instead of the training-opertor, and then the training-operator v2 contains multiple controllers like trainjob-controller and trainingruntime-controller.
So, I updated this section with kubeflow.org/trainjob-controller. If you have any suggestions, please let me know.

Yeah, maybe we should use the trainjob-controller to make it consistent with the CRD name, JobSet is doing the same: https://github.com/kubernetes-sigs/jobset/blob/main/api/jobset/v1alpha2/jobset_types.go#L46

andreyvelich · 2024-08-08T11:58:40Z

docs/proposals/2170-kubeflow-training-v2/README.md

 	Name string `json:"name"`

-	// Namespace for the runtime. In that case, user should use TrainingRuntime
-	Namespace *string `json:"namespace,omitempty"`


Agree with that!

andreyvelich · 2024-08-08T11:59:40Z

docs/proposals/2170-kubeflow-training-v2/README.md

-	// Namespace for the runtime. In that case, user should use TrainingRuntime
-	Namespace *string `json:"namespace,omitempty"`
+	// APIVersion is the apiVersion for the runtime.
+	// Defaults to the v2alpha1.


@tenzen-y Do we have plans to support different runtime version APIs in TrainJob ? e.g. v2alpha1 and v2beta1

andreyvelich · 2024-08-08T12:00:31Z

docs/proposals/2170-kubeflow-training-v2/README.md


-	// Kind for the runtime. TrainingRuntime or ClusterTrainingRuntime
+	// Kind for the runtime, which must be TrainingRuntime or ClusterTrainingRuntime.
+	// Defaults to the TrainingRuntime.


Should we make Defaults to ClusterTrainingRuntime since in OSS we will implement them ?

Yeah, I agree with you.
I try to refine this comment.

andreyvelich · 2024-08-08T12:05:28Z

docs/proposals/2170-kubeflow-training-v2/README.md

 	Kind *string `json:"kind,omitempty"`
-
-	// Version for the runtime. For example: v2alpha1
-	Version *string `json:"version,omitempty"`


Are we going to introduce Revision parameter in the future versions according to this ?
https://github.com/kubeflow/training-operator/blob/82d6fe581bacb0b4961b527f00385ece8fc1311c/docs/proposals/2170-kubeflow-training-v2/README.md#:~:text=In%20the%20future%20implementation%2C%20we%20will%20introduce%20a%20revision%20control%20mechanism%20similar%20to%20Kubernetes%20Deployment%20to%20control%20versions%20of%20TrainingRuntime%20and%20enable%20rolling%20updates.

Yeah, I still think that the runtime revision control would be useful.

…kubeflow.org/trainjob-controller' Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

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

andreyvelich

Thanks for the update @tenzen-y!
/lgtm
/approve

google-oss-prow · 2024-08-09T11:58:02Z

[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 added 2 commits August 8, 2024 02:11

Fix formatting issues

deebfab

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

Make trainingRuntimeRef more clarify

a4872f8

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

google-oss-prow bot requested review from jinchihe and kuizhiqing August 7, 2024 19:45

google-oss-prow bot added the size/M label Aug 7, 2024

tenzen-y force-pushed the update-kep-2170 branch from a9df00f to 300df75 Compare August 7, 2024 19:52

Update the managedBy specifications

82d6fe5

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

tenzen-y force-pushed the update-kep-2170 branch from 300df75 to 82d6fe5 Compare August 7, 2024 19:54

tenzen-y commented Aug 7, 2024

View reviewed changes

mimowo reviewed Aug 8, 2024

View reviewed changes

andreyvelich reviewed Aug 8, 2024

View reviewed changes

tenzen-y added 3 commits August 9, 2024 01:20

Use 'kubeflow.org/trainjob-controller' instead of 'training-operator.…

530b33e

…kubeflow.org/trainjob-controller' Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

The ClusterTrainingRuntime is used in the runtimeRef as a default

153ea1d

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

Move apiVersion for the TrainingRuntime to alternative section

43edaef

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

andreyvelich reviewed Aug 9, 2024

View reviewed changes

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

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

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

google-oss-prow bot merged commit 94140ed into kubeflow:master Aug 9, 2024
38 checks passed

tenzen-y deleted the update-kep-2170 branch August 9, 2024 12:26

tenzen-y mentioned this pull request Aug 9, 2024

KEP-2170: Add the apiGroup to the TrainingRuntimeRef #2201

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Make API specification more restricting #2198

KEP-2170: Make API specification more restricting #2198

tenzen-y commented Aug 7, 2024

tenzen-y commented Aug 7, 2024

coveralls commented Aug 7, 2024 •

edited

Loading

tenzen-y commented Aug 7, 2024

tenzen-y Aug 7, 2024

mimowo Aug 8, 2024

andreyvelich Aug 8, 2024

tenzen-y Aug 8, 2024

andreyvelich Aug 8, 2024

tenzen-y Aug 8, 2024

tenzen-y Aug 9, 2024

andreyvelich Aug 9, 2024

mimowo left a comment

mimowo Aug 8, 2024

mimowo Aug 8, 2024

andreyvelich Aug 8, 2024 •

edited

Loading

mimowo Aug 8, 2024

tenzen-y Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024

andreyvelich Aug 8, 2024

andreyvelich Aug 8, 2024

andreyvelich Aug 8, 2024

andreyvelich Aug 8, 2024

tenzen-y Aug 8, 2024

tenzen-y Aug 8, 2024

andreyvelich Aug 8, 2024

tenzen-y Aug 8, 2024

andreyvelich left a comment

google-oss-prow bot commented Aug 9, 2024

	// Defaults to the v2alpha1.
	// Defaults to the kubeflow.org/v2alpha1.

KEP-2170: Make API specification more restricting #2198

KEP-2170: Make API specification more restricting #2198

Conversation

tenzen-y commented Aug 7, 2024

tenzen-y commented Aug 7, 2024

coveralls commented Aug 7, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10307520952

Details

💛 - Coveralls

tenzen-y commented Aug 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

tenzen-y Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

tenzen-y Aug 8, 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

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 9, 2024

coveralls commented Aug 7, 2024 •

edited

Loading

andreyvelich Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024 •

edited

Loading

tenzen-y Aug 8, 2024 •

edited

Loading