add feature gate mechanism to ray-operator #2219

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

Why are these changes needed?

Add feature gating to KubeRay. This uses standard feature gate implementation used by Kubernetes components.

Related issue number

None

Checks

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

andrewsykim · 2024-07-02T19:47:11Z

ray-operator/Dockerfile

@@ -13,6 +13,7 @@ RUN go mod download
 COPY main.go main.go
 COPY apis/ apis/
 COPY controllers/ controllers/
+COPY pkg/ pkg/


this also includes pkg/client, which I think is fine? I can also update to pkg/features

What's the difference in image size with and without pkg/?

I'll just update this to only copy pkg/features, we can always change it to pkg/ when we introduce more packages that need to be compiled

kevin85421

I have no experience about feature gate in K8s. Would you mind explaining what will happen if we turn off a feature flag?

I am reading this PR: kubernetes/kubernetes#116261. It adds // +featureGate=AdmissionWebhookMatchConditions to types.go. Will the +featureGate annotation affect the CRD generation if the feature is off?

	// This is an alpha feature and managed by the AdmissionWebhookMatchConditions feature gate.
	//
	// +featureGate=AdmissionWebhookMatchConditions
	// +optional
	MatchConditions []MatchCondition

kevin85421 · 2024-07-05T20:56:10Z

ray-operator/pkg/features/features.go

+	return featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.DefaultFeatureGate, f, value)
+}
+
+// Enabled is helper for `utilfeature.DefaultFeatureGate.Enabled()`


Do we need to make Enabled a public method?

I think so, there's no usage of it in this PR, but it'll be nice to have for feature gating the new RayCluster status chfanges

kevin85421 · 2024-07-05T21:06:08Z

ray-operator/Dockerfile

@@ -13,6 +13,7 @@ RUN go mod download
 COPY main.go main.go
 COPY apis/ apis/
 COPY controllers/ controllers/
+COPY pkg/ pkg/


What's the difference in image size with and without pkg/?

kevin85421 · 2024-07-05T21:11:46Z

ray-operator/main.go

@@ -92,6 +95,7 @@ func main() {
 	flag.StringVar(&configFile, "config", "", "Path to structured config file. Flags are ignored if config file is set.")
 	flag.BoolVar(&useKubernetesProxy, "use-kubernetes-proxy", false,
 		"Use Kubernetes proxy subresource when connecting to the Ray Head node.")
+	flag.StringVar(&featureGates, "feature-gates", "", "A set of key=value pairs that describe feature gates.")


Should we mention that the value needs to be "true" or "false"?

https://pkg.go.dev/k8s.io/component-base/featuregate#MutableFeatureGate

feature1=true,feature2=false,...

ack, will update

Added an example in the flag description

kevin85421 · 2024-07-05T21:14:40Z

ray-operator/pkg/features/features.go

+	features := make(map[featuregate.Feature]bool, len(defaultFeatureGates))
+	for f := range utilfeature.DefaultMutableFeatureGate.GetAll() {
+		if _, ok := defaultFeatureGates[f]; ok {
+			features[f] = Enabled(f)


Enabled seems to be a one-line function. Maybe we can use utilfeature.DefaultFeatureGate.Enabled(f) directly?

I think Enabled will be useful when we implement the feature gate check for the RayCluster status changes

kevin85421 · 2024-07-05T21:26:10Z

ray-operator/pkg/features/features.go

+}
+
+var defaultFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
+	RayClusterStatusConditions: {Default: false, PreRelease: featuregate.Alpha},


We can turn it on by default for the alpha release if we successfully deliver it before v1.2.0. Unlike a big project like Kubernetes, if we don't turn on a feature, it is hard for us to collect feedback because no user will turn it on.

If we plan to turn feature gates on by default in alpha, then we probably don't need feature gates?

The main purpose of the feature gate is to introduce early versions of features in alpha that we can break / change before going to Beta / GA. Agree that significantly less users will use alpha features if disabled by default, but that's the trade-off of being able to break alpha stage features. If we enable by default, then breaking a feature in alpha phase will break many users, which defeats the purpose

How about we turn it off at this moment? We can try to find some users to try the alpha feature. If we get enough users to test this alpha feature, we can turn it off by default.

If we don't find enough users, we can turn it on by default to collect user feedback more aggressively.

How about we turn it off at this moment? We can try to find some users to try the alpha feature. If we get enough users to test this alpha feature, we can turn it off by default.

This sounds good to me

kevin85421 · 2024-07-08T20:37:10Z

When the feature gate is off, the CRD will still be generated in the OpenAPI YAML file, but we will check the feature gate to determine whether to write to the conditions field or not. If it is off, conditions will always be empty.

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

kevin85421

LGTM. @rueian would you mind reviewing this PR? Thanks!

rueian · 2024-07-14T14:53:11Z

ray-operator/pkg/features/features.go

+
+// SetFeatureGateDuringTest is a helper method to override feature gates in tests.
+func SetFeatureGateDuringTest(tb testing.TB, f featuregate.Feature, value bool) func() {
+	return featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.DefaultFeatureGate, f, value)


It feels weird to import the testing and the featuregatetesting package outside a _test.go file. I believe that will have some impact on the compilation time. Is this a common practice while implementing a custom feature gate with the utilfeature package? Or could we just move the SetFeatureGateDuringTest into a _test.go file?

I believe that will have some impact on the compilation time.

I believe this is negligible impact to compilation time

Is this a common practice while implementing a custom feature gate with the utilfeature package? Or could we just move the SetFeatureGateDuringTest into a _test.go file?

I believe it's common to do this because many test packages will execute code that checks the feature gate and will compile this package already. If you move it into _test.go files, then each test will have to carry a copy of this method or import it from a common test package.

andrewsykim mentioned this pull request Jul 2, 2024

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

Merged

3 tasks

andrewsykim force-pushed the feature-gates branch from e21d9ea to c46ec29 Compare July 2, 2024 19:44

andrewsykim commented Jul 2, 2024

View reviewed changes

andrewsykim changed the title ~~[WIP] add feature gate mechanism to ray-operator~~ add feature gate mechanism to ray-operator Jul 2, 2024

kevin85421 self-assigned this Jul 3, 2024

kevin85421 reviewed Jul 5, 2024

View reviewed changes

This was referenced Jul 11, 2024

[Feature] REP 54: Don’t assign the rayv1.Failed to the State field #2234

Closed

[Feature] REP 54: Re-define rayv1.Ready #2238

Closed

add feature gate mechanism to ray-operator

945fdce

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the feature-gates branch from c46ec29 to 945fdce Compare July 14, 2024 02:27

kevin85421 approved these changes Jul 14, 2024

View reviewed changes

rueian reviewed Jul 14, 2024

View reviewed changes

rueian approved these changes Jul 15, 2024

View reviewed changes

kevin85421 merged commit b05c396 into ray-project:master Jul 15, 2024
25 checks passed

Yicheng-Lu-llll mentioned this pull request Jul 19, 2024

Don’t assign the rayv1.Failed to the State field #2258

Merged

4 tasks

rueian mentioned this pull request Jul 30, 2024

[Feature] REP 54: Replace rayv1.Suspended with RayClusterSuspended condition #2277

Closed

2 tasks

add feature gate mechanism to ray-operator #2219

add feature gate mechanism to ray-operator #2219

Uh oh!

Conversation

andrewsykim commented Jul 2, 2024

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 left a comment

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 Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kevin85421 commented Jul 8, 2024

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrewsykim Jul 14, 2024 •

edited

Loading

andrewsykim Jul 5, 2024 •

edited

Loading

rueian Jul 14, 2024 •

edited

Loading

andrewsykim Jul 15, 2024 •

edited

Loading