Add apply configurations to generated client #1818

astefanutti · 2024-01-09T13:03:07Z

Why are these changes needed?

This PR generates apply configurations for the Kuberay API client.

Related issue number

Closes #1811.

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

astefanutti · 2024-01-09T18:18:36Z

@kevin85421 the new e2e test, that covers SSA, currently fails because of the creationTimestamp fields from the corev1.PodTemplateSpec fields that are embedded into KubeRay API.

I need to review the possible solutions, but I might request your feedback on this if I fail to find a good solution.

kevin85421 · 2024-01-10T03:35:02Z

@astefanutti Do you mind giving me some keywords that I need to understand in order to have enough context to review this PR? Thanks!

astefanutti · 2024-01-15T19:16:39Z

@kevin85421 you'll find a very good introduction about SSA here https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/.

Here is the breakdown of the changes introduced by this PR:

Adapt update-codegen.sh to generate apply configurations, note generate-groups.sh cannot be used, because it lacks support for external apply configurations (such as PodTemplateSpec), so the code-generator binaries are used directly
Re-generate the client, with the new apply configuration packages
Add an e2e test using SSA

kevin85421 · 2024-01-17T00:10:10Z

ray-operator/test/support/support.go

 )

 var (
+	TestApplyOptions = metav1.ApplyOptions{FieldManager: "kuberay-test", Force: true}


I read this article and it said "Controllers typically should unconditionally set all the fields they own by setting Force: true in the ApplyOptions.". I can't understand what "the fields they own" means. Would you mind explaining why we need to add Force: true and what's the difference with and without Force: true? Thanks!

The Force option is used to control the resolution logic when conflicts occur. The possible strategies are documented at:

https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts

In the case of e2e tests here, the recommendation to use Force: true applies well. The e2e tests impersonate / emulate a real client / end-user, and what the tests specify using SSA apply configurations (e.g. RayCluster / RayJob spec) is what's owned by that client / end-user.

kevin85421 · 2024-01-17T00:30:41Z

ray-operator/test/e2e/support.go

+	}
+}
+
+func headPodTemplateApplyConfiguration() *corev1ac.PodTemplateSpecApplyConfiguration {


It seems we can create a RayJob custom resource by SSA using an applyconfiguration (e.g., headPodTemplateApplyConfiguration()), or we can directly construct the CR using headPodTemplate(). Should we consider gradually updating existing tests to use SSA for creating RayJob CRs in the future?

That would be my suggestion / recommendation too. I find apply configurations fluent API style and SSA conflict management very convenient.

My original intention with the e2e tests was to use SSA, but that needed the work introduced by that PR first. We could gradually update the existing tests as you suggest.

Note the benefits of SSA are also applicable to the operator itself, but that would require more substantial work.

ray-operator/test/support/core.go

kevin85421 · 2024-01-17T00:42:49Z

ray-operator/test/support/meta.go

+
+type labelSelector string
+
+var _ Option[*metav1.ListOptions] = (*labelSelector)(nil)


ChatGPT tells me that *"This line asserts that labelSelector implements the Option interface for metav1.ListOptions. It's a common way in Go to ensure at compile time that a type satisfies an interface.". Is that correct?

It's exactly that :)

kevin85421 · 2024-01-17T00:45:08Z

ray-operator/test/support/meta.go

+
+// nolint: unused
+// To be removed when the false-positivity is fixed.
+func (l labelSelector) applyTo(to *metav1.ListOptions) error {


Would you mind sharing more details about "To be removed when the false-positivity is fixed."? Thanks!

I think the root issue is dominikh/go-tools#1294.

There are still a number of false positive w.r.t. generics support: https://github.com/dominikh/go-tools/issues?q=is%3Aopen+label%3Afalse-positive+generic.

ray-operator/test/support/meta.go

kevin85421

Q1: I haven't reviewed the files under pkg/client. Are they generated files?

I want to double-check the reason why we decided to use applyconfiguration.

There are two field managers, the KubeRay operator and kuberay-test, in e2e tests.
Prior to this PR, we used Update without SSA, but it may have led to some 409 conflicts.
In this PR, we use SSA Apply with Force: true. The Apply request will always succeed.

Is my understanding correct? Thanks!

astefanutti · 2024-01-18T08:20:26Z

Q1: I haven't reviewed the files under pkg/client. Are they generated files?

Yes, these files are generated, as the other ones that already exist in pkg/client.

They are "validated" by the added e2e test.

I want to double-check the reason why we decided to use applyconfiguration.

There are two field managers, the KubeRay operator and kuberay-test, in e2e tests.

Prior to this PR, we used Update without SSA, but it may have led to some 409 conflicts.

In this PR, we use SSA Apply with Force: true. The Apply request will always succeed.

Is my understanding correct? Thanks!

Yes, your understanding is correct 👍🏼.

kevin85421

LGTM. I learned a lot from reviewing this PR. Thank you!

andrewsykim · 2024-01-18T18:24:28Z

FYI I think CI is currently broken because the generated applyconfigurtaion was not updated after this change #1839

is missing. Running 'go mod download'.
>> Using /home/runner/go/pkg/mod/k8s.io/code-generator@v0.28.4
diffing ./hack/../pkg against freshly generated codegen
diff -Naupr ./hack/../pkg/client/applyconfiguration/ray/v1/rayservicestatus.go ./hack/../_tmp/pkg/client/applyconfiguration/ray/v1/rayservicestatus.go
--- ./hack/../pkg/client/applyconfiguration/ray/v1/rayservicestatus.go	2024-01-18 17:47:09.837[5](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:6)[6](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:7)3[7](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:8)13 +0000
+++ ./hack/../_tmp/pkg/client/applyconfiguration/ray/v1/rayservicestatus.go	2024-01-1[8](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:9) 17:46:26.866143286 +0000
@@ -6,6 +6,7 @@ package v1
 // with apply.
 type RayServiceStatusApplyConfiguration struct {
 	Applications     map[string]AppStatusApplyConfiguration `json:"applicationStatuses,omitempty"`
+	DashboardStatus  *DashboardStatusApplyConfiguration     `json:"dashboardStatus,omitempty"`
 	RayClusterName   *string                                `json:"rayClusterName,omitempty"`
 	RayClusterStatus *RayClusterStatusApplyConfiguration    `json:"rayClusterStatus,omitempty"`
 }
@@ -30,6 +31,14 @@ func (b *RayServiceStatusApplyConfigurat
 	return b
 }
 
+// WithDashboardStatus sets the DashboardStatus field in the declarative configuration to the given value
+// and returns the receiver, so that objects can be built by chaining "With" function invocations.
+// If called multiple times, the DashboardStatus field is set to the value of the last call.
+func (b *RayServiceStatusApplyConfiguration) WithDashboardStatus(value *DashboardStatusApplyConfiguration) *RayServiceStatusApplyConfiguration {
+	b.DashboardStatus = value
+	return b
+}
+
 // WithRayClusterName sets the RayClusterName field in the declarative configuration to the given value
 // and returns the receiver, so that objects can be built by chaining "With" function invocations.
 // If called multiple times, the RayClusterName field is set to the value of the last call.
diff -Naupr ./hack/../pkg/client/applyconfiguration/utils.go ./hack/../_tmp/pkg/client/applyconfiguration/utils.go
--- ./hack/../pkg/client/applyconfiguration/utils.go	2024-01-18 17:47:0[9](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:10).849563549 +0000
+++ ./hack/../_tmp/pkg/client/applyconfiguration/utils.go	2024-01-18 17:46:26.866[14](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:15)3286 +0000
@@ -[17](https://github.com/ray-project/kuberay/actions/runs/7573949808/job/20627230668?pr=1822#step:5:18),6 +17,8 @@ func ForKind(kind schema.GroupVersionKin
 		return &rayv1.AppStatusApplyConfiguration{}
 	case v1.SchemeGroupVersion.WithKind("AutoscalerOptions"):
 		return &rayv1.AutoscalerOptionsApplyConfiguration{}
+	case v1.SchemeGroupVersion.WithKind("DashboardStatus"):
+		return &rayv1.DashboardStatusApplyConfiguration{}
 	case v1.SchemeGroupVersion.WithKind("HeadGroupSpec"):
 		return &rayv1.HeadGroupSpecApplyConfiguration{}
 	case v1.SchemeGroupVersion.WithKind("HeadInfo"):
./hack/../pkg is out of date. Please run hack/update-codegen.sh

andrewsykim · 2024-01-18T18:25:23Z

(I'll open a PR to fix it)

andrewsykim · 2024-01-18T18:29:34Z

#1847

andrewsykim · 2024-01-19T02:33:51Z

@astefanutti I've been finding that runing ./hack/update-codegen.sh is really slow. Is that just for me or is it expeted?

astefanutti · 2024-01-19T09:28:14Z

@astefanutti I've been finding that runing ./hack/update-codegen.sh is really slow. Is that just for me or is it expeted?

@andrewsykim it takes around 2 minutes to complete on my setup. How long does that take on your side?

I'm not that surprised, compared to the data points I have from other projects. It may be embedding the PodTemplateSpec struct in multiple places contributes to increasing the generation time. It's a very large struct, which also increases the size of the CRDs significantly.

I don't see anything obvious we could do to optimise that generation time. That'd probably require to look deeper at code-generator. There is kubernetes/code-generator#69 open that could be related as well.

astefanutti force-pushed the pr-08 branch 2 times, most recently from 6590e96 to ae215d5 Compare January 9, 2024 14:18

kevin85421 self-requested a review January 9, 2024 18:03

kevin85421 self-assigned this Jan 9, 2024

astefanutti force-pushed the pr-08 branch 4 times, most recently from e1f4d29 to 6100e89 Compare January 15, 2024 19:11

astefanutti force-pushed the pr-08 branch 11 times, most recently from 2039d4b to 5effdac Compare January 16, 2024 18:40

kevin85421 reviewed Jan 17, 2024

View reviewed changes

astefanutti force-pushed the pr-08 branch from 5effdac to 67cbe90 Compare January 17, 2024 08:49

Add apply configurations to generated client

73cbac5

astefanutti force-pushed the pr-08 branch from 67cbe90 to fcf4a7c Compare January 17, 2024 09:01

astefanutti added 2 commits January 17, 2024 10:11

Add suspend e2e test with Server-Side Apply

168f25b

Fix missing v1 API group name in apply configurations

30925cc

astefanutti force-pushed the pr-08 branch from fcf4a7c to 147a114 Compare January 17, 2024 09:25

Use apply configurations from k8s.io/client-go

17ff130

astefanutti force-pushed the pr-08 branch from 147a114 to 17ff130 Compare January 17, 2024 09:39

kevin85421 approved these changes Jan 18, 2024

View reviewed changes

kevin85421 merged commit 966d9b3 into ray-project:master Jan 18, 2024
24 checks passed

astefanutti deleted the pr-08 branch January 18, 2024 16:42

andrewsykim mentioned this pull request Jan 18, 2024

fix applyconfiguration generated code #1847

Merged

4 tasks

andrewsykim mentioned this pull request Jan 26, 2024

Remove generate target from build/test targets #1874

Merged

4 tasks

Yicheng-Lu-llll mentioned this pull request Feb 9, 2024

Refactor Current e2e Test to Use Server-Side Apply #1917

Closed

2 tasks

ryanaoleary pushed a commit to ryanaoleary/kuberay that referenced this pull request Feb 13, 2024

Add apply configurations to generated client (ray-project#1818)

e897aac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add apply configurations to generated client #1818

Add apply configurations to generated client #1818

astefanutti commented Jan 9, 2024

astefanutti commented Jan 9, 2024

kevin85421 commented Jan 10, 2024

astefanutti commented Jan 15, 2024

kevin85421 Jan 17, 2024

astefanutti Jan 17, 2024

kevin85421 Jan 17, 2024

astefanutti Jan 17, 2024

kevin85421 Jan 17, 2024

astefanutti Jan 17, 2024

kevin85421 Jan 17, 2024

astefanutti Jan 17, 2024

kevin85421 left a comment

astefanutti commented Jan 18, 2024

kevin85421 left a comment

andrewsykim commented Jan 18, 2024

andrewsykim commented Jan 18, 2024 •

edited

andrewsykim commented Jan 18, 2024

andrewsykim commented Jan 19, 2024

astefanutti commented Jan 19, 2024


		type labelSelector string

		var _ Option[metav1.ListOptions] = (labelSelector)(nil)

Add apply configurations to generated client #1818

Add apply configurations to generated client #1818

Conversation

astefanutti commented Jan 9, 2024

Why are these changes needed?

Related issue number

Checks

astefanutti commented Jan 9, 2024

kevin85421 commented Jan 10, 2024

astefanutti commented Jan 15, 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

kevin85421 left a comment

Choose a reason for hiding this comment

astefanutti commented Jan 18, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

andrewsykim commented Jan 18, 2024

andrewsykim commented Jan 18, 2024 • edited

andrewsykim commented Jan 18, 2024

andrewsykim commented Jan 19, 2024

astefanutti commented Jan 19, 2024

andrewsykim commented Jan 18, 2024 •

edited