Skip to content

[Refactor] Improve developer experience of API server e2e-test#3466

Merged
kevin85421 merged 11 commits intoray-project:masterfrom
JiangJiaWei1103:improve-apiserver-e2e-test-dx
May 2, 2025
Merged

[Refactor] Improve developer experience of API server e2e-test#3466
kevin85421 merged 11 commits intoray-project:masterfrom
JiangJiaWei1103:improve-apiserver-e2e-test-dx

Conversation

@JiangJiaWei1103
Copy link
Contributor

Changes

  • Remove run from the start-local-apiserver target
  • Simplify prerequisites for the local-e2e-test target by replacing multiple dependencies with a single start-local-apiserver target

Why are these changes needed?

There are currently two ways to run API server e2e tests:

  1. Run make start-local-apiserver followed by make e2e-test
    This approach is useful during test development, allowing quick iterations without rebuilding the operator image, restarting the kind cluster, and redeploying the API server within the cluster.

  2. Run make local-e2e-test
    This is a convenient all-in-one command for running the full e2e test workflow, including the cluster setup, running the e2e test, and tearing down the cluster.

However, the existing setup includes redundant dependencies that may confuse developers and make it harder to choose the best workflow. This PR streamlines the local e2e test process and improve the overall developer experience.

Related issue number

Closes #3384

Checks

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

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
@JiangJiaWei1103
Copy link
Contributor Author

@dentiny PTAL, thanks!

Follow-up

Once these changes are approved, I'll update the README.md and DEVELOPMENT.md to reflect the latest setup for running e2e tests in different scenarios.

Discussion

It appears that the load-ray-test-image target is not used during the e2e test process. Would it make sense to remove this from the local-e2e-test target to further improve the efficiency?

cc @machichima

@JiangJiaWei1103 JiangJiaWei1103 marked this pull request as ready for review April 23, 2025 14:13

.PHONY: start-local-apiserver ## Build and start apiserver from scratch.
start-local-apiserver: operator-image cluster load-operator-image deploy-operator install run
start-local-apiserver: operator-image cluster load-operator-image deploy-operator install
Copy link
Collaborator

@machichima machichima Apr 23, 2025

Choose a reason for hiding this comment

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

Some note here, as install will build and deploy the apiserver to cluster., we think that running the apiserver locally is not needed, as it is already on the cluster

@dentiny
Copy link
Contributor

dentiny commented Apr 24, 2025

Once these changes are approved, I'll update the README.md and DEVELOPMENT.md to reflect the latest setup for running e2e tests in different scenarios.

If it could be inconsistency for readme, can we do that in this PR?
The main reason is I don't want users to get confused under any circumstances, thank you!

@JiangJiaWei1103
Copy link
Contributor Author

Once these changes are approved, I'll update the README.md and DEVELOPMENT.md to reflect the latest setup for running e2e tests in different scenarios.

If it could be inconsistency for readme, can we do that in this PR? The main reason is I don't want users to get confused under any circumstances, thank you!

No problem, I'll fix the docs in this PR. thanks!

JiangJiaWei1103 and others added 5 commits April 25, 2025 20:59
Signed-off-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
@JiangJiaWei1103
Copy link
Contributor Author

Hi @dentiny and @machichima,

I've updated the documentations to reflect the latest API server setup. Please let me know if there's any problem. Thanks!

```

### Install KubeRay Operator
#### Install KubeRay Operator
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, why do we make kuberay operator installation a subsection for helm installation?

1. Install the KubeRay Operator into the cluster by running `make operator-image load-operator-image deploy-operator`
2. Install the KubeRay API server into the cluster by running `make install`
3. Verify the setup with a smoke test `curl localhost:31888`
* You should see a response like `{"code":5, "message":"Not Found"}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The response with message "Not Found" is a bit confusing while it represent the connection is valid. Consider putting command like curl -I localhost:31888/healthz here, which provides a more informative response.

❯ curl -I localhost:31888/healthz
HTTP/1.1 200 OK
Date: Mon, 28 Apr 2025 14:36:35 GMT

3. Verify the setup with a smoke test `curl localhost:31888`
* You should see a response like `{"code":5, "message":"Not Found"}`

> Alternatively, you can simply run `make start-local-apiserver` to spin up the API server within the kind cluster in one single command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add a small note here that if we modify the code for apiserver, the make install should be re-run?

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
@machichima
Copy link
Collaborator

@JiangJiaWei1103 LGTM, Thanks!

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Copy link
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

Thanks!

@dentiny dentiny requested a review from kevin85421 May 1, 2025 10:00
@kevin85421 kevin85421 merged commit 4ff8316 into ray-project:master May 2, 2025
23 checks passed
@JiangJiaWei1103 JiangJiaWei1103 deleted the improve-apiserver-e2e-test-dx branch May 3, 2025 00:32
win5923 pushed a commit to win5923/kuberay that referenced this pull request May 3, 2025
…roject#3466)

* refactor: Simplify the API server launch logic

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Polish help msg of local-e2e-test

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Structure apiserver installation with Helm

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Update the API server setup

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Run apiserver smoke test with a health check endpoint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Add notes for rebuilding and reploying API server

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Teardown the existing API server deployment

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* fix: Solve md  lint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

---------

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com>
laurafitzgerald pushed a commit to laurafitzgerald/kuberay that referenced this pull request May 7, 2025
…roject#3466)

* refactor: Simplify the API server launch logic

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Polish help msg of local-e2e-test

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Structure apiserver installation with Helm

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Update the API server setup

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Run apiserver smoke test with a health check endpoint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Add notes for rebuilding and reploying API server

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Teardown the existing API server deployment

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* fix: Solve md  lint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

---------

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com>
laurafitzgerald pushed a commit to laurafitzgerald/kuberay that referenced this pull request May 8, 2025
…roject#3466)

* refactor: Simplify the API server launch logic

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Polish help msg of local-e2e-test

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Structure apiserver installation with Helm

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Update the API server setup

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Run apiserver smoke test with a health check endpoint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Add notes for rebuilding and reploying API server

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* docs: Teardown the existing API server deployment

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

* fix: Solve md  lint

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>

---------

Signed-off-by: jiangjiawei1103 <waynechuang97@gmail.com>
Signed-off-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Enhance the convenience in running e2e-test

4 participants