[Feat][RayCluster] Introduce the RayClusterStatus.Conditions field #2214

rueian · 2024-07-01T13:42:08Z

Why are these changes needed?

This is the first part of the process to refine the Ready/Failed status in RayClusterStatus https://docs.google.com/document/d/1bRL0cZa87eCX6SI7gqthN68CgmHaB6l3-vJuIse-BrY

In this first PR, we introduce the new RayClusterStatus.Conditions field, borrowed from the idea of k8s Deployment https://github.com/kubernetes/api/blob/857a946a225f212b64d42c68a7da0dc44837636f/apps/v1/types.go#L532-L542

Related issue number

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests

kevin85421 · 2024-07-01T20:51:14Z

ray-operator/apis/ray/v1/raycluster_types.go

+	// +patchStrategy=merge
+	// +listType=map
+	// +listMapKey=type
+	Conditions []RayClusterCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`


Why not use metav1.Condition instead? https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition

I missed that. Thank you for reminding me.

kevin85421 · 2024-07-02T05:39:40Z

ray-operator/apis/ray/v1/raycluster_types.go

@@ -140,6 +140,14 @@ type RayClusterStatus struct {
 	Head HeadInfo `json:"head,omitempty"`
 	// Reason provides more information about current State
 	Reason string `json:"reason,omitempty"`
+
+	// Represents the latest available observations of a deployment's current state.
+	// +patchMergeKey=type


Why do we need these?

This is used by strategic merge patch: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch.
This is also used by server-side apply, when listType=map and listMapKey are not specified: https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy.

Yes! Thank you @astefanutti for helping me clarify those markers.

astefanutti · 2024-07-02T06:39:47Z

ray-operator/apis/ray/v1/raycluster_types.go

@@ -140,6 +140,14 @@ type RayClusterStatus struct {
 	Head HeadInfo `json:"head,omitempty"`
 	// Reason provides more information about current State
 	Reason string `json:"reason,omitempty"`
+
+	// Represents the latest available observations of a deployment's current state.
+	// +patchMergeKey=type


This is used by strategic merge patch: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch.
This is also used by server-side apply, when listType=map and listMapKey are not specified: https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy.

astefanutti · 2024-07-02T06:40:40Z

ray-operator/apis/ray/v1/raycluster_types.go

@@ -140,6 +140,14 @@ type RayClusterStatus struct {
 	Head HeadInfo `json:"head,omitempty"`
 	// Reason provides more information about current State
 	Reason string `json:"reason,omitempty"`
+
+	// Represents the latest available observations of a deployment's current state.


This line does not seem specific to the conditions field. It can probably be removed.

Updated to // Represents the latest available observations of a RayCluster's current state.

andrewsykim · 2024-07-02T15:03:22Z

ray-operator/apis/ray/v1/raycluster_types.go

+type RayClusterConditionType string
+
+const (
+	HeadReady RayClusterConditionType = "HeadReady"


nit: Add a comment for HeadReady

Signed-off-by: Rueian <rueiancsie@gmail.com>

andrewsykim · 2024-07-02T16:40:15Z

@rueian @kevin85421 can we have the API change and implementation in the same PR if possible? Otherwise, if we release v1.2 before the implementation we'd have to revert this PR.

kevin85421 · 2024-07-02T19:01:49Z

can we have the API change and implementation in the same PR if possible? Otherwise, if we release v1.2 before the implementation we'd have to revert this PR.

Do you mean adding HeadReady and RayClusterReplicaFailure only when we implement them? cc @andrewsykim

andrewsykim · 2024-07-02T19:26:10Z

Do you mean adding HeadReady and RayClusterReplicaFailure only when we implement them?

Yes, and also the addition of the new conditions field in status. We should avoid changing the CRD / API until there is an working / tested minimum implementation. What do you think?

andrewsykim · 2024-07-02T19:35:56Z

@kevin85421 on second thought, I think incremental PRs is fine as long as it's feature gated. I added initial feature gate implementation in #2219, what do you think?

rueian · 2024-07-03T12:55:07Z

I am ok with the feature gate or waiting for the implementation of HeadReady and RayClusterReplicaFailure.

We should avoid changing the CRD / API until there is an working / tested minimum implementation.

Hi @andrewsykim, do we still need to keep the old CRD / API if the gate is disabled? I thought that could only be achieved by introducing a new CRD version.

kevin85421 · 2024-07-05T21:56:58Z

@andrewsykim Does #2219 need to be merged before this PR or the order is flexible?

kevin85421 · 2024-07-08T23:59:30Z

@rueian I have synchronized with @andrewsykim today. If the feature flag is off, we will still generate conditions for the CRD, but we will not write anything to conditions. This PR doesn't write anything to conditions, so this PR is independent from #2219.

rueian force-pushed the raycluster-status-conditions branch 2 times, most recently from 5fb664e to be47372 Compare July 1, 2024 14:11

rueian marked this pull request as ready for review July 1, 2024 14:38

kevin85421 reviewed Jul 1, 2024

View reviewed changes

kevin85421 self-assigned this Jul 1, 2024

kevin85421 added the crd-observability label Jul 1, 2024

rueian force-pushed the raycluster-status-conditions branch from be47372 to 31401e0 Compare July 2, 2024 00:01

kevin85421 reviewed Jul 2, 2024

View reviewed changes

astefanutti reviewed Jul 2, 2024

View reviewed changes

rueian force-pushed the raycluster-status-conditions branch from 31401e0 to 2178270 Compare July 2, 2024 12:13

rueian requested a review from kevin85421 July 2, 2024 14:26

andrewsykim reviewed Jul 2, 2024

View reviewed changes

[Feat][RayCluster] Introduce the RayClusterStatus.Conditions field

040e810

Signed-off-by: Rueian <rueiancsie@gmail.com>

rueian force-pushed the raycluster-status-conditions branch from 2178270 to 040e810 Compare July 2, 2024 15:54

kevin85421 approved these changes Jul 9, 2024

View reviewed changes

kevin85421 merged commit ea314d7 into ray-project:master Jul 9, 2024

[Feat][RayCluster] Introduce the RayClusterStatus.Conditions field #2214

[Feat][RayCluster] Introduce the RayClusterStatus.Conditions field #2214

Uh oh!

Conversation

rueian commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewsykim commented Jul 2, 2024

Uh oh!

kevin85421 commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewsykim commented Jul 2, 2024

Uh oh!

andrewsykim commented Jul 2, 2024

Uh oh!

rueian commented Jul 3, 2024

Uh oh!

kevin85421 commented Jul 5, 2024

Uh oh!

kevin85421 commented Jul 8, 2024

Uh oh!

Uh oh!

rueian commented Jul 1, 2024 •

edited

Loading

kevin85421 commented Jul 2, 2024 •

edited

Loading