From 3371dee7bf4364386a5c391f9623ce60f5d229a5 Mon Sep 17 00:00:00 2001 From: Constantin Macaria Date: Wed, 3 May 2023 15:12:05 +0300 Subject: [PATCH] Fixed PR comments --- .../kuberay-operator/crds/ray.io_rayjobs.yaml | 2 +- ray-operator/config/crd/bases/ray.io_rayjobs.yaml | 2 +- ray-operator/controllers/ray/rayjob_controller.go | 6 +++--- .../ray/rayjob_controller_suspended_test.go | 10 +++++++--- .../ray/utils/dashboard_httpclient_test.go | 2 +- .../client/clientset/versioned/fake/register.go | 14 +++++++------- .../client/clientset/versioned/scheme/register.go | 14 +++++++------- 7 files changed, 27 insertions(+), 23 deletions(-) diff --git a/helm-chart/kuberay-operator/crds/ray.io_rayjobs.yaml b/helm-chart/kuberay-operator/crds/ray.io_rayjobs.yaml index 33e44e91b06..47fb59d5c40 100644 --- a/helm-chart/kuberay-operator/crds/ray.io_rayjobs.yaml +++ b/helm-chart/kuberay-operator/crds/ray.io_rayjobs.yaml @@ -11745,7 +11745,7 @@ spec: type: boolean suspend: description: suspend specifies whether the RayJob controller should - create a RayCluster instance + create a RayCluster instance If a job is appl type: boolean ttlSecondsAfterFinished: description: TTLSecondsAfterFinished is the TTL to clean up RayCluster. diff --git a/ray-operator/config/crd/bases/ray.io_rayjobs.yaml b/ray-operator/config/crd/bases/ray.io_rayjobs.yaml index 33e44e91b06..47fb59d5c40 100644 --- a/ray-operator/config/crd/bases/ray.io_rayjobs.yaml +++ b/ray-operator/config/crd/bases/ray.io_rayjobs.yaml @@ -11745,7 +11745,7 @@ spec: type: boolean suspend: description: suspend specifies whether the RayJob controller should - create a RayCluster instance + create a RayCluster instance If a job is appl type: boolean ttlSecondsAfterFinished: description: TTLSecondsAfterFinished is the TTL to clean up RayCluster. diff --git a/ray-operator/controllers/ray/rayjob_controller.go b/ray-operator/controllers/ray/rayjob_controller.go index 206b94abb4e..510f68e70ab 100644 --- a/ray-operator/controllers/ray/rayjob_controller.go +++ b/ray-operator/controllers/ray/rayjob_controller.go @@ -243,10 +243,10 @@ func (r *RayJobReconciler) Reconcile(ctx context.Context, request ctrl.Request) return ctrl.Result{RequeueAfter: RayJobDefaultRequeueDuration}, err } } - err = r.updateState(ctx, rayJobInstance, jobInfo, rayv1alpha1.JobStatusStopped, rayJobInstance.Status.JobDeploymentStatus, nil) - if err != nil { - return ctrl.Result{RequeueAfter: RayJobDefaultRequeueDuration}, err + if info.JobStatus != rayv1alpha1.JobStatusStopped { + return ctrl.Result{RequeueAfter: RayJobDefaultRequeueDuration}, nil } + _, err = r.deleteCluster(ctx, rayJobInstance) if err != nil && !errors.IsNotFound(err) { return ctrl.Result{RequeueAfter: RayJobDefaultRequeueDuration}, nil diff --git a/ray-operator/controllers/ray/rayjob_controller_suspended_test.go b/ray-operator/controllers/ray/rayjob_controller_suspended_test.go index 66a03539c02..689fc4b33a7 100644 --- a/ray-operator/controllers/ray/rayjob_controller_suspended_test.go +++ b/ray-operator/controllers/ray/rayjob_controller_suspended_test.go @@ -20,6 +20,7 @@ import ( "fmt" "time" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" "github.com/ray-project/kuberay/ray-operator/controllers/ray/common" @@ -51,7 +52,7 @@ var _ = Context("Inside the default namespace", func() { Suspend: true, Entrypoint: "sleep 999", RayClusterSpec: &rayiov1alpha1.RayClusterSpec{ - RayVersion: "1.12.1", + RayVersion: "2.4.0", HeadGroupSpec: rayiov1alpha1.HeadGroupSpec{ ServiceType: corev1.ServiceTypeClusterIP, Replicas: pointer.Int32(1), @@ -204,8 +205,11 @@ var _ = Context("Inside the default namespace", func() { // However the actual cluster instance and underlying resources should not be created while suspend == true Eventually( // k8sClient client throws error if resource not found - getResourceFunc(ctx, client.ObjectKey{Name: mySuspendedRayJob.Status.RayClusterName, Namespace: "default"}, mySuspendedRayCluster), - time.Second*10, time.Millisecond*500).Should(Not(BeNil())) + func() bool { + err := getResourceFunc(ctx, client.ObjectKey{Name: mySuspendedRayJob.Status.RayClusterName, Namespace: "default"}, mySuspendedRayCluster)() + return errors.IsNotFound(err) + }, + time.Second*10, time.Millisecond*500).Should(BeTrue()) }) It("should unsuspend a rayjob object", func() { diff --git a/ray-operator/controllers/ray/utils/dashboard_httpclient_test.go b/ray-operator/controllers/ray/utils/dashboard_httpclient_test.go index acf39be59da..9ec14b67642 100644 --- a/ray-operator/controllers/ray/utils/dashboard_httpclient_test.go +++ b/ray-operator/controllers/ray/utils/dashboard_httpclient_test.go @@ -90,7 +90,7 @@ var _ = Describe("RayFrameworkGenerator", func() { Expect(rayJobInfo.Entrypoint).To(Equal(rayJob.Spec.Entrypoint)) Expect(rayJobInfo.JobStatus).To(Equal(rayv1alpha1.JobStatusRunning)) - _, err = rayDashboardClient.GetJobInfo(errorJobId) + _, err = rayDashboardClient.GetJobInfo(context.TODO(), errorJobId) Expect(err).NotTo(BeNil()) Expect(err.Error()).To(ContainSubstring("GetJobInfo fail")) Expect(err.Error()).To(ContainSubstring("Ray misbehaved")) diff --git a/ray-operator/pkg/client/clientset/versioned/fake/register.go b/ray-operator/pkg/client/clientset/versioned/fake/register.go index c64765715ad..5130e6ba593 100644 --- a/ray-operator/pkg/client/clientset/versioned/fake/register.go +++ b/ray-operator/pkg/client/clientset/versioned/fake/register.go @@ -21,14 +21,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/ray-operator/pkg/client/clientset/versioned/scheme/register.go b/ray-operator/pkg/client/clientset/versioned/scheme/register.go index 7ea47d1c691..4c376262b39 100644 --- a/ray-operator/pkg/client/clientset/versioned/scheme/register.go +++ b/ray-operator/pkg/client/clientset/versioned/scheme/register.go @@ -21,14 +21,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly.