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

[RayCluster][CI] add e2e tests for RayClusterStatusCondition #2661

Merged
merged 5 commits into from
Dec 18, 2024
Merged

[RayCluster][CI] add e2e tests for RayClusterStatusCondition #2661

merged 5 commits into from
Dec 18, 2024

Conversation

rueian
Copy link
Contributor

@rueian rueian commented Dec 17, 2024 •
edited
Loading

Why are these changes needed?

Continue #2645.

This PR adds e2e tests for the HeadPodReady, RayClusterProvisioned, and ReplicaFailure status conditions.

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests

@rueian rueian force-pushed the e2e-raycluster-status-condition branch 2 times, most recently from d3b8b47 to 13a3cf8 Compare December 17, 2024 03:41
rueian
rueian commented Dec 17, 2024
View reviewed changes
ray-operator/config/manager/manager.yaml Outdated
@@ -26,7 +26,8 @@ spec:
containers:
- command:
- /manager
# args:
args:
- --feature-gates=RayClusterStatusConditions=true
Copy link
Contributor Author

@rueian rueian Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We need this to make buildkite/ray-ecosystem-ci-kuberay-ci/test-sample-yamls-nightly-operator buildkite/ray-ecosystem-ci-kuberay-ci/test-sample-yamls-latest-release, where the feature gate is not enabled by default (v1.2), to pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that using nightly build though? Nightly build should have this feature gate enabled by default at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I copied the wrong link. I should mean this one buildkite/ray-ecosystem-ci-kuberay-ci/test-sample-yamls-latest-release

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see, makes sense. Can you add a comment that we can remove this once v1.3 is released?

Copy link
Contributor Author

@rueian rueian Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the confusion. The nightly build has this feature gate enabled by default. But in test-sample-yamls-latest-release, we have v1.2 operator where the feature gate is not enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see, makes sense. Can you add a comment that we can remove this once v1.3 is released?

Yes. The comment is added.

@rueian rueian marked this pull request as ready for review December 17, 2024 09:21
@rueian
Copy link
Contributor Author

rueian commented Dec 17, 2024

Hi @andrewsykim, please take a look. Thanks.

andrewsykim
andrewsykim approved these changes Dec 17, 2024
View reviewed changes
ray-operator/config/manager/manager.yaml Outdated
@@ -26,7 +26,8 @@ spec:
containers:
- command:
- /manager
# args:
args:
- --feature-gates=RayClusterStatusConditions=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah I see, makes sense. Can you add a comment that we can remove this once v1.3 is released?

kevin85421
kevin85421 reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Nice

ray-operator/test/support/yaml.go Outdated
t.T().Helper()
kubectlCmd := exec.CommandContext(t.Ctx(), "kubectl", "create", "quota", namespace, "-n", namespace, quota)
err := kubectlCmd.Run()
assert.NoError(t.T(), err)
Copy link
Member

Choose a reason for hiding this comment

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

assert.NoError does not fail immediately; the error is only printed after the test finishes and "Successfully applied quota %s in %s" will always be printed no matter the command succeed or not. Should we make it fail fast?

ray-operator/test/support/yaml.go Outdated
t.T().Helper()
kubectlCmd := exec.CommandContext(t.Ctx(), "kubectl", "delete", "--all", "pods", "-n", namespace)
err := kubectlCmd.Run()
assert.NoError(t.T(), err)
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

@rueian rueian Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! I replaced all assert with require which will fail fast in the yaml.go file. I can file a PR to replace all assert usages with require throughout the entire project if needed.

@kevin85421 kevin85421 self-assigned this Dec 17, 2024
@kevin85421
Copy link
Member

can you fix the conflict?

@rueian rueian force-pushed the e2e-raycluster-status-condition branch from dd39cfb to 8738874 Compare December 18, 2024 00:31
@rueian
Copy link
Contributor Author

rueian commented Dec 18, 2024 •
edited
Loading

can you fix the conflict?

Hi @kevin85421, the conflict came from https://github.com/ray-project/kuberay/pull/2660/files#r1887796931 where the acquire package dependency was removed.

Right now the conflict is resolved by using the require package, but I think relying only on the standard library is also a good idea. Do we now prefer not to use any acquire or require packages? If yes, I can open another PR to remove them.

@kevin85421
Copy link
Member

Right now the conflict is resolved by using the require package, but I think relying only on the standard library is also a good idea. Do we now prefer not to use any acquire or require packages

I don't have any preference.

kevin85421
kevin85421 reviewed Dec 18, 2024
View reviewed changes
ray-operator/test/support/yaml.go Outdated
if err != nil {
t.T().Fatalf("Failed to apply %s to namespace %s: %v", filename, namespace, err)
}
require.NoError(t.T(), err)
Copy link
Member

Choose a reason for hiding this comment

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

I think err is not very informative. From what I recall, it seems like just something similar to "exit 1".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Manual error messages have been added to require.NoError() calls.

@kevin85421 kevin85421 mentioned this pull request Dec 18, 2024
Closed
@kevin85421 kevin85421 merged commit 8362483 into ray-project:master Dec 18, 2024
23 checks passed
@rueian rueian mentioned this pull request Dec 25, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin85421 kevin85421 kevin85421 approved these changes

@andrewsykim andrewsykim andrewsykim approved these changes

Assignees

@kevin85421 kevin85421

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rueian @kevin85421 @andrewsykim