Skip to content

Add RayCLuster SDK Oauth Authentication test #449

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

Conversation

Srihari1192
Copy link
Contributor

@Srihari1192 Srihari1192 commented Jan 23, 2024

Issue link

closes https://issues.redhat.com/browse/RHOAIENG-55

What changes have been made

  • Added tests for RayCluster SDK Oauth Authentication scenarios with and without login using python
  • Added pytest marker for each tests for selecting tests to run on kind or openshift cluster environment
  • Refactored setup methods kube configuration , create and delete namespace in support.py

Verification steps

Checks

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2024
@Srihari1192 Srihari1192 requested a review from sutaakar January 23, 2024 07:48
@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch 3 times, most recently from eefec6f to d494d2e Compare January 25, 2024 07:26
@Srihari1192 Srihari1192 marked this pull request as ready for review January 25, 2024 07:42
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2024
@Srihari1192 Srihari1192 marked this pull request as draft February 2, 2024 10:51
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch from 6f60dfc to 4123677 Compare February 2, 2024 11:11
@Srihari1192 Srihari1192 marked this pull request as ready for review February 2, 2024 11:14
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@Srihari1192 Srihari1192 requested a review from sutaakar February 2, 2024 11:14
@Srihari1192 Srihari1192 marked this pull request as draft February 2, 2024 13:24
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@Srihari1192
Copy link
Contributor Author

Will convert this test to python tests similar to PR #451

@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch 2 times, most recently from 95c4dc3 to 361a9ec Compare February 9, 2024 09:43
@Srihari1192 Srihari1192 marked this pull request as ready for review February 9, 2024 10:01
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2024
@openshift-ci openshift-ci bot requested a review from KPostOffice February 9, 2024 10:01
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

I ran through these changes with poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_oauth_test.py -m openshift and I received this error.

tests/e2e/mnist_raycluster_sdk_oauth_test.py:65: in run_mnist_raycluster_sdk_oauth
    self.assert_jobsubmit_withlogin(cluster)
tests/e2e/mnist_raycluster_sdk_oauth_test.py:105: in assert_jobsubmit_withlogin
    status = job.status()
../.local/lib/python3.11/site-packages/codeflare_sdk/job/jobs.py:201: in status
    return self._runner.status(self._app_handle)
../.local/lib/python3.11/site-packages/torchx/runner/api.py:419: in status
    desc = scheduler.describe(app_id)
../.local/lib/python3.11/site-packages/torchx/schedulers/ray_scheduler.py:383: in describe
    job_status_info = self._get_job_status(app_id)
../.local/lib/python3.11/site-packages/torchx/schedulers/ray_scheduler.py:376: in _get_job_status
    client = self._get_ray_client(job_submission_netloc=addr)

With the final pytest error being:
FAILED tests/e2e/mnist_raycluster_sdk_oauth_test.py::TestRayClusterSDKOauth::test_mnist_ray_cluster_sdk_auth - ValueError: client netloc (ray-dashboard-mnist-test-ns-6t2pj.apps.rosa.mcampbel.n399.p3.openshiftapps.com) does not match job netloc (ray)

Did you come across something like this when testing @Srihari1192

@Srihari1192
Copy link
Contributor Author

Srihari1192 commented Feb 9, 2024

poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_oauth_test.py

@Bobbins228 no it works fine on my cluster
I didnt added import ray as assumed not required , is it failing because of this?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Just one nitpick there. After that, lgtm

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2024
@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch from b2c34ff to 34567dc Compare February 12, 2024 12:36
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

It seems to just need a rebase, afterwards, /lgtm

Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2024
@sutaakar
Copy link
Contributor

@Srihari1192 Can you adjust https://github.com/project-codeflare/codeflare-sdk/blob/main/.github/workflows/e2e_tests.yaml#L126 to run all tests with proper kind marker?

@sutaakar
Copy link
Contributor

Can you add timeout for tests?
From my local experiments it seems that https://pypi.org/project/pytest-timeout/ works quite well.

@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch from 3785a49 to 190e0da Compare February 13, 2024 11:58
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2024
@Srihari1192 Srihari1192 marked this pull request as draft February 13, 2024 12:28
@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch 2 times, most recently from 0c912e9 to b4a1af4 Compare February 13, 2024 14:02
@Srihari1192 Srihari1192 force-pushed the raycluster-sdk-auth-test branch from b4a1af4 to 067b3d0 Compare February 13, 2024 14:03
@Srihari1192 Srihari1192 marked this pull request as ready for review February 13, 2024 14:09
@openshift-ci openshift-ci bot requested a review from Maxusmusti February 13, 2024 14:10
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2024
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

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

Awesome stuff Srihari works like a charm 🥇
/approve

Copy link
Contributor

openshift-ci bot commented Feb 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobbins228, ChristianZaccaria, sutaakar

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 91356f4 into project-codeflare:main Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants