Skip to content
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

Jump to bottom

[Refactor] Extract KubectlApplyYaml and yaml deserialization to support package #2498

Merged
merged 2 commits into from
Nov 5, 2024
Merged

[Refactor] Extract KubectlApplyYaml and yaml deserialization to support package #2498

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ecff73
[Refactor] Extract KubectlApplyYaml and yaml deserialization to suppo…
MortalHappiness Nov 5, 2024
dc639ff
Address issue comment: Restore check of status.DesiredWorkerReplicas
MortalHappiness Nov 5, 2024
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
7 changes: 4 additions & 3 deletions ray-operator/test/sampleyaml/raycluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sampleyaml

import (
"path"
"testing"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -62,10 +63,11 @@ func TestRayCluster(t *testing.T) {
test := With(t)
g := NewWithT(t)

yamlFilePath := path.Join(GetSampleYAMLDir(test), tt.name)
namespace := test.NewTestNamespace()
test.StreamKubeRayOperatorLogs()
rayClusterFromYaml := DeserializeRayClusterSampleYAML(test, tt.name)
KubectlApplyYAML(test, tt.name, namespace.Name)
rayClusterFromYaml := DeserializeRayClusterYAML(test, yamlFilePath)
KubectlApplyYAML(test, yamlFilePath, namespace.Name)

rayCluster, err := GetRayCluster(test, namespace.Name, rayClusterFromYaml.Name)
g.Expect(err).NotTo(HaveOccurred())
Expand All @@ -85,7 +87,6 @@ func TestRayCluster(t *testing.T) {
}
}
g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerReplicas)))
g.Expect(rayCluster.Status.DesiredWorkerReplicas).To(Equal(desiredWorkerReplicas))
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?


// Check if the head pod is ready
g.Eventually(HeadPod(test, rayCluster), TestTimeoutShort).Should(WithTransform(IsPodRunningAndReady, BeTrue()))
Expand Down
8 changes: 4 additions & 4 deletions ray-operator/test/sampleyaml/rayjob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sampleyaml

import (
"path"
"testing"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -35,10 +36,11 @@ func TestRayJob(t *testing.T) {
test := With(t)
g := NewWithT(t)

yamlFilePath := path.Join(GetSampleYAMLDir(test), tt.name)
namespace := test.NewTestNamespace()
test.StreamKubeRayOperatorLogs()
rayJobFromYaml := DeserializeRayJobSampleYAML(test, tt.name)
KubectlApplyYAML(test, tt.name, namespace.Name)
rayJobFromYaml := DeserializeRayJobYAML(test, yamlFilePath)
KubectlApplyYAML(test, yamlFilePath, namespace.Name)

rayJob, err := GetRayJob(test, namespace.Name, rayJobFromYaml.Name)
g.Expect(err).NotTo(HaveOccurred())
Expand All @@ -65,9 +67,7 @@ func TestRayJob(t *testing.T) {
desiredWorkerReplicas += *workerGroupSpec.Replicas
}
}

g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerReplicas)))
g.Expect(rayCluster.Status.DesiredWorkerReplicas).To(Equal(desiredWorkerReplicas))
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we don't need to check this because we've already checked the number of worker pods above. Also because this is a one-line fix so I do it directly in this refactoring PR. Do you think it's better to create a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this differs from the above logic. The above logic checks the number of worker Pods by querying the K8s API server, which is the source of truth. The deleted line checks whether the RayCluster status reflects the actual cluster status.

Copy link
Member Author

@MortalHappiness MortalHappiness Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay I've restored this check, but updating it to

g.Expect(GetRayCluster(test, namespace.Name, rayCluster.Name)).To(WithTransform(RayClusterDesiredWorkerReplicas, Equal(desiredWorkerReplicas)))

because I found that we forgot to get the RayCluster again to update its status.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using a transform just to return a field seems overkill :P

Copy link
Member Author

Choose a reason for hiding this comment

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

I used WithTransform because I found there already been a RayClusterDesiredWorkerReplicas function.

Copy link
Member Author

Choose a reason for hiding this comment

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

And GetRayCluster returns 2 parameters: one is raycluster, and the other is error. WithTransform can successfully take only the first one as input and fail if err is not nil. I think this is an ideal and neat syntax.


// Check if the head pod is ready
g.Eventually(HeadPod(test, rayCluster), TestTimeoutShort).Should(WithTransform(IsPodRunningAndReady, BeTrue()))
Expand Down
6 changes: 4 additions & 2 deletions ray-operator/test/sampleyaml/rayservice_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sampleyaml

import (
"path"
"testing"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -32,10 +33,11 @@ func TestRayService(t *testing.T) {
test := With(t)
g := NewWithT(t)

yamlFilePath := path.Join(GetSampleYAMLDir(test), tt.name)
namespace := test.NewTestNamespace()
test.StreamKubeRayOperatorLogs()
rayServiceFromYaml := DeserializeRayServiceSampleYAML(test, tt.name)
KubectlApplyYAML(test, tt.name, namespace.Name)
rayServiceFromYaml := DeserializeRayServiceYAML(test, yamlFilePath)
KubectlApplyYAML(test, yamlFilePath, namespace.Name)

rayService, err := GetRayService(test, namespace.Name, rayServiceFromYaml.Name)
g.Expect(err).NotTo(HaveOccurred())
Expand Down
53 changes: 1 addition & 52 deletions ray-operator/test/sampleyaml/support.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package sampleyaml

import (
"os"
"os/exec"
"path/filepath"
"runtime"

Expand All @@ -11,11 +10,10 @@ import (
corev1 "k8s.io/api/core/v1"

rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
rayscheme "github.com/ray-project/kuberay/ray-operator/pkg/client/clientset/versioned/scheme"
. "github.com/ray-project/kuberay/ray-operator/test/support"
)

func getSampleYAMLDir(t Test) string {
func GetSampleYAMLDir(t Test) string {
t.T().Helper()
_, b, _, _ := runtime.Caller(0)
sampleYAMLDir := filepath.Join(filepath.Dir(b), "../../config/samples")
Expand All @@ -25,55 +23,6 @@ func getSampleYAMLDir(t Test) string {
return sampleYAMLDir
}

func readYAML(t Test, filename string) []byte {
t.T().Helper()
sampleYAMLDir := getSampleYAMLDir(t)
yamlFile := filepath.Join(sampleYAMLDir, filename)
yamlFileContent, err := os.ReadFile(yamlFile)
assert.NoError(t.T(), err)
return yamlFileContent
}

func DeserializeRayClusterSampleYAML(t Test, filename string) *rayv1.RayCluster {
t.T().Helper()
yamlFileContent := readYAML(t, filename)
decoder := rayscheme.Codecs.UniversalDecoder()
rayCluster := &rayv1.RayCluster{}
_, _, err := decoder.Decode(yamlFileContent, nil, rayCluster)
assert.NoError(t.T(), err)
return rayCluster
}

func DeserializeRayServiceSampleYAML(t Test, filename string) *rayv1.RayService {
t.T().Helper()
yamlFileContent := readYAML(t, filename)
decoder := rayscheme.Codecs.UniversalDecoder()
rayService := &rayv1.RayService{}
_, _, err := decoder.Decode(yamlFileContent, nil, rayService)
assert.NoError(t.T(), err)
return rayService
}

func DeserializeRayJobSampleYAML(t Test, filename string) *rayv1.RayJob {
t.T().Helper()
yamlFileContent := readYAML(t, filename)
decoder := rayscheme.Codecs.UniversalDecoder()
rayJob := &rayv1.RayJob{}
_, _, err := decoder.Decode(yamlFileContent, nil, rayJob)
assert.NoError(t.T(), err)
return rayJob
}

func KubectlApplyYAML(t Test, filename string, namespace string) {
t.T().Helper()
sampleYAMLDir := getSampleYAMLDir(t)
sampleYAMLPath := filepath.Join(sampleYAMLDir, filename)
kubectlCmd := exec.CommandContext(t.Ctx(), "kubectl", "apply", "-f", sampleYAMLPath, "-n", namespace)
err := kubectlCmd.Run()
assert.NoError(t.T(), err)
t.T().Logf("Successfully applied %s", filename)
}

func IsPodRunningAndReady(pod *corev1.Pod) bool {
if pod.Status.Phase != corev1.PodRunning {
return false
Expand Down
57 changes: 57 additions & 0 deletions ray-operator/test/support/yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package support

import (
"os"
"os/exec"

"github.com/stretchr/testify/assert"

"k8s.io/apimachinery/pkg/runtime"

rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
rayscheme "github.com/ray-project/kuberay/ray-operator/pkg/client/clientset/versioned/scheme"
)

func deserializeYAML(filename string, into runtime.Object) error {
yamlFileContent, err := os.ReadFile(filename)
if err != nil {
return err
}
decoder := rayscheme.Codecs.UniversalDecoder()
if _, _, err = decoder.Decode(yamlFileContent, nil, into); err != nil {
return err
}
return nil
}

func DeserializeRayClusterYAML(t Test, filename string) *rayv1.RayCluster {
t.T().Helper()
rayCluster := &rayv1.RayCluster{}
err := deserializeYAML(filename, rayCluster)
assert.NoError(t.T(), err)
return rayCluster
}

func DeserializeRayJobYAML(t Test, filename string) *rayv1.RayJob {
t.T().Helper()
rayJob := &rayv1.RayJob{}
err := deserializeYAML(filename, rayJob)
assert.NoError(t.T(), err)
return rayJob
}

func DeserializeRayServiceYAML(t Test, filename string) *rayv1.RayService {
t.T().Helper()
rayService := &rayv1.RayService{}
err := deserializeYAML(filename, rayService)
assert.NoError(t.T(), err)
return rayService
}

func KubectlApplyYAML(t Test, filename string, namespace string) {
t.T().Helper()
kubectlCmd := exec.CommandContext(t.Ctx(), "kubectl", "apply", "-f", filename, "-n", namespace)
err := kubectlCmd.Run()
assert.NoError(t.T(), err)
t.T().Logf("Successfully applied %s", filename)
}
Loading