Don't delete Ray head Pod when ImagePullSecret is provided

Author: sutaakar

go.mod

@@ -41,6 +41,8 @@ replace sigs.k8s.io/kueue v0.8.3 => github.com/opendatahub-io/kueue v0.8.3
 // This replace directive deal with the unaligned Ray operator version transitively pulled by Kueue 0.8.3
 replace github.com/ray-project/kuberay/ray-operator v1.2.1 => github.com/ray-project/kuberay/ray-operator v1.1.1
 
+replace github.com/project-codeflare/codeflare-common => /home/ksuta/src/github.com/project-codeflare/codeflare-common
+
 require (
 	github.com/aymerick/douceur v0.2.0 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect

pkg/controllers/raycluster_controller.go

@@ -219,8 +219,11 @@ func (r *RayClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			return ctrl.Result{RequeueAfter: requeueTime}, err
 		}
 
-		if err := r.deleteHeadPodIfMissingImagePullSecrets(ctx, cluster); err != nil {
-			return ctrl.Result{RequeueAfter: requeueTime}, err
+		if len(cluster.Spec.HeadGroupSpec.Template.Spec.ImagePullSecrets) == 0 {
+			// Delete head pod only if user doesn't specify own imagePullSecrets and imagePullSecrets from OAuth ServiceAccount are not present in the head Pod
+			if err := r.deleteHeadPodIfMissingImagePullSecrets(ctx, cluster); err != nil {
+				return ctrl.Result{RequeueAfter: requeueTime}, err
+			}
 		}
 
 		_, err = r.kubeClient.RbacV1().ClusterRoleBindings().Apply(ctx, desiredOAuthClusterRoleBinding(cluster), metav1.ApplyOptions{FieldManager: controllerName, Force: true})

pkg/controllers/raycluster_controller_test.go

@@ -192,6 +192,73 @@ var _ = Describe("RayCluster controller", func() {
 			}).WithTimeout(time.Second * 10).Should(Satisfy(errors.IsNotFound))
 		})
 
+		It("should not delete the head pod if RayCluster CR provides image pull secrets", func(ctx SpecContext) {
+			By("creating an instance of the RayCluster CR with imagePullSecret")
+			rayclusterWithPullSecret := &rayv1.RayCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "pull-secret-cluster",
+					Namespace: namespaceName,
+				},
+				Spec: rayv1.RayClusterSpec{
+					HeadGroupSpec: rayv1.HeadGroupSpec{
+						Template: corev1.PodTemplateSpec{
+							Spec: corev1.PodSpec{
+								ImagePullSecrets: []corev1.LocalObjectReference{{Name: "custom-pull-secret"}},
+								Containers:       []corev1.Container{},
+							},
+						},
+						RayStartParams: map[string]string{},
+					},
+				},
+			}
+			_, err := rayClient.RayV1().RayClusters(namespaceName).Create(ctx, rayclusterWithPullSecret, metav1.CreateOptions{})
+			Expect(err).To(Not(HaveOccurred()))
+
+			Eventually(func() (*corev1.ServiceAccount, error) {
+				return k8sClient.CoreV1().ServiceAccounts(namespaceName).Get(ctx, oauthServiceAccountNameFromCluster(rayclusterWithPullSecret), metav1.GetOptions{})
+			}).WithTimeout(time.Second * 10).Should(WithTransform(OwnerReferenceKind, Equal("RayCluster")))
+
+			headPodName := "head-pod"
+			headPod := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      headPodName,
+					Namespace: namespaceName,
+					Labels: map[string]string{
+						"ray.io/node-type": "head",
+						"ray.io/cluster":   rayclusterWithPullSecret.Name,
+					},
+				},
+				Spec: corev1.PodSpec{
+					ImagePullSecrets: []corev1.LocalObjectReference{
+						{Name: "custom-pull-secret"},
+					},
+					Containers: []corev1.Container{
+						{
+							Name:  "head-container",
+							Image: "busybox",
+						},
+					},
+				},
+			}
+			_, err = k8sClient.CoreV1().Pods(namespaceName).Create(ctx, headPod, metav1.CreateOptions{})
+			Expect(err).To(Not(HaveOccurred()))
+
+			Eventually(func() (*corev1.Pod, error) {
+				return k8sClient.CoreV1().Pods(namespaceName).Get(ctx, headPodName, metav1.GetOptions{})
+			}).WithTimeout(time.Second * 10).ShouldNot(BeNil())
+
+			sa, err := k8sClient.CoreV1().ServiceAccounts(namespaceName).Get(ctx, oauthServiceAccountNameFromCluster(rayclusterWithPullSecret), metav1.GetOptions{})
+			Expect(err).To(Not(HaveOccurred()))
+
+			sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "test-image-pull-secret"})
+			_, err = k8sClient.CoreV1().ServiceAccounts(namespaceName).Update(ctx, sa, metav1.UpdateOptions{})
+			Expect(err).To(Not(HaveOccurred()))
+
+			Consistently(func() (*corev1.Pod, error) {
+				return k8sClient.CoreV1().Pods(namespaceName).Get(ctx, headPodName, metav1.GetOptions{})
+			}).WithTimeout(time.Second * 5).Should(Not(BeNil()))
+		})
+
 		It("should remove CRB when the RayCluster is deleted", func(ctx SpecContext) {
 			foundRayCluster, err := rayClient.RayV1().RayClusters(namespaceName).Get(ctx, rayClusterName, metav1.GetOptions{})
 			Expect(err).To(Not(HaveOccurred()))

test/e2e/mnist_rayjob_raycluster_test.go

@@ -211,6 +211,52 @@ func runMnistRayJobRayClusterAppWrapper(t *testing.T, accelerator string, number
 	test.Eventually(AppWrappers(test, namespace), TestTimeoutShort).Should(BeEmpty())
 }
 
+// Verifying https://github.com/project-codeflare/codeflare-operator/issues/649
+func TestRayClusterImagePullSecret(t *testing.T) {
+	test := With(t)
+
+	// Create a namespace
+	namespace := test.NewTestNamespace()
+
+	// Create Kueue resources
+	resourceFlavor := CreateKueueResourceFlavor(test, v1beta1.ResourceFlavorSpec{})
+	defer func() {
+		_ = test.Client().Kueue().KueueV1beta1().ResourceFlavors().Delete(test.Ctx(), resourceFlavor.Name, metav1.DeleteOptions{})
+	}()
+	clusterQueue := createClusterQueue(test, resourceFlavor, 0)
+	defer func() {
+		_ = test.Client().Kueue().KueueV1beta1().ClusterQueues().Delete(test.Ctx(), clusterQueue.Name, metav1.DeleteOptions{})
+	}()
+	CreateKueueLocalQueue(test, namespace.Name, clusterQueue.Name, AsDefaultQueue)
+
+	// Create ServiceAccount, wait until corresponding imagePullSecret is available
+	sa := CreateServiceAccount(test, namespace.Name)
+	test.Eventually(ServiceAccount(test, sa.Namespace, sa.Name), TestTimeoutShort).
+		Should(
+			HaveField("ImagePullSecrets", HaveLen(1)),
+		)
+	sa = GetServiceAccount(test, sa.Namespace, sa.Name)
+
+	// Create MNIST training script
+	mnist := constructMNISTConfigMap(test, namespace)
+	mnist, err := test.Client().Core().CoreV1().ConfigMaps(namespace.Name).Create(test.Ctx(), mnist, metav1.CreateOptions{})
+	test.Expect(err).NotTo(HaveOccurred())
+	test.T().Logf("Created ConfigMap %s/%s successfully", mnist.Namespace, mnist.Name)
+
+	// Create RayCluster with imagePullSecret and assign it to the localqueue
+	rayCluster := constructRayCluster(test, namespace, mnist, 0)
+	rayCluster.Spec.HeadGroupSpec.Template.Spec.ImagePullSecrets = sa.ImagePullSecrets
+	rayCluster, err = test.Client().Ray().RayV1().RayClusters(namespace.Name).Create(test.Ctx(), rayCluster, metav1.CreateOptions{})
+	test.Expect(err).NotTo(HaveOccurred())
+	test.T().Logf("Created RayCluster %s/%s successfully", rayCluster.Namespace, rayCluster.Name)
+
+	test.T().Logf("Waiting for RayCluster %s/%s to be running", rayCluster.Namespace, rayCluster.Name)
+	test.Eventually(RayCluster(test, namespace.Name, rayCluster.Name), TestTimeoutMedium).
+		Should(WithTransform(RayClusterState, Equal(rayv1.Ready)))
+}
+
+// Helper functions
+
 func constructMNISTConfigMap(test Test, namespace *corev1.Namespace) *corev1.ConfigMap {
 	return &corev1.ConfigMap{
 		TypeMeta: metav1.TypeMeta{
@@ -274,11 +320,11 @@ func constructRayCluster(_ Test, namespace *corev1.Namespace, mnist *corev1.Conf
 								Resources: corev1.ResourceRequirements{
 									Requests: corev1.ResourceList{
 										corev1.ResourceCPU:    resource.MustParse("250m"),
-										corev1.ResourceMemory: resource.MustParse("512Mi"),
+										corev1.ResourceMemory: resource.MustParse("2G"),
 									},
 									Limits: corev1.ResourceList{
 										corev1.ResourceCPU:    resource.MustParse("1"),
-										corev1.ResourceMemory: resource.MustParse("2G"),
+										corev1.ResourceMemory: resource.MustParse("4G"),
 									},
 								},
 							},