[Feature] Test sample RayService YAML to catch invalid or out of date one #731

jasoonn · 2022-11-16T03:46:18Z

Why are these changes needed?

Use #605 to test sample RayService YAML.
The test includes the following two checking rules.

It is able to submit a simple job to the created raycluster.
It is able to use curl in the pod to access the service.

Why we choose Subprocess instead of os.system

According to doc of os.system, the subprocess module provides more powerful facilities for spawning new processes and retrieving their results; using subprocess is preferable to using this function.

shell=True in subprocess.run(): The specified command will be executed through the shell.
check=True in some of subprocess.run(): If the process exits with a non-zero exit code, a CalledProcessError exception will be raised.

Why removing the `kubectl wait` when creating kind cluster

kuberay/tests/framework/prototype.py

Lines 46 to 50 in 12225db

    
           def create_kind_cluster(): 
        
               """Create a KinD cluster""" 
        
               shell_subprocess_run("kind create cluster") 
        
               shell_subprocess_run( 
        
                   "kubectl wait --for=condition=ready pod -n kube-system --all --timeout=900s")

Sometimes when calling "kubectl wait --for=condition=ready pod -n kube-system --all --timeout=900s", it will report an error error: no matching resources found(example log in line 1157), which I think because the namespace has not yet been created. When this situation happened, the test in the past still passed because we only use os.system() to execute the command and do not check the return code. In this pr, we updated os.system to subprocess.run(..., check=True) which will automatically raise an error when the return code of the exection is not 0. To solve this, we utilize --wait flag in kind to substitute the kubectl wait functionality.

Why we set `tail=-1` in getting log?

kuberay/tests/framework/prototype.py

Line 128 in 48b0b05

    
           shell_subprocess_run(f'kubectl logs -n={cr_namespace} -l ray.io/node-type=head --tail=-1')

The argument tail default is -1 with no selector, showing all log lines. However, we utilize a selector in this case. Hence, we need to specify --tail=-1 to display all of the log.

Related issue number

Closes #719

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

cd tests/framework
python3 test_sample_rayservice_yamls.py 2>&1 | tee log
# I have run more than ten times on my cluster, and the result is always pass.

kevin85421

Thank @jasoonn for your contribution!

.github/workflows/actions/configuration/action.yaml

tests/framework/prototype.py

jasoonn · 2022-11-16T16:34:55Z

@kevin85421 Thanks for the suggestions. I have updated the pr.

tests/framework/prototype.py

kevin85421 · 2022-11-16T19:20:45Z

@shrekris-anyscale Can you help review the wait function of RayServiceAddCREvent? The function waits until RayService is ready to serve traffic. Thank you!

jasoonn · 2022-11-17T03:39:03Z

@kevin85421 Thanks for the suggestions. I have updated the pr.

kevin85421

There are too many shell = ..., check = .... Can we simplify it?

tests/framework/prototype.py

jasoonn · 2022-11-18T18:24:01Z

@kevin85421 Thanks for the suggestion. I have updated the pr.

tests/framework/prototype.py

jasoonn · 2022-11-18T23:51:57Z

@kevin85421 Thanks for the suggestion. I have updated the pr.

kevin85421

LGTM

sihanwang41 · 2022-12-07T22:40:07Z

Hi @kevin85421 @jasoonn , is this test enabled in ci? (I don't see it is enabled.)

kevin85421 · 2022-12-07T23:01:42Z

Hi @kevin85421 @jasoonn , is this test enabled in ci? (I don't see it is enabled.)

@sihanwang41 No, KubeRay CI (2 CPUs) does not have enough CPUs to run this test. I will deploy these tests in Ray CI (#695).

… one (ray-project#731) Use ray-project#605 to test sample RayService YAML. The test includes the following two checking rules. 1. It is able to [submit a simple job](https://github.com/jasoonn/kuberay/blob/69efe590970f9d8a4fe0fff66300049f008c6ae9/tests/framework/prototype.py#L188-L197) to the created raycluster. 2. It is able to use [curl in the pod](https://github.com/jasoonn/kuberay/blob/69efe590970f9d8a4fe0fff66300049f008c6ae9/tests/framework/prototype.py#L199-L216) to access the service.

jasoonn added 4 commits November 16, 2022 01:48

update proto.py and ray-service test

91dfb10

update gitaction

1d737b6

remove unused print

affb63a

remove log and __pycache__

69efe59

kevin85421 self-requested a review November 16, 2022 04:59

kevin85421 reviewed Nov 16, 2022

View reviewed changes

jasoonn added 2 commits November 16, 2022 15:40

Update

cb25356

extract check_pod_running

1905636

jasoonn changed the title ~~Ray service test~~ [Feature] Test sample RayServiceYAML to catch invalid or out of date one Nov 16, 2022

kevin85421 reviewed Nov 16, 2022

View reviewed changes

tests/framework/prototype.py Outdated Show resolved Hide resolved

tests/framework/prototype.py Outdated Show resolved Hide resolved

tests/framework/prototype.py Outdated Show resolved Hide resolved

tests/framework/prototype.py Outdated Show resolved Hide resolved

kevin85421 mentioned this pull request Nov 16, 2022

[Feature] Migrate CI infra from GitHub Actions to Buildkite #695

Closed

2 tasks

jasoonn added 2 commits November 16, 2022 23:42

os.system to subprocess

6ff58de

fix type

0937f5e

kevin85421 reviewed Nov 17, 2022

View reviewed changes

kevin85421 changed the title ~~[Feature] Test sample RayServiceYAML to catch invalid or out of date one~~ [Feature] Test sample RayService YAML to catch invalid or out of date one Nov 17, 2022

jasoonn added 2 commits November 18, 2022 17:58

update

48b0b05

update

12225db

kevin85421 reviewed Nov 18, 2022

View reviewed changes

tests/framework/prototype.py Show resolved Hide resolved

tests/framework/prototype.py Show resolved Hide resolved

jasoonn added 2 commits November 18, 2022 23:22

update

6c6f18c

update

2d1b48b

jasoonn added 2 commits November 19, 2022 03:44

add logging

30189b2

update wait for creating cluster

eab585b

kevin85421 approved these changes Nov 19, 2022

View reviewed changes

kevin85421 merged commit 65a7703 into ray-project:master Nov 19, 2022

This was referenced Dec 13, 2022

[Feature] Move some functions from prototype test framework to a new utils file #837

Merged

[Bug] A mini RayService requires more than 90 seconds to converge #838

Closed

	def create_kind_cluster():
	"""Create a KinD cluster"""
	shell_subprocess_run("kind create cluster")
	shell_subprocess_run(
	"kubectl wait --for=condition=ready pod -n kube-system --all --timeout=900s")

[Feature] Test sample RayService YAML to catch invalid or out of date one #731

[Feature] Test sample RayService YAML to catch invalid or out of date one #731

Uh oh!

Conversation

jasoonn commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Why we choose Subprocess instead of os.system

Why removing the kubectl wait when creating kind cluster

Why we set tail=-1 in getting log?

Related issue number

Checks

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasoonn commented Nov 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevin85421 commented Nov 16, 2022

Uh oh!

jasoonn commented Nov 17, 2022

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasoonn commented Nov 18, 2022

Uh oh!

Uh oh!

Uh oh!

jasoonn commented Nov 18, 2022

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

sihanwang41 commented Dec 7, 2022

Uh oh!

kevin85421 commented Dec 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasoonn commented Nov 16, 2022 •

edited

Loading

Why removing the `kubectl wait` when creating kind cluster

Why we set `tail=-1` in getting log?