KEP-2170: Adding CEL validations on v2 TrainJob CRD #2260

akshaychitneni · 2024-09-16T22:38:34Z

What this PR does / why we need it:
This PR relates to #2209 adding CEL validations on TrainJob CRD. I will followup with validations implemented in webhook in separate PR

cc @andreyvelich @tenzen-y

google-oss-prow · 2024-09-16T22:38:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

tenzen-y

Thanks for doing this.
I left my first feedback.

tenzen-y · 2024-09-17T13:21:36Z

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

Could you implement integration testing if these validations should work?
We can implement those tests in https://github.com/kubeflow/training-operator/tree/126110fd4d76439bd04ca9fdf96bafb7ea3b6910/test/integration/webhook.v2.

coveralls · 2024-09-17T13:22:37Z

Pull Request Test Coverage Report for Build 11003550343

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on cel-crd at 100.0%

Totals
Change from base Build 11001381410:	100.0%
Covered Lines:	66
Relevant Lines:	66

💛 - Coveralls

tenzen-y · 2024-09-17T13:23:34Z

/hold

tenzen-y · 2024-09-17T13:32:17Z

Additionally, could you sign DCO?

Signed-off-by: Akshay Chitneni <achitneni@apple.com>

andreyvelich · 2024-09-24T15:40:14Z

/ok-to-test
/rerun-all

andreyvelich · 2024-09-30T22:30:23Z

/assign @saileshd1402 @varshaprasad96

google-oss-prow · 2024-09-30T22:30:26Z

@andreyvelich: GitHub didn't allow me to assign the following users: saileshd1402, varshaprasad96.

Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @saileshd1402 @varshaprasad96

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andreyvelich

Thank you for this @akshaychitneni!
I left my initial comments.
/assign @kubeflow/wg-training-leads

andreyvelich · 2024-10-01T14:56:09Z

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

@@ -56,6 +56,7 @@ type TrainJobList struct {
 }

 // TrainJobSpec represents specification of the desired TrainJob.
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.managedBy) || has(self.managedBy)", message="ManagedBy is required once set"


Why do we need this ?

This is a type scoped rule, making sure that it is not removed once set. Not sure if this is necessary as a default is being set.

Don't we set it here ?

training-operator/pkg/apis/kubeflow.org/v2alpha1/trainjob_types.go

Line 99 in fba853b

// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="ManagedBy value is immutable"

andreyvelich · 2024-10-01T14:58:10Z

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

 	APIGroup *string `json:"apiGroup,omitempty"`

 	// Kind of the runtime being referenced.
 	// It must be one of TrainingRuntime or ClusterTrainingRuntime.
 	// Defaults to ClusterTrainingRuntime.
+	// +kubebuilder:default="ClusterTrainingRuntime"
+	// +kubebuilder:validation:XValidation:rule="self in ['ClusterTrainingRuntime', 'TrainingRuntime']", message="Kind must be ClusterTrainingRuntime or TrainingRuntime if set"


@tenzen-y Any thoughts on this validation given that we want to accept user's defined CRDs in the runtime reference as part of the runtime framework: #2248 ?

andreyvelich · 2024-10-01T14:59:14Z

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

@@ -56,6 +56,7 @@ type TrainJobList struct {
 }

 // TrainJobSpec represents specification of the desired TrainJob.
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.managedBy) || has(self.managedBy)", message="ManagedBy is required once set"
 type TrainJobSpec struct {


@akshaychitneni Do we want to add validations/defaults for other pars of TrainJob (e.g. Trainer, DatasetConfig, ModelConfig) as part of this PR ?

andreyvelich · 2024-10-01T15:01:02Z

test/integration/cel.v2/trainjob_crd_test.go

@@ -0,0 +1,155 @@
+package cel_v2


I think, we can add those integration tests as part of /test/integration/trainjob_controller_test.go, similar to JobSet: https://github.com/kubernetes-sigs/jobset/blob/main/test/integration/controller/jobset_controller_test.go#L49
WDYT @akshaychitneni @tenzen-y ?

andreyvelich · 2024-10-01T15:09:28Z

test/integration/cel.v2/trainjob_crd_test.go

+			apiGroup := "kubeflow.org"
+			kind := "TrainingRuntime"


Let's create a new module constants under /pkg/constants/constants.go, that we will use for common constants like: Kind, APIGroup, etc. We will use them in different places.
WDYT @tenzen-y @akshaychitneni ?

Do we want to add these under respective APIs groupversion_info.go instead, to make sure the import paths are cleaner while calling these constants for both the API versions?

Sure! I think, the GroupVersion is already set here: https://github.com/kubeflow/training-operator/blob/master/pkg/apis/kubeflow.org/v2alpha1/groupversion_info.go#L29, but not the Kind.
@varshaprasad96 @tenzen-y Where do you think we should put the Kind constants ?

It would be good to have Kind defined in there too.

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

google-oss-prow bot added the size/M label Sep 16, 2024

tenzen-y reviewed Sep 17, 2024

View reviewed changes

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

akshaychitneni force-pushed the cel-crd branch from 5c876bb to 2b97162 Compare September 23, 2024 22:32

google-oss-prow bot added size/L and removed size/M labels Sep 23, 2024

Adding cel validations on trainjob crd

fba853b

Signed-off-by: Akshay Chitneni <achitneni@apple.com>

akshaychitneni force-pushed the cel-crd branch from 2b97162 to fba853b Compare September 23, 2024 22:39

google-oss-prow bot added the ok-to-test label Sep 24, 2024

andreyvelich changed the title ~~Adding CEL validations on v2 TrainJob CRD~~ KEP-2170: Adding CEL validations on v2 TrainJob CRD Sep 30, 2024

andreyvelich reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2170: Adding CEL validations on v2 TrainJob CRD #2260

KEP-2170: Adding CEL validations on v2 TrainJob CRD #2260

akshaychitneni commented Sep 16, 2024 •

edited

Loading

google-oss-prow bot commented Sep 16, 2024

tenzen-y left a comment

tenzen-y Sep 17, 2024

coveralls commented Sep 17, 2024 •

edited

Loading

tenzen-y commented Sep 17, 2024

tenzen-y commented Sep 17, 2024

andreyvelich commented Sep 24, 2024

andreyvelich commented Sep 30, 2024

google-oss-prow bot commented Sep 30, 2024

andreyvelich left a comment

andreyvelich Oct 1, 2024

varshaprasad96 Oct 2, 2024 •

edited

Loading

andreyvelich Oct 2, 2024

andreyvelich Oct 1, 2024

andreyvelich Oct 1, 2024

andreyvelich Oct 1, 2024

andreyvelich Oct 1, 2024

varshaprasad96 Oct 2, 2024 •

edited

Loading

andreyvelich Oct 2, 2024

varshaprasad96 Oct 2, 2024

KEP-2170: Adding CEL validations on v2 TrainJob CRD #2260

Are you sure you want to change the base?

KEP-2170: Adding CEL validations on v2 TrainJob CRD #2260

Conversation

akshaychitneni commented Sep 16, 2024 • edited Loading

google-oss-prow bot commented Sep 16, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 17, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11003550343

Details

💛 - Coveralls

tenzen-y commented Sep 17, 2024

tenzen-y commented Sep 17, 2024

andreyvelich commented Sep 24, 2024

andreyvelich commented Sep 30, 2024

google-oss-prow bot commented Sep 30, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Oct 2, 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

varshaprasad96 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaychitneni commented Sep 16, 2024 •

edited

Loading

coveralls commented Sep 17, 2024 •

edited

Loading

varshaprasad96 Oct 2, 2024 •

edited

Loading

varshaprasad96 Oct 2, 2024 •

edited

Loading