Refactor creation logic of ingress/routes into RayCluster Controller #493

ChristianZaccaria · 2024-03-21T07:49:15Z

Issue link

Issue: https://issues.redhat.com/browse/RHOAIENG-1056

What changes have been made

Refactor the ingress/route creation and TLS support to the RayCluster Controller in the CFO.
Added creation of rayclient route OpenShift, rayclient ingress and ingresses for KinD/vanilla clusters.
RayCluster Controller to always run alongside the CFO.
Adjusted e2e tests to disable OAuth in the CFO config, using this config bool value to indicate to the RayCluster Controller whether to create a secure route and OAuth resources on OpenShift or an ingress for KinD clusters (for e2e tests).
Created a support package and several functions including:
- createRayClientRoute
- createRayClientIngress
- createIngressApplyConfiguration
- isOnKindCluster
- getClusterType
- annotationBoolVal
- getIngressDomain

Verification steps

This PR should be tested alongside this PR in the SDK which removes the creation of ingress/routes creation logic: project-codeflare/codeflare-sdk#495
To test these changes:
In an OpenShift cluster:

Checkout this PR and run make run NAMESPACE=default and make install.
Deploy and install KubeRay v1.1.0
Checkout this PR and install the SDK: Remove ingress/routes logic from SDK codeflare-sdk#495
Deploy Kueue and create a LocalQueue and ClusterQueue.
Run through this notebook
Switch mcad to false.
Add write_to_file to be true to have access to the RayCluster yaml and add the Kueue label kueue.x-k8s.io/queue-name: <name of localqueue>
Add an ingress_domain to the ClusterConfiguration i.e., apps.rosa.clustername.k1pm.p3.openshiftapps.com
Once cluster.up() is ran, the oauth-proxy sidecar is created by default on the SDK side if on an OpenShift cluster and the RayCluster Controller creates all the necessary OAuth resources + necessary rayclient route if the RayCluster is not in a suspended state provided by Kueue. If not enough resources, The RayCluster controller will hold the creation of the routes/ingresses.
After going through the notebook, the route should be accessible through authentication and running ray.get(ref) should result in = 1789.4644.....

In a KinD cluster:

For the local_interactive notebook, same steps as above except you will find that no OAuth resources are created including the oauth-proxy sidecar container. The ingress and RayClient ingress will be created by the RayCluster Controller.

Add in your local /etc/hosts 127.0.0.1 kind as per documentation
For the ingress_domain, use kind unless you are running an un-named KinD cluster of which hostname will resolve to kind-control-plane by default, in this scenario, ingress_domain is not required.
After going through the notebook, the route should be accessible through authentication and running ray.get(ref) should result in = 1789.4644.....

For a basic notebook, same steps involved as above, except you will find that the RayClient ingress is not created this time, only the ingress to the service is created.

Checks

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

openshift-ci · 2024-03-21T07:49:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign astefanutti for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

ChristianZaccaria · 2024-04-04T11:26:16Z

/hold Should be merged with project-codeflare/codeflare-sdk#495

pkg/controllers/raycluster_controller.go

KPostOffice · 2024-04-04T17:33:18Z

pkg/controllers/raycluster_controller.go

@@ -97,6 +101,10 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}

+	isLocalInteractive := annotationBoolVal(logger, &cluster, "sdk.codeflare.dev/local_interactive")
+	isOpenShift, ingressHost := getClusterType(logger, r.kubeClient, &cluster)
+	ingressDomain := getIngressDomain(&cluster)


Since you only care about the presence of ingress

This can just be simplified to:

_, ingressDomainExists := cluster.ObjectMeta.Annotations["sdk.codeflare.dev/ingress_domain"]

and edit the conditionals below

I see why that can be implied, that was wrong on my part. I do require the actual value of the ingress domain annotation too as it's used for the Host field for creating ingresses and RayClient ingress/route.

I was originally declaring a new variable for ingress domain on each creation of ingresses, that's why it looks like I only needed the presence of it. Great catch thanks. Changes made.

pkg/controllers/support.go

KPostOffice · 2024-04-04T17:50:50Z

pkg/controllers/support.go

+	} else {
+		logger.Info("Cannot retrieve config, assuming we're on Vanilla Kubernetes")
+		return false, fmt.Sprintf("ray-dashboard-%s-%s.%s", cluster.Name, cluster.Namespace, ingress_domain)
+	}


Can you change this to:

if err != nil || dclient == nil { logger.Info("Cannot retrieve config, assuming we're on Vanilla Kubernetes") return false, fmt.Sprintf("ray-dashboard-%s-%s.%s", cluster.Name, cluster.Namespace, ingress_domain) }

and then leave the majority of the body outside of conditionals

#NeverNester https://www.youtube.com/watch?v=CFRhGnuXG-4

Oops wrong conditional, but same with this one:

if err != nil || config == nil { ... return ... }

Wow Kevin that video was so enlightening! Thanks for saving me early, I will 100% follow the path of a Never Nester. Hopefully this next commit shows that :) - Thanks a lot for the video and advise.

pkg/controllers/support.go

KPostOffice · 2024-04-04T18:18:16Z

pkg/controllers/support.go

+	return cluster.Name + "-head-svc"
+}
+
+func createRayClientRoute(cluster *rayv1.RayCluster) *routeapply.RouteApplyConfiguration {


I think create is a misnomer here. Can you use the same verbage use for the other resources (desired)?

ChristianZaccaria

Thanks for the review. I added a commit per comment - Will squash them afterwards.

astefanutti · 2024-04-05T10:18:02Z

.github/workflows/e2e_tests.yaml

@@ -60,6 +60,7 @@ jobs:
        run: |
          echo Deploying CodeFlare operator
          IMG="${REGISTRY_ADDRESS}"/codeflare-operator
+          sed -i 's/RayDashboardOAuthEnabled: pointer.Bool(true)/RayDashboardOAuthEnabled: pointer.Bool(false)/' main.go


That should be done in the e2e ConfigMap instead.

astefanutti · 2024-04-05T10:19:04Z

main.go

@@ -184,8 +184,8 @@ func main() {
 	}

 	v, err := HasAPIResourceForGVK(kubeClient.DiscoveryClient, rayv1.GroupVersion.WithKind("RayCluster"))
-	if v && *cfg.KubeRay.RayDashboardOAuthEnabled {
-		rayClusterController := controllers.RayClusterReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}
+	if v {


nit: Let's rename that v to ok :)

astefanutti · 2024-04-05T10:20:01Z

pkg/controllers/raycluster_controller.go

@@ -50,15 +52,17 @@ type RayClusterReconciler struct {
 	routeClient *routev1client.RouteV1Client
 	Scheme      *runtime.Scheme
 	CookieSalt  string
+	Config      *config.CodeFlareOperatorConfiguration


Maybe only the RayCluster controller configuration could be enough here?

astefanutti · 2024-04-05T10:23:21Z

pkg/controllers/raycluster_controller.go

@@ -97,6 +101,10 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}

+	isLocalInteractive := annotationBoolVal(ctx, &cluster, "sdk.codeflare.dev/local_interactive", false)
+	ingressDomain := cluster.ObjectMeta.Annotations["sdk.codeflare.dev/ingress_domain"]
+	isOpenShift, ingressHost := getClusterType(ctx, r.kubeClient, &cluster, ingressDomain)


Checking the cluster type should only be done once when the operator starts. Calling the Discovery API for each reconciliation is also not really acceptable.

astefanutti · 2024-04-05T10:24:26Z

pkg/controllers/support.go

+			return true, ""
+		}
+	}
+	onKind, _ := isOnKindCluster(clientset)


That really feels like testing concerns leaking in application code. Why is it needed to explicitly check KinD?

You are right, we don't explicitly need to check for KinD, anything that is not OpenShift could suit. The only reason that we check for KinD is for our own testing purposes. If it's true, then the ingress Host will use "kind". We could make it more generic by supplying the ingress_domain to the e2e tests, then there is no need for checking for KinD explicitly. Should I change or leave as is for now... WDYT?

astefanutti · 2024-04-05T10:27:38Z

pkg/controllers/raycluster_controller.go

-	if err != nil {
-		logger.Error(err, "Failed to update OAuth Route")
-	}
+	if cluster.Status.State != "suspended" && r.isRayDashboardOAuthEnabled() && isOpenShift {


It's possible the cluster transitions from running to suspended state. Should the resources be removed in that case?

astefanutti · 2024-04-05T10:30:16Z

pkg/controllers/raycluster_controller.go

+	isLocalInteractive := annotationBoolVal(ctx, &cluster, "sdk.codeflare.dev/local_interactive", false)
+	ingressDomain := cluster.ObjectMeta.Annotations["sdk.codeflare.dev/ingress_domain"]


What does justify we let clients / users decide for these? Shouldn't that be the responsibility of the platform admin to configure?

Those are the annotations that are created when local_interactive is true or the user is using an ingress domain.
The local_interactive one is used for the RCC know to create the Ray Client.
And the ingress domain one is needed for Kubernetes clusters right?

Bobbins228

/lgtm
Just tested it out and it creates oauth objects successfully and does not when the RC is suspended.
Also tested suspended -> admitted working as expected

sutaakar · 2024-04-08T06:50:34Z

I miss here a test coverage being added into https://github.com/project-codeflare/codeflare-operator/blob/faff28ab80941db913f509ffdc22eb9cffbed7c8/pkg/controllers/raycluster_controller_test.go

openshift-ci bot added the do-not-merge/work-in-progress label Mar 21, 2024

openshift-ci bot requested review from jbusche and tedhtchang March 21, 2024 07:49

ChristianZaccaria changed the title ~~WIP - Creation/Deletion of ingress and routes in RayCluster Controller~~ WIP - creation and deletion of ingress/routes logic in RayCluster Controller Mar 21, 2024

ChristianZaccaria changed the title ~~WIP - creation and deletion of ingress/routes logic in RayCluster Controller~~ WIP - refactor creation logic of ingress/routes into RayCluster Controller Mar 21, 2024

ChristianZaccaria force-pushed the rc-controller-kueue branch 3 times, most recently from 991d9eb to 6835c21 Compare March 28, 2024 21:35

ChristianZaccaria force-pushed the rc-controller-kueue branch 5 times, most recently from 02e9c95 to 1d9e1fc Compare April 3, 2024 18:09

ChristianZaccaria changed the title ~~WIP - refactor creation logic of ingress/routes into RayCluster Controller~~ Refactor creation logic of ingress/routes into RayCluster Controller Apr 4, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 4, 2024

openshift-ci bot added the do-not-merge/hold label Apr 4, 2024

ChristianZaccaria mentioned this pull request Apr 4, 2024

Remove ingress/routes logic from SDK project-codeflare/codeflare-sdk#495

Merged

4 tasks

anishasthana reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/raycluster_controller.go Show resolved Hide resolved

KPostOffice reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/support.go Outdated Show resolved Hide resolved

KPostOffice reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/support.go Outdated Show resolved Hide resolved

KPostOffice reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/support.go Outdated Show resolved Hide resolved

KPostOffice reviewed Apr 4, 2024

View reviewed changes

pkg/controllers/support.go Outdated Show resolved Hide resolved

KPostOffice reviewed Apr 4, 2024

View reviewed changes

ChristianZaccaria commented Apr 4, 2024

View reviewed changes

ChristianZaccaria force-pushed the rc-controller-kueue branch from ccc3315 to 7071fd3 Compare April 4, 2024 19:47

astefanutti reviewed Apr 5, 2024

View reviewed changes

ChristianZaccaria added 5 commits April 5, 2024 15:40

WIP - Creation/Deletion of ingress and routes in RayCluster Controller

a02e9b9

Always run RayCluster Controller

9f583dc

Add support file for RC Controller

321ef04

Disable OAuth for e2e tests

bd3f2f7

Add OAuth config to reconciler struct

a62b141

ChristianZaccaria force-pushed the rc-controller-kueue branch from 7071fd3 to a62b141 Compare April 5, 2024 14:40

Bobbins228 reviewed Apr 5, 2024

View reviewed changes

openshift-ci bot assigned Bobbins228 Apr 5, 2024

openshift-ci bot added the lgtm label Apr 5, 2024

dimakis requested a review from astefanutti April 5, 2024 15:18

dimakis merged commit 465da20 into project-codeflare:main Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor creation logic of ingress/routes into RayCluster Controller #493

Refactor creation logic of ingress/routes into RayCluster Controller #493

ChristianZaccaria commented Mar 21, 2024 •

edited

Loading

openshift-ci bot commented Mar 21, 2024

ChristianZaccaria commented Apr 4, 2024

KPostOffice Apr 4, 2024

ChristianZaccaria Apr 4, 2024

KPostOffice Apr 4, 2024 •

edited

Loading

KPostOffice Apr 4, 2024

ChristianZaccaria Apr 4, 2024

KPostOffice Apr 4, 2024

ChristianZaccaria left a comment

astefanutti Apr 5, 2024

astefanutti Apr 5, 2024

astefanutti Apr 5, 2024

astefanutti Apr 5, 2024

astefanutti Apr 5, 2024

ChristianZaccaria Apr 5, 2024

astefanutti Apr 5, 2024

astefanutti Apr 5, 2024

ChristianZaccaria Apr 5, 2024

Bobbins228 left a comment

sutaakar commented Apr 8, 2024

		isLocalInteractive := annotationBoolVal(ctx, &cluster, "sdk.codeflare.dev/local_interactive", false)
		ingressDomain := cluster.ObjectMeta.Annotations["sdk.codeflare.dev/ingress_domain"]

Refactor creation logic of ingress/routes into RayCluster Controller #493

Refactor creation logic of ingress/routes into RayCluster Controller #493

Conversation

ChristianZaccaria commented Mar 21, 2024 • edited Loading

Issue link

What changes have been made

Verification steps

Checks

openshift-ci bot commented Mar 21, 2024

ChristianZaccaria commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPostOffice Apr 4, 2024 • 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

ChristianZaccaria left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobbins228 left a comment

Choose a reason for hiding this comment

sutaakar commented Apr 8, 2024

ChristianZaccaria commented Mar 21, 2024 •

edited

Loading

KPostOffice Apr 4, 2024 •

edited

Loading