add raycluster controller to CFO #453

KPostOffice · 2024-01-25T22:57:27Z

Issue link

https://issues.redhat.com/browse/RHOAIENG-1992

What changes have been made

Added a controller to CFO operator which does a series of ServerSideApplies in order to create the requisite k8 resources for securing the RayDashboard

Verification steps

This PR does not implement the webhook required for automatically injecting the oauth sidecar into the RayCluster definition so that must be done by the user at this point.

TODO Provide a working RayCluster definition for validating the authenticated dashboard

Checks

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

anishasthana · 2024-01-26T13:15:31Z

/retest

KPostOffice · 2024-01-30T19:27:14Z

I tested and made sure that all the expected resources are created. Still need to write unit tests and add a RayCluster def which with a OAuth sidecar that will work with the created resources

KPostOffice · 2024-02-07T16:34:28Z

apiVersion: ray.io/v1
kind: RayCluster
metadata:
  labels:
    controller-tools.k8s.io: '1.0'
  annotations:
    codeflare.dev/oauth: 'true'
  name: raytest
  namespace: default
spec:
  enableInTreeAutoscaling: false
  headGroupSpec:
    rayStartParams:
      block: 'true'
      dashboard-host: 0.0.0.0
      num-gpus: '0'
    serviceType: ClusterIP
    template:
      spec:
        containers:
        - env:
          - name: MY_POD_IP
            valueFrom:
              fieldRef:
                fieldPath: status.podIP
          image: quay.io/project-codeflare/ray:latest-py39-cu118
          imagePullPolicy: Always
          lifecycle:
            preStop:
              exec:
                command:
                - /bin/sh
                - -c
                - ray stop
          name: ray-head
          ports:
          - containerPort: 6379
            name: gcs
          - containerPort: 8265
            name: dashboard
          - containerPort: 10001
            name: client
          resources:
            limits:
              cpu: 1
              memory: 8G
              nvidia.com/gpu: 0
            requests:
              cpu: 1
              memory: 8G
              nvidia.com/gpu: 0
        - args:
          - --https-address=:8443
          - --provider=openshift
          - --openshift-service-account=raytest
          - --upstream=http://localhost:8265
          - --tls-cert=/etc/tls/private/tls.crt
          - --tls-key=/etc/tls/private/tls.key
          - --cookie-secret=$(COOKIE_SECRET)
          - --openshift-delegate-urls={"/":{"resource":"pods","namespace":"default","verb":"get"}}
          image: registry.redhat.io/openshift4/ose-oauth-proxy@sha256:1ea6a01bf3e63cdcf125c6064cbd4a4a270deaf0f157b3eabb78f60556840366
          name: oauth-proxy
          ports:
          - containerPort: 8443
            name: oauth-proxy
          resources: {}
          volumeMounts:
          - mountPath: /etc/tls/private
            name: proxy-tls-secret
            readOnly: true
          env:
          - name: COOKIE_SECRET
            valueFrom:
              secretKeyRef:
                name: raytest-oauth-config
                key: cookie_secret
        imagePullSecrets: []
        serviceAccount: raytest
        volumes:
        - name: proxy-tls-secret
          secret:
            secretName: raytest-tls
  rayVersion: 2.7.0
  workerGroupSpecs:
  - groupName: small-group-raytest
    maxReplicas: 1
    minReplicas: 1
    rayStartParams:
      block: 'true'
      num-gpus: '0'
    replicas: 1
    template:
      metadata:
        annotations:
          key: value
        labels:
          key: value
      spec:
        containers:
        - env:
          - name: MY_POD_IP
            valueFrom:
              fieldRef:
                fieldPath: status.podIP
          image: quay.io/project-codeflare/ray:latest-py39-cu118
          lifecycle:
            preStop:
              exec:
                command:
                - /bin/sh
                - -c
                - ray stop
          name: machine-learning
          resources:
            limits:
              cpu: 1
              memory: 4G
              nvidia.com/gpu: 0
            requests:
              cpu: 1
              memory: 4G
              nvidia.com/gpu: 0
        imagePullSecrets: []

Here is a well formatted RayCluster that can be used to test these changes. Run the controller using:

$ NAMESPACE=foobar make run

Then apply the yaml above. There should be a route in the same namespace on the cluster for the dashboard that is protected by an OAuth Proxy

KPostOffice · 2024-02-07T16:46:06Z

/hold

Still need to edit CI/makefile so that the new Envtest tests will run

astefanutti

Two points:

Do we want to leverage static sidecar injection in the Kuberay deployment?
Could we have a e2e test that run on OpenShift?

@sutaakar FYI.

config/rbac/role.yaml

astefanutti · 2024-02-07T17:24:08Z

config/crd/test/ray.io_rayclusters.yaml

@@ -0,0 +1,14234 @@
+---


Can we use a reference to import these CRDs as we do for MCAD: https://github.com/project-codeflare/codeflare-operator/blob/1264faabc1835b3aafb5e11fc31c4b1bbb9a1382/config/crd/mcad/kustomization.yaml#L4C6-L4C83

We only need it for testing and I couldn't get envtest to work with remote files. I'm thinking of changing it so that it downloads them and cleans them up as part of the BeforeSuite and AfterSuite. It really only needs the route and the raycluster. Do we need to have the Ray CRDs applied as well before starting the controller?

Ah I missed that envtest cannot use Kustomize reference mechanism.

For the dependency on the RayCluster API and the KubeRay CRD, I have some ideas that I need to formalise, but in the short term, what we can do is only start the controller when the API is present in the cluster.

This can cause race conditions when both components are enabled. If CFO starts before KubeRay, then this Reconciler will never start despite the API being present soon after

Right, but on the other hand, the CFO will never work if KubeRay is not installed. I don't think there is an ideal solution to this problem in the short term. We can document the CFO has to be restarted after KubeRay is installed, until we rework the deployment strategy.

My current thinking is that this controller should be deployed along side KubeRay, e.g. in a sidecar container.

controllers/raycluster_controller.go

main.go

controllers/raycluster_controller_test.go

Bobbins228

Tested this with Christians work on the SDK works as expected

/lgtm

KPostOffice · 2024-03-22T18:13:14Z

I changed adding the taint to be info level in the case of an error since errors are not unexpected due to the two controllers managing the resource

KPostOffice · 2024-03-22T18:17:01Z

/unhold

Signed-off-by: Kevin <[email protected]>

Bobbins228

/lgtm

@Bobbins228

@Bobbins228 has lgtm'd it.

Signed-off-by: Kevin <[email protected]>

astefanutti · 2024-04-02T15:11:09Z

PROJECT

@@ -6,4 +10,10 @@ plugins:
  scorecard.sdk.operatorframework.io/v2: {}
 projectName: codeflare-operator
 repo: github.com/project-codeflare/codeflare-operator
+resources:
+- controller: true
+  domain: codeflare.dev


Maybe a nit, but I'm not sure that's the right domain here if it's needed.

astefanutti · 2024-04-02T15:15:03Z

controllers/raycluster_controller.go

+	} else if err != nil {
+		return err
+	}
+	return r.Client.Delete(ctx, obj, &deleteOptions)


Isn't it simpler to ignore not found error on delete?

astefanutti · 2024-04-02T15:16:46Z

pkg/config/config.go

@@ -36,6 +36,8 @@ type CodeFlareOperatorConfiguration struct {

 	// The InstaScale controller configuration
 	InstaScale *InstaScaleConfiguration `json:"instascale,omitempty"`
+
+	RayClusterOAuth *bool `json:"rayClusterOAuth,omitempty"`


I'd suggest to structure the configuration a bit, in a dedicated struct.

astefanutti · 2024-04-02T15:17:46Z

controllers/raycluster_controller.go

+	strTrue                 = "true"
+	strFalse                = "false"


That can be removed?

astefanutti · 2024-04-02T15:18:51Z

controllers/raycluster_controller.go

+const (
+	requeueTime             = 10
+	controllerName          = "codeflare-raycluster-controller"
+	oauthAnnotation         = "codeflare.dev/oauth=true"


That can be removed?

astefanutti · 2024-04-02T15:20:34Z

controllers/raycluster_controller.go

+	requeueTime             = 10
+	controllerName          = "codeflare-raycluster-controller"
+	oauthAnnotation         = "codeflare.dev/oauth=true"
+	CodeflareOAuthFinalizer = "codeflare.dev/oauth-finalizer"


May I suggest something like openshift.ai/oauth-proxy? In case we move the controller outside codeflare.

astefanutti · 2024-04-02T16:38:31Z

pkg/config/config.go

@@ -37,6 +37,10 @@ type CodeFlareOperatorConfiguration struct {
 	// The InstaScale controller configuration
 	InstaScale *InstaScaleConfiguration `json:"instascale,omitempty"`

+	Authorization *AuthorizationConfiguration `json:"authorization,omitempty"`


Ah I should have been more specific. Here for the first level it's about controller, so that'd be KubeRay.

Ahh, got it. Will fix

astefanutti · 2024-04-02T16:43:25Z

pkg/config/config.go

+	Authorization *AuthorizationConfiguration `json:"authorization,omitempty"`
+}
+
+type AuthorizationConfiguration struct {
 	RayClusterOAuth *bool `json:"rayClusterOAuth,omitempty"`


Can this be RayClusterDashboardOAuthEnabled as other endpoints might require different configuration.

astefanutti · 2024-04-02T17:27:50Z

pkg/config/config.go

@@ -36,6 +36,12 @@ type CodeFlareOperatorConfiguration struct {

 	// The InstaScale controller configuration
 	InstaScale *InstaScaleConfiguration `json:"instascale,omitempty"`
+
+	KubeRay *KubeRayConfiguration `json:"authorization,omitempty"`


json:"authorization,omitempty" -> json:"kuberay,omitempty"

astefanutti · 2024-04-02T17:28:33Z

pkg/config/config.go

+}
+
+type KubeRayConfiguration struct {
+	RayDashboardOAuthEnabled *bool `json:"rayClusterOAuth,omitempty"`


json:"rayClusterOAuth,omitempty" -> json:"rayClusterOAuthEnabled,omitempty"

Signed-off-by: Kevin <[email protected]>

astefanutti · 2024-04-03T07:57:12Z

/lgtm

astefanutti · 2024-04-03T08:04:19Z

LGTM, we may want to be more fine-grained to enable the creation of Route without OAuth, but that can be done later.

astefanutti · 2024-04-03T12:28:08Z

/approve

openshift-ci · 2024-04-03T12:28:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti

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

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

KPostOffice requested a review from astefanutti January 25, 2024 22:57

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

KPostOffice force-pushed the kuberay-auth branch from 9383cc7 to d1d1069 Compare January 26, 2024 22:12

openshift-merge-robot added the needs-rebase label Jan 26, 2024

KPostOffice force-pushed the kuberay-auth branch from d1d1069 to 28f85b8 Compare January 30, 2024 19:25

KPostOffice force-pushed the kuberay-auth branch 2 times, most recently from 18ecf16 to c878213 Compare February 1, 2024 21:33

KPostOffice marked this pull request as ready for review February 1, 2024 21:33

openshift-merge-robot removed the needs-rebase label Feb 1, 2024

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

openshift-ci bot requested review from jbusche and Maxusmusti February 1, 2024 21:33

KPostOffice force-pushed the kuberay-auth branch 5 times, most recently from 2735edb to 6659ded Compare February 7, 2024 15:41

openshift-merge-robot added the needs-rebase label Feb 7, 2024

KPostOffice force-pushed the kuberay-auth branch from 6659ded to e1af880 Compare February 7, 2024 15:42

openshift-merge-robot removed the needs-rebase label Feb 7, 2024

KPostOffice force-pushed the kuberay-auth branch from e1af880 to c7ed522 Compare February 7, 2024 15:46

openshift-ci bot added the do-not-merge/hold label Feb 7, 2024

astefanutti reviewed Feb 7, 2024

View reviewed changes

KPostOffice force-pushed the kuberay-auth branch from c7ed522 to 56b8857 Compare February 7, 2024 18:35

astefanutti reviewed Feb 8, 2024

View reviewed changes

controllers/raycluster_controller.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

sutaakar reviewed Feb 8, 2024

View reviewed changes

controllers/raycluster_controller_test.go Show resolved Hide resolved

Bobbins228 reviewed Mar 21, 2024

View reviewed changes

openshift-ci bot added the lgtm label Mar 21, 2024

openshift-ci bot removed the do-not-merge/hold label Mar 22, 2024

add raycluster controller to CFO

2a1de75

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the kuberay-auth branch from c65dbce to 2a1de75 Compare March 25, 2024 12:28

openshift-ci bot removed the lgtm label Mar 25, 2024

Bobbins228 reviewed Mar 25, 2024

View reviewed changes

openshift-ci bot added the lgtm label Mar 25, 2024

openshift-ci bot removed the lgtm label Apr 2, 2024

make oauth part of config and remove annotation check

1ff6d05

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the kuberay-auth branch from b139c3c to 1ff6d05 Compare April 2, 2024 14:20

astefanutti reviewed Apr 2, 2024

View reviewed changes

KPostOffice force-pushed the kuberay-auth branch from 0184f58 to 2bac4d3 Compare April 2, 2024 17:25

astefanutti reviewed Apr 2, 2024

View reviewed changes

address comments from PR review

1f7f96b

Signed-off-by: Kevin <[email protected]>

KPostOffice force-pushed the kuberay-auth branch from 2bac4d3 to 1f7f96b Compare April 2, 2024 17:42

openshift-ci bot assigned astefanutti Apr 3, 2024

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

openshift-ci bot added the approved label Apr 3, 2024

openshift-merge-bot bot merged commit 9ea041f into project-codeflare:main Apr 3, 2024
7 checks passed

astefanutti mentioned this pull request Apr 3, 2024

RayCluster controller follow-up #496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add raycluster controller to CFO #453

add raycluster controller to CFO #453

KPostOffice commented Jan 25, 2024 •

edited

Loading

anishasthana commented Jan 26, 2024

KPostOffice commented Jan 30, 2024

KPostOffice commented Feb 7, 2024 •

edited

Loading

KPostOffice commented Feb 7, 2024

astefanutti left a comment

astefanutti Feb 7, 2024

KPostOffice Feb 7, 2024

astefanutti Feb 8, 2024

KPostOffice Feb 8, 2024

astefanutti Feb 9, 2024

Bobbins228 left a comment

KPostOffice commented Mar 22, 2024

KPostOffice commented Mar 22, 2024

Bobbins228 left a comment

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

KPostOffice Apr 2, 2024

astefanutti Apr 2, 2024 •

edited

Loading

astefanutti Apr 2, 2024

astefanutti Apr 2, 2024

astefanutti commented Apr 3, 2024

astefanutti commented Apr 3, 2024

astefanutti commented Apr 3, 2024

openshift-ci bot commented Apr 3, 2024

add raycluster controller to CFO #453

add raycluster controller to CFO #453

Conversation

KPostOffice commented Jan 25, 2024 • edited Loading

Issue link

What changes have been made

Verification steps

Checks

anishasthana commented Jan 26, 2024

KPostOffice commented Jan 30, 2024

KPostOffice commented Feb 7, 2024 • edited Loading

KPostOffice commented Feb 7, 2024

astefanutti 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

Bobbins228 left a comment

Choose a reason for hiding this comment

KPostOffice commented Mar 22, 2024

KPostOffice commented Mar 22, 2024

Bobbins228 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

astefanutti Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti commented Apr 3, 2024

astefanutti commented Apr 3, 2024

astefanutti commented Apr 3, 2024

openshift-ci bot commented Apr 3, 2024

KPostOffice commented Jan 25, 2024 •

edited

Loading

KPostOffice commented Feb 7, 2024 •

edited

Loading

astefanutti Apr 2, 2024 •

edited

Loading