only use the default-dsci applicationsNamespace in openshift #676

szaher · 2025-03-27T23:48:54Z

the codeflare operator checks for kuberay namespaces to apply network policies so it uses opendatahub DSCInitialization CRD which isn't available on vanilla k8s clusters so added a condition to check for DSCInitialization only when running on openshift

This fixes the gatejobs running the codeflare-sdk repo as the e2e test jobs are failing because DSCInitialization isn't available on kind cluster that is being used for testing.

Issue link

What changes have been made

Verification steps

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- Testing is not required for this change

the codeflare operator checks for kuberay namespaces to apply network policies so it uses opendatahub DSCInitialization CRD which isn't available on vanilla k8s clusters so added a condition to check for DSCInitialization only when running on openshift This fixes the gatejobs running the codeflare-sdk repo as the e2e test jobs are failing because DSCInitialization isn't available on kind cluster that is being used for testing.

kryanbeane · 2025-03-28T11:17:00Z

pkg/controllers/raycluster_controller.go

-		return ctrl.Result{}, err
-	} else {
-		kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}
+	kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}


@szaher We're still passing these namespaces (which I assume are openshift specific) to the network policy creation below. I may be wrong but on a native k8s installation, kuberay won't be installed in either of those namespaces. The OpenShift specific kustomization.yaml points to opendatahub by default but the deployments namespace by default I think is just ray-system. So wouldn't this still set up those network policies to point to ODH/RHOAI namespaces?

Thanks @kryanbeane, I think the kuberay operator namespace is added via https://github.com/project-codeflare/codeflare-operator/blob/main/pkg/controllers/raycluster_controller.go#L553

openshift-ci · 2025-03-28T11:28:17Z

@kryanbeane: changing LGTM is restricted to collaborators

In response to this:

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-sigs/prow repository.

chipspeak

LGTM Thanks, Saad!

eoinfennessy

Nit: Small readability change avoiding mutation of kubeRayNamespaces and removing unneeded else block after early return.

Otherwise LGTM.

eoinfennessy · 2025-03-28T11:29:43Z

pkg/controllers/raycluster_controller.go

+	kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}
+
+	if r.IsOpenShift {
+		dsci := &dsciv1.DSCInitialization{}
+		err := r.Client.Get(ctx, client.ObjectKey{Name: "default-dsci"}, dsci)
+		if err != nil {
+			return ctrl.Result{}, err
+		} else {
+			kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}
+		}


Suggested change

kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}

if r.IsOpenShift {

dsci := &dsciv1.DSCInitialization{}

err := r.Client.Get(ctx, client.ObjectKey{Name: "default-dsci"}, dsci)

if err != nil {

return ctrl.Result{}, err

} else {

kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}

}

var kubeRayNamespaces []string

if r.IsOpenShift {

dsci := &dsciv1.DSCInitialization{}

if err := r.Client.Get(ctx, client.ObjectKey{Name: "default-dsci"}, dsci); err != nil {

return ctrl.Result{}, err

}

kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}

} else {

kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}

Thanks @eoinfennessy actually there is a missing condition that needs to be handled. if the dsci is not found error we use the fall back namespaces if it's available we use it.
otherwise if it's not openshift, I will use the cluster namespace

Signed-off-by: Saad Zaher <[email protected]>

eoinfennessy · 2025-03-28T12:53:15Z

pkg/controllers/raycluster_controller.go

 		err := r.Client.Get(ctx, client.ObjectKey{Name: "default-dsci"}, dsci)
-		if err != nil {
+		if errors.IsNotFound(err) {
+			kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}


This variable always gets changed on line 285, so kubeRayNamespaces will never be set to []string{"opendatahub", "redhat-ods-applications"}.

good catch, you're right 👍 I'll fix it now

Signed-off-by: Saad Zaher <[email protected]>

sutaakar · 2025-04-01T08:00:36Z

pkg/controllers/raycluster_controller.go

-		return ctrl.Result{}, err
-	} else {
-		kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}
+	kubeRayNamespaces = []string{cluster.Namespace}


Why is RayCluster namespace used here for KubeRay namespace?

@sutaakar This ns would only be used in a non-OpenShift cluster in which I believe we we would be using ray-system for both, correct me if I'm wrong Saad. If it is OpenShift, the ODH and ODS-App.. namespaces get added to this

This code implicates that KubeRay operator and RayCluster CR are installed in the same namespace for Kubernetes.
Which IMHO is not correct.

From my perspective we can provide some specific default value (i.e. ray-system) which would be configurable using KubeRayConfiguration so admin can provide own value for custom installation.

@sutaakar are you suggesting that we add a new config item to the KubeRayConfiguration to allow users to use their own namespace for ray operator if not default one or is it already available?

checked here https://github.com/project-codeflare/codeflare-operator/blob/main/pkg/config/config.go#L47 has no namespace in the config

@sutaakar would it make sense to work on this config item later as feature not a bug fix?

fine for me to adjust it later

Signed-off-by: Saad Zaher <[email protected]>

sutaakar · 2025-04-01T13:12:56Z

/lgtm

sutaakar · 2025-04-01T15:06:38Z

/approve

openshift-ci · 2025-04-01T15:06:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chipspeak, kryanbeane, sutaakar

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~~ [sutaakar]

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

openshift-ci bot requested review from dimakis and Fiona-Waters March 27, 2025 23:48

szaher changed the title ~~only use the applicationsNamespace in openshift~~ only use the default-dsci applicationsNamespace in openshift Mar 28, 2025

kryanbeane reviewed Mar 28, 2025

View reviewed changes

kryanbeane approved these changes Mar 28, 2025

View reviewed changes

chipspeak self-requested a review March 28, 2025 11:29

chipspeak approved these changes Mar 28, 2025

View reviewed changes

openshift-ci bot assigned chipspeak Mar 28, 2025

openshift-ci bot added the lgtm label Mar 28, 2025

eoinfennessy reviewed Mar 28, 2025

View reviewed changes

use fallback namespaces if dsci cluster isn't found

4d1fd3a

Signed-off-by: Saad Zaher <[email protected]>

openshift-ci bot removed the lgtm label Mar 28, 2025

eoinfennessy reviewed Mar 28, 2025

View reviewed changes

only use default-dsci namespace when available

1146637

Signed-off-by: Saad Zaher <[email protected]>

sutaakar reviewed Apr 1, 2025

View reviewed changes

szaher added 2 commits April 1, 2025 13:53

use ray-system as default ray namespace

c583416

Signed-off-by: Saad Zaher <[email protected]>

update copyrights for constants.go

8db15e7

Signed-off-by: Saad Zaher <[email protected]>

openshift-ci bot assigned sutaakar Apr 1, 2025

openshift-ci bot added the lgtm label Apr 1, 2025

openshift-ci bot added the approved label Apr 1, 2025

openshift-merge-bot bot merged commit 9d1836d into project-codeflare:main Apr 1, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only use the default-dsci applicationsNamespace in openshift #676

only use the default-dsci applicationsNamespace in openshift #676

szaher commented Mar 27, 2025

kryanbeane Mar 28, 2025

szaher Mar 28, 2025

openshift-ci bot commented Mar 28, 2025

chipspeak left a comment

eoinfennessy left a comment

eoinfennessy Mar 28, 2025 •

edited

Loading

szaher Mar 28, 2025

eoinfennessy Mar 28, 2025 •

edited

Loading

szaher Mar 28, 2025

sutaakar Apr 1, 2025

kryanbeane Apr 1, 2025

sutaakar Apr 1, 2025 •

edited

Loading

sutaakar Apr 1, 2025

szaher Apr 1, 2025

szaher Apr 1, 2025

sutaakar Apr 1, 2025

sutaakar commented Apr 1, 2025

sutaakar commented Apr 1, 2025

openshift-ci bot commented Apr 1, 2025

only use the default-dsci applicationsNamespace in openshift #676

only use the default-dsci applicationsNamespace in openshift #676

Conversation

szaher commented Mar 27, 2025

Issue link

What changes have been made

Verification steps

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 28, 2025

chipspeak left a comment

Choose a reason for hiding this comment

eoinfennessy left a comment

Choose a reason for hiding this comment

eoinfennessy Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoinfennessy Mar 28, 2025 • 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

sutaakar Apr 1, 2025 • 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

sutaakar commented Apr 1, 2025

sutaakar commented Apr 1, 2025

openshift-ci bot commented Apr 1, 2025

eoinfennessy Mar 28, 2025 •

edited

Loading

eoinfennessy Mar 28, 2025 •

edited

Loading

sutaakar Apr 1, 2025 •

edited

Loading