Skip to content

only use the default-dsci applicationsNamespace in openshift #676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 1, 2025
Merged

only use the default-dsci applicationsNamespace in openshift #676

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9a258c
only use the applicationsNamespace in openshift
szaher Mar 27, 2025
4d1fd3a
use fallback namespaces if dsci cluster isn't found
szaher Mar 28, 2025
1146637
only use default-dsci namespace when available
szaher Mar 28, 2025
c583416
use ray-system as default ray namespace
szaher Apr 1, 2025
8db15e7
update copyrights for constants.go
szaher Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions pkg/controllers/raycluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,19 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
// - Or fallback to the well-known defaults
// add check if running on openshift or vanilla kubernetes
var kubeRayNamespaces []string
kubeRayNamespaces = []string{"opendatahub", "redhat-ods-applications"}
kubeRayNamespaces = []string{cluster.Namespace}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is RayCluster namespace used here for KubeRay namespace?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@sutaakar sutaakar Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine for me to adjust it later


if r.IsOpenShift {
dsci := &dsciv1.DSCInitialization{}

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

@eoinfennessy eoinfennessy Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if err != nil {
return ctrl.Result{}, err
} else {
kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}
}
kubeRayNamespaces = []string{dsci.Spec.ApplicationsNamespace}

}

_, err := r.kubeClient.NetworkingV1().NetworkPolicies(cluster.Namespace).Apply(ctx, desiredHeadNetworkPolicy(cluster, r.Config, kubeRayNamespaces), metav1.ApplyOptions{FieldManager: controllerName, Force: true})
Expand Down
Loading