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

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 11412640672

Details

0 of 0 changed or added relevant lines in 0 files 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 · 2024-09-17T13:23:34Z

/hold

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

Additionally, could you sign DCO?

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"

Ack. I agree this is a rule that covers removal which need not be added here as we always set a default value. Updated PR. Thanks

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

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 I was planning to cover other validations in webhook as they seem to complicated for CEL validation. For example validating dataset config with rule "training runtime must have the dataset-initializer container in the Initializer Job" require accessing referenced trainingruntime object. Let me know other wise.

Let's revisit the discussion of which validations we should implement after webhook validations are implemented.

test/integration/cel.v2/trainjob_crd_test.go

tenzen-y · 2024-10-17T20:06:25Z

Sorry for the late response.
I will revisit here and start reviewing tomorrow.

tenzen-y

Thank you!
Basically, lgtm

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

test/integration/controller.v2/trainjob_controller_test.go

tenzen-y · 2024-10-18T20:47:20Z

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 {


Let's revisit the discussion of which validations we should implement after webhook validations are implemented.

test/integration/cel.v2/trainjob_crd_test.go

tenzen-y · 2024-10-18T20:49:55Z

test/integration/controller.v2/trainjob_controller_test.go

@@ -71,4 +73,120 @@ var _ = ginkgo.Describe("TrainJob controller", ginkgo.Ordered, func() {
 			gomega.Expect(k8sClient.Create(ctx, trainJob)).Should(gomega.Succeed())
 		})
 	})
+
+	ginkgo.When("TrainJob CR Validation", func() {


This is just comment. We will revisit the directory structure discussion after we implement the webhook validations.

test/integration/controller.v2/trainjob_controller_test.go

tenzen-y

Thank you for updating!
/lgtm
/approve

tenzen-y · 2024-10-18T21:19:39Z

@andreyvelich Do you want to recheck this PR?

andreyvelich

Thank you for this @akshaychitneni!
Just a few small comments

test/integration/controller.v2/trainjob_controller_test.go

andreyvelich · 2024-10-18T22:55:13Z

test/integration/controller.v2/trainjob_controller_test.go

+				ManagedBy:  &managedBy,
+			}
+			trainJob := &kubeflowv2.TrainJob{
+				TypeMeta: metav1.TypeMeta{


@tenzen-y @akshaychitneni Is this TypeMeta required for ginkgo if we use the TrainJob struct ?

I believe it is needed to infer group/version for create api

Signed-off-by: Akshay Chitneni <[email protected]>

andreyvelich

Thank you for this contribution @akshaychitneni 🎉
/lgtm
/approve

google-oss-prow · 2024-10-19T00:02:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, 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~~ [andreyvelich,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-19T00:03:04Z

/hold cancel

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

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

akshaychitneni force-pushed the cel-crd branch 5 times, most recently from 9d739ea to 4299515 Compare October 17, 2024 19:45

akshaychitneni force-pushed the cel-crd branch from 4299515 to 54ee0b8 Compare October 18, 2024 20:07

tenzen-y reviewed Oct 18, 2024

View reviewed changes

akshaychitneni force-pushed the cel-crd branch 2 times, most recently from 8e4291f to b580661 Compare October 18, 2024 21:03

tenzen-y reviewed Oct 18, 2024

View reviewed changes

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

google-oss-prow bot added lgtm approved labels Oct 18, 2024

andreyvelich reviewed Oct 18, 2024

View reviewed changes

akshaychitneni mentioned this pull request Oct 18, 2024

Add strict validation on error messages in tests for v2 APIs #2293

Closed

Adding cel validations on trainjob crd

8258232

Signed-off-by: Akshay Chitneni <[email protected]>

akshaychitneni force-pushed the cel-crd branch from b580661 to 8258232 Compare October 18, 2024 23:59

google-oss-prow bot removed the lgtm label Oct 18, 2024

andreyvelich approved these changes Oct 19, 2024

View reviewed changes

google-oss-prow bot assigned andreyvelich Oct 19, 2024

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

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

google-oss-prow bot merged commit 0149eb0 into kubeflow:master Oct 19, 2024
40 of 41 checks passed

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

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

akshaychitneni Oct 16, 2024

andreyvelich Oct 1, 2024

akshaychitneni Oct 16, 2024

tenzen-y Oct 18, 2024

tenzen-y commented Oct 17, 2024

tenzen-y left a comment

tenzen-y Oct 18, 2024

tenzen-y Oct 18, 2024

tenzen-y left a comment

tenzen-y commented Oct 18, 2024

andreyvelich left a comment

andreyvelich Oct 18, 2024

akshaychitneni Oct 18, 2024 •

edited

Loading

andreyvelich left a comment

google-oss-prow bot commented Oct 19, 2024

andreyvelich commented Oct 19, 2024

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

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

Conversation

akshaychitneni commented Sep 16, 2024 • edited Loading

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 11412640672

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

tenzen-y commented Oct 17, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y commented Oct 18, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaychitneni Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 19, 2024

andreyvelich commented Oct 19, 2024

akshaychitneni commented Sep 16, 2024 •

edited

Loading

coveralls commented Sep 17, 2024 •

edited

Loading

varshaprasad96 Oct 2, 2024 •

edited

Loading

akshaychitneni Oct 18, 2024 •

edited

Loading