Skip to content

Add Ray cluster REST API support in test support package #197

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.

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

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

sutaakar
Copy link
Contributor

@sutaakar sutaakar commented Jul 27, 2023

Issue link

Part of opendatahub-io/distributed-workloads#63

What changes have been made

Adjustments in e2e test support package to provide support for new ODH integration test - opendatahub-io/distributed-workloads#71

Verification steps

The changes can be verified by running ODH integration test, see instructions in opendatahub-io/distributed-workloads#71

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

It is enough to run the local e2e tests (part of the PR check) to make sure nothing is broken.

@sutaakar
Copy link
Contributor Author

seems that k8s api upgrade is too big change for a codebase, need to find compatible OpenShift client versions (dependency hell strikes back)

@sutaakar sutaakar force-pushed the ray-cluster branch 3 times, most recently from d04b9b0 to 48f7ac5 Compare July 27, 2023 13:38
@sutaakar sutaakar marked this pull request as draft July 27, 2023 13:47
@sutaakar sutaakar force-pushed the ray-cluster branch 2 times, most recently from 3af0cee to 06b57b7 Compare July 27, 2023 13:53
return RayJobAPIDetails(t, rayClient, jobID)(t)
}

func WriteRayJobAPILogs(t Test, rayClient RayClusterClient, jobID string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to remove the GetRayJobLogs and WriteRayJobLogs functions from the ray.go file, that rely on the API server proxy, and have them replaced by these one, so there are used in the tests upstream as well.

For it to work, we can install the NGINX ingress controller in KinD (The KinD configuration already contains the "ingress-ready=true" kubelet extra argument), and create an Ingress resource in the TestMNISTRayJobMCADRayCluster test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about creating a follow up task for this. Not sure if I will have enough time to address it tomorrow, before my vacation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds good. I can take of it 👍🏼.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@astefanutti Created #199

@sutaakar sutaakar marked this pull request as ready for review July 27, 2023 15:21
@astefanutti
Copy link
Contributor

/lgtm

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit a822747 into project-codeflare:main Jul 27, 2023
@sutaakar sutaakar deleted the ray-cluster branch July 28, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants