[RayJob] Add e2e sample yaml test for shutdownAfterJobFinishes #1269

architkulkarni · 2023-07-25T17:34:23Z

Why are these changes needed?

This PR adds an end to end test for shutdownAfterJobFinishes. It tests that the RayCluster is deleted after the job completes.

Potential followups:

Test that the underlying Kubernetes job (the submitterPod which runs ray job submit) is also deleted

Related issue number

Closes #1200

Checks

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

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-07-25T18:17:28Z

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

+  #    }
+  #}'
+  # ShutdownAfterJobFinishes specifies whether the RayCluster should be deleted after the RayJob finishes. Default is false.
+  shutdownAfterJobFinishes: true


The only change in the new sample YAML is setting shutdownAfterJobFinishes to true and setting ttlSecondsAfterFinished.

It's not ideal to copy the entire YAML file and only change one line, because if this YAML file needs to be updated then it's hard to remember to update the other one. Another approach would be to update shutdownAfterJobFinishes in the original YAML file at runtime, but it doesn't seem to fit in well with the current test framework (which accepts a list of sample YAML files to test.)

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-07-25T18:36:14Z

I also tried to add a test in https://github.com/ray-project/kuberay/blob/master/ray-operator/controllers/ray/rayjob_controller_test.go but I couldn't get it to work. I think it might be related to https://book.kubebuilder.io/reference/envtest.html#testing-considerations.

kevin85421 · 2023-07-25T18:54:54Z

Is this necessary to be included in v0.6.0?

architkulkarni · 2023-07-25T19:01:32Z

Is this necessary to be included in v0.6.0?

No, it just adds a test. No code changes were necessary to make the test pass.

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-07-27T16:16:46Z

The test passed: https://github.com/ray-project/kuberay/actions/runs/5674665019/job/15378729972?pr=1269#step:3:1737

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

kevin85421 · 2023-08-01T17:31:35Z

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

+apiVersion: ray.io/v1alpha1
+kind: RayJob
+metadata:
+  name: rayjob-sample


Perhaps we could come up with a new name for this CR.

kevin85421 · 2023-08-01T17:54:30Z

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

+                  name: dashboard
+                - containerPort: 10001
+                  name: client
+                - containerPort: 8000


Why do we need this port?

You're right, I think we can remove it. I think it's the default Ray Serve port, but it's not needed for Ray Job.

kevin85421 · 2023-08-01T18:11:44Z

tests/framework/prototype.py

+class ShutdownJobRule(Rule):
+    """Check the Ray cluster is shutdown when setting `spec.shutdownAfterJobFinishes` to true."""
+    def assert_rule(self, custom_resource=None, cr_namespace='default'):
+        custom_api = client.CustomObjectsApi()


Multiple K8s API client instances will introduce unstability. You can reuse K8S_CLUSTER_MANAGER.k8s_client_dict[CONST.K8S_CR_CLIENT_KEY] instead.

Oh I didn't know this, thanks for the heads up!

kevin85421 · 2023-08-01T18:12:17Z

tests/framework/prototype.py

        converge = False
        k8s_v1_api = K8S_CLUSTER_MANAGER.k8s_client_dict[CONST.K8S_V1_CLIENT_KEY]
+        custom_api = client.CustomObjectsApi()


kevin85421 · 2023-08-01T18:17:28Z

tests/framework/prototype.py

+        steps = "spec.shutdownAfterJobFinishes".split('.')
+        value = search_path(custom_resource, steps)
+        logger.info("ShutdownJobRule trigger_condition(): %s", value)
+        return value is not None and value


value seems to be a string here. In my understanding, value is not None and value will return a string.

Oh, great catch!

Actually value seems to be a bool. I added an assertion just to be safe: https://github.com/ray-project/kuberay/pull/1269/files#diff-665dbdd284a6d0222ec3ddb2710957546d6c3974808765d71fb73d119d4cbef8R277

assert isinstance(value, bool) or value is None

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

kevin85421 · 2023-08-01T18:21:26Z

ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

+    class Counter:
+        def __init__(self):
+            # Used to verify runtimeEnv
+            self.name = os.getenv("counter_name")


Should we add an assertion?

Good idea, I can also add one in the requests version check

architkulkarni · 2023-08-01T18:55:28Z

@kevin85421 Thanks for the review. The new CR is just a copy of the existing RayJob sample CR, the only change is the shutdownAfterJobFinishes field.

In general what's our philosophy for how to write tests for these fields? Maintaining a whole new CR with just one field changed doesn't seem like the best approach. But the e2e test framework requires a list of sample YAMLs as input. Is it acceptable to create the modified sample YAML file at runtime?

Co-authored-by: Kai-Hsun Chen <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

kevin85421 · 2023-08-01T19:58:42Z

Is it acceptable to create the modified sample YAML file at runtime?

Yes.
https://github.com/ray-project/kuberay/blob/master/tests/test_sample_rayservice_yamls.py#L223-L227

Signed-off-by: Archit Kulkarni <[email protected]>

…/kuberay into add-shutdown-test

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-08-02T20:27:23Z

@kevin85421 Thanks for the review! Comments have been addressed, please take another look.

For now I just included two sample YAML files, but in the future when we add more tests I can refactor the RayJob test runner to look more like the RayService test.

The test passed with the expected behavior:

Test 0 (no shutdown): https://github.com/ray-project/kuberay/actions/runs/5742686996/job/15566967892#step:3:1571

2023-08-02:20:12:49,990 INFO     [prototype.py:276] ShutdownJobRule trigger_condition(): None

Test 1 (shutdown): https://github.com/ray-project/kuberay/actions/runs/5742686996/job/15566967892#step:3:1596

2023-08-02:20:13:54,239 INFO     [prototype.py:276] ShutdownJobRule trigger_condition(): True
2023-08-02:20:13:54,239 INFO     [prototype.py:253] Waiting for RayCluster to be deleted...
[...]
2023-08-02:20:14:00,286 INFO     [prototype.py:260] ShutdownJobRule wait() hasn't converged yet.
2023-08-02:20:14:00,287 INFO     [prototype.py:261] Number of RayClusters: 1
2023-08-02:20:14:01,294 INFO     [prototype.py:260] ShutdownJobRule wait() hasn't converged yet.
2023-08-02:20:14:01,294 INFO     [prototype.py:261] Number of RayClusters: 0
2023-08-02:20:14:01,294 INFO     [prototype.py:268] RayCluster has been deleted.

kevin85421

LGTM! Would you mind opening a follow-up PR to link the YAML files as examples in the RayJob doc?

…roject#1269) This PR adds an end to end test for shutdownAfterJobFinishes. It tests that the RayCluster is deleted after the job completes. Potential followups: Test that the underlying Kubernetes job (the submitterPod which runs ray job submit) is also deleted Related issue number Closes ray-project#1200 --------- Signed-off-by: Archit Kulkarni <[email protected]> Co-authored-by: Kai-Hsun Chen <[email protected]>

architkulkarni added 2 commits July 25, 2023 10:33

Add e2e sample yaml test

3dd7d90

Signed-off-by: Archit Kulkarni <[email protected]>

Add shutdown sample yaml

99cf629

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni commented Jul 25, 2023

View reviewed changes

architkulkarni added 4 commits July 25, 2023 11:21

Add log to figure out if new test actually ran

fb7b17d

Signed-off-by: Archit Kulkarni <[email protected]>

Add comment for fields in sample yaml

e387887

Signed-off-by: Archit Kulkarni <[email protected]>

Merge branch 'master' into add-shutdown-test

a8bb3e8

Signed-off-by: Archit Kulkarni <[email protected]>

Fix ordering

1960bc4

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni requested a review from kevin85421 July 25, 2023 18:34

architkulkarni marked this pull request as ready for review July 25, 2023 18:34

architkulkarni added 5 commits July 25, 2023 12:07

Add check for rayclusters=1 to prevent test spuriously passing

2c47561

Signed-off-by: Archit Kulkarni <[email protected]>

Fix wrong namespace

20f6c2c

Signed-off-by: Archit Kulkarni <[email protected]>

debug print

22d55f1

Signed-off-by: Archit Kulkarni <[email protected]>

Fix len(rayclusters) calculation

e235fa1

Signed-off-by: Archit Kulkarni <[email protected]>

Raise error if deletion fails

b4ed626

Signed-off-by: Archit Kulkarni <[email protected]>

kevin85421 reviewed Aug 1, 2023

View reviewed changes

Update ray-operator/config/samples/ray_v1alpha1_rayjob.shutdown.yaml

25606d6

Co-authored-by: Kai-Hsun Chen <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni added 4 commits August 1, 2023 16:47

Assert value is bool

45d1cf0

Signed-off-by: Archit Kulkarni <[email protected]>

Merge branch 'add-shutdown-test' of https://github.com/architkulkarni…

904faed

…/kuberay into add-shutdown-test

Add asserts

63722bd

Signed-off-by: Archit Kulkarni <[email protected]>

Reuse custom API client from dict

70f144c

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni requested a review from kevin85421 August 7, 2023 23:36

kevin85421 approved these changes Aug 8, 2023

View reviewed changes

architkulkarni merged commit 7386427 into ray-project:master Aug 8, 2023
18 checks passed

architkulkarni mentioned this pull request Aug 8, 2023

[Enhancement] [Doc] Link shutdown sample YAML in RayJob doc #1302

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RayJob] Add e2e sample yaml test for shutdownAfterJobFinishes #1269

[RayJob] Add e2e sample yaml test for shutdownAfterJobFinishes #1269

architkulkarni commented Jul 25, 2023 •

edited

Loading

architkulkarni Jul 25, 2023 •

edited

Loading

architkulkarni Jul 25, 2023

architkulkarni commented Jul 25, 2023

kevin85421 commented Jul 25, 2023

architkulkarni commented Jul 25, 2023

architkulkarni commented Jul 27, 2023

kevin85421 Aug 1, 2023

kevin85421 Aug 1, 2023

architkulkarni Aug 1, 2023

kevin85421 Aug 1, 2023

architkulkarni Aug 1, 2023

kevin85421 Aug 1, 2023

kevin85421 Aug 1, 2023

architkulkarni Aug 1, 2023

architkulkarni Aug 2, 2023

kevin85421 Aug 1, 2023

architkulkarni Aug 1, 2023

architkulkarni commented Aug 1, 2023

kevin85421 commented Aug 1, 2023

architkulkarni commented Aug 2, 2023 •

edited

Loading

kevin85421 left a comment

[RayJob] Add e2e sample yaml test for shutdownAfterJobFinishes #1269

[RayJob] Add e2e sample yaml test for shutdownAfterJobFinishes #1269

Conversation

architkulkarni commented Jul 25, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

architkulkarni Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Jul 25, 2023

kevin85421 commented Jul 25, 2023

architkulkarni commented Jul 25, 2023

architkulkarni commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Aug 1, 2023

kevin85421 commented Aug 1, 2023

architkulkarni commented Aug 2, 2023 • edited Loading

kevin85421 left a comment

Choose a reason for hiding this comment

architkulkarni commented Jul 25, 2023 •

edited

Loading

architkulkarni Jul 25, 2023 •

edited

Loading

architkulkarni commented Aug 2, 2023 •

edited

Loading