KEP-2170: Generate CRD manifests for v2 CustomResources #2237

tenzen-y · 2024-08-27T23:07:53Z

What this PR does / why we need it:
I generated CRD manifests for v2 CustomResources because we need CRD manifests in the first implementation PR: #2236.
Since this PR, the manifests directory looks like the following:

- /
  - /manifests
    - /manifests/v2/base/crds 
    - /manifests/base             # for v1
    - /manifests/overlays       # for v1

Additionally, I added some kubebuilder markers to generate CRDs.

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: #2208

Checklist:

Docs included if any changes are user facing

coveralls · 2024-08-27T23:11:59Z

Pull Request Test Coverage Report for Build 10638979820

Details

0 of 0 changed or added relevant lines in 0 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.06%) to 31.753%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller.v1/mpi/mpijob.go	1	91.06%
pkg/controller.v1/mpi/mpijob_controller.go	6	80.67%

Totals
Change from base Build 10637686351:	-0.06%
Covered Lines:	3944
Relevant Lines:	12421

💛 - Coveralls

tenzen-y · 2024-08-27T23:27:05Z

/assign @kubeflow/wg-training-leads

tenzen-y · 2024-08-28T01:35:02Z

/hold

kannon92 · 2024-08-28T14:28:21Z

Makefile

@@ -38,9 +38,11 @@ help: ## Display this help.

 manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.


This could be a followup but I have found it useful to have some check in the CI to make sure external contributors are not commiting an API change without generating the manifests.

Indeed, we already checked in our CI.

training-operator/.github/workflows/test-go.yaml

Lines 32 to 35 in ea5272f

- name: Check manifests

run: |

make manifests && git add manifests &&

git diff --cached --exit-code || (echo 'Please run "make manifests" to generate manifests' && exit 1);

andreyvelich · 2024-08-30T13:39:29Z

@tenzen-y What do you think about this structure ?

manifests/v1/base
manifests/v1/overlays
manifests/v2/base
manifests/v2/overlays

Similar to Katib: https://github.com/kubeflow/katib/tree/master/manifests/v1beta1 ?

Also, are we planning to maintain and deploy both CRDs for v2alpha1 and v2beta1 in the future ?

andreyvelich

Thanks for this @tenzen-y!
/assign @kubeflow/wg-training-leads @shravan-achar

andreyvelich · 2024-08-30T13:43:23Z

manifests/base/v2alpha1/crds/kubeflow.org_clustertrainingruntimes.yaml

@@ -0,0 +1,9275 @@
+---


I am wondering are what are advantages for the CEL validation if we have validation webhook ?
I noticed that if CRD becomes to large it can cause issues. For example: kserve/kserve#3487
cc @terrytangyuan @kubeflow/wg-training-leads

That's not a problem. We can avoid the problem with --server-side option.
We should introduce the CEL validation to reduce Kube-API server loads.

In the big cluster, many webhook validations are a big deal for performance issues. Specifically, kube-apiserver and etcd are so busy in batch clusters with hundreds and thousands of Jobs.

For UX, CEL validation could be reduce kubectl / SDK response time since the kube-apiserver does not need to call external webhook servers, the kubeapi-server could just evaluate incoming Job objects inside of kube-apiserver.

Make sense! Let's see if our CRD can go too-large even with server side option.

andreyvelich · 2024-08-30T13:45:49Z

pkg/apis/kubeflow.org/v2alpha1/trainingruntime_types.go

@@ -23,6 +23,8 @@ import (
 )

 // +kubebuilder:object:root=true
+// +kubebuilder:storageversion


Do we need storageversion for CRD version migrations in the future ?

Yes, I guess that we need this storageVersion for the situation where we switched to v2apha2 or v2beta1.

tenzen-y · 2024-08-30T17:06:44Z

@tenzen-y What do you think about this structure ?
manifests/v1/base
manifests/v1/overlays
manifests/v2/base
manifests/v2/overlays
Similar to Katib: https://github.com/kubeflow/katib/tree/master/manifests/v1beta1 ?

Also, are we planning to maintain and deploy both CRDs for v2alpha1 and v2beta1 in the future ?

I don't think that we should support both v2alpha1 and other v2 versions. So, let's rename v2alpha1 with v2.

For the directory structure, The directory structure looks good to me.
But, I wouldn't modify the overlays directory in this PR.
I believe that we should work on entirely manifests creation and move user facing directory changes (overlays) so that users can easily observe the change of installation way.

andreyvelich · 2024-08-30T17:43:09Z

I believe that we should work on entirely manifests creation and move user facing directory changes (overlays) so that users can easily observe the change of installation way.

In that case, how are going to support deployment/overlay of v1 and v2 version ?

tenzen-y · 2024-08-30T17:47:46Z

I believe that we should work on entirely manifests creation and move user facing directory changes (overlays) so that users can easily observe the change of installation way.

In that case, how are going to support deployment/overlay of v1 and v2 version ?

Sorry, I accidentally sent the message on the way...
I meant we want to introduce the directory structure you proposed there: #2208

tenzen-y · 2024-08-30T17:54:51Z

I renamed v2alpha1 with v2.

andreyvelich · 2024-08-30T18:30:58Z

manifests/overlays/kubeflow/kustomization.yaml

@@ -2,7 +2,7 @@ apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 namespace: kubeflow
 resources:
-  - ../../base
+  - ../../base/v1


@tenzen-y So why you don't want to separate overlays between v1 and v2 version ?

I want to prepare /manifests/v2 and /manifests/v1 when we create manifests for v2.
Because we want to mitigate the impact for the existing v1 users who use the master branch.

In that case, in this PR, we may want to just add the /manifests/v2 directory, and then we can keep the existing v1 manifests directory until we introduce the v2 manifests. WDYT?

I see, that makes sense, since it will break any users' GitOps/CD that might be configured on the master branch for Training Operator installation.
In that case, are we planning to introduce /manifests/overlays/v2 in the future ?

If you want we can move base + overlays to the v2 folder all together and don't touch v1 at all:

/manifests/v2/base/crds /manifests/v2/base/controller /manifests/v2/overlays/standalone /manifests/v2/overlays/kubeflow

If you want we can move base + overlays to the v2 folder all together and don't touch v1 at all:

/manifests/v2/base/crds /manifests/v2/base/controller /manifests/v2/overlays/standalone /manifests/v2/overlays/kubeflow

Yes, I want to select the approach. Let's align this PR with the directory structure.

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y · 2024-08-30T20:52:54Z

@andreyvelich I updated this PR, PTAL. Thanks.

andreyvelich

/lgtm
/approve

google-oss-prow · 2024-08-30T20:59:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, shravan-achar

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-08-30T21:27:43Z

/hold cancel

google-oss-prow bot requested review from jinchihe and kuizhiqing August 27, 2024 23:07

google-oss-prow bot added the size/XXL label Aug 27, 2024

tenzen-y mentioned this pull request Aug 27, 2024

KEP-2170: Initial Implementations for v2 Manager #2236

Merged

1 task

tenzen-y force-pushed the geenrate-crds-v2 branch from e762c35 to 33f0d4b Compare August 27, 2024 23:48

tenzen-y changed the title ~~KEP-2170: Generate CRDs for v2 CRDs~~ KEP-2170: Generate CRD manifests for v2 CRDs Aug 28, 2024

tenzen-y changed the title ~~KEP-2170: Generate CRD manifests for v2 CRDs~~ KEP-2170: Generate CRD manifests for v2 CustomResources Aug 28, 2024

google-oss-prow bot added the do-not-merge/hold label Aug 28, 2024

tenzen-y force-pushed the geenrate-crds-v2 branch from 33f0d4b to efa6c8d Compare August 28, 2024 04:53

kannon92 reviewed Aug 28, 2024

View reviewed changes

andreyvelich reviewed Aug 30, 2024

View reviewed changes

shravan-achar approved these changes Aug 30, 2024

View reviewed changes

andreyvelich reviewed Aug 30, 2024

View reviewed changes

tenzen-y force-pushed the geenrate-crds-v2 branch 3 times, most recently from f43bbce to 46bd6e5 Compare August 30, 2024 20:43

tenzen-y added 2 commits August 31, 2024 05:47

KEP-2170: Generate CRDs for v2 CRDs

cc752b9

Signed-off-by: Yuki Iwai <[email protected]>

Replace v2alpha with v2

a5d1ed0

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y force-pushed the geenrate-crds-v2 branch from 46bd6e5 to a5d1ed0 Compare August 30, 2024 20:48

andreyvelich reviewed Aug 30, 2024

View reviewed changes

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

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

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

google-oss-prow bot removed the do-not-merge/hold label Aug 30, 2024

google-oss-prow bot merged commit 2455f7d into kubeflow:master Aug 30, 2024
39 checks passed

tenzen-y deleted the geenrate-crds-v2 branch August 30, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Generate CRD manifests for v2 CustomResources #2237

KEP-2170: Generate CRD manifests for v2 CustomResources #2237

tenzen-y commented Aug 27, 2024 •

edited

Loading

coveralls commented Aug 27, 2024 •

edited

Loading

tenzen-y commented Aug 27, 2024

tenzen-y commented Aug 28, 2024

kannon92 Aug 28, 2024

tenzen-y Aug 28, 2024

andreyvelich commented Aug 30, 2024

andreyvelich left a comment

andreyvelich Aug 30, 2024

tenzen-y Aug 30, 2024

andreyvelich Aug 30, 2024

andreyvelich Aug 30, 2024

tenzen-y Aug 30, 2024

tenzen-y commented Aug 30, 2024

andreyvelich commented Aug 30, 2024

tenzen-y commented Aug 30, 2024

tenzen-y commented Aug 30, 2024

andreyvelich Aug 30, 2024

tenzen-y Aug 30, 2024

andreyvelich Aug 30, 2024

andreyvelich Aug 30, 2024

tenzen-y Aug 30, 2024

tenzen-y commented Aug 30, 2024

andreyvelich left a comment

google-oss-prow bot commented Aug 30, 2024

tenzen-y commented Aug 30, 2024

		@@ -38,9 +38,11 @@ help: ## Display this help.

		manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.

	- name: Check manifests
	run: \|
	make manifests && git add manifests &&
	git diff --cached --exit-code \|\| (echo 'Please run "make manifests" to generate manifests' && exit 1);

KEP-2170: Generate CRD manifests for v2 CustomResources #2237

KEP-2170: Generate CRD manifests for v2 CustomResources #2237

Conversation

tenzen-y commented Aug 27, 2024 • edited Loading

coveralls commented Aug 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10638979820

Details

💛 - Coveralls

tenzen-y commented Aug 27, 2024

tenzen-y commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Aug 30, 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

tenzen-y commented Aug 30, 2024

andreyvelich commented Aug 30, 2024

tenzen-y commented Aug 30, 2024

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

tenzen-y commented Aug 30, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 30, 2024

tenzen-y commented Aug 30, 2024

tenzen-y commented Aug 27, 2024 •

edited

Loading

coveralls commented Aug 27, 2024 •

edited

Loading