Convert Go e2e test to Python Test to leverage SDK directly #451

ChristianZaccaria · 2024-01-30T17:56:50Z

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-2058

What changes have been made

Converted Go e2e test to a Python test to leverage the SDK directly.
Removed unnecessary RBAC, ConfigMap, and Job specs.
Removed Python script and moved its contents directly into the test file, this way allowing to perform assertions in the test at any given point.
Adjusted ingress_options in the script to include this annotation: nginx.ingress.kubernetes.io/proxy-body-size: "100M", else, on KinD, we would encounter the following error:

E       RuntimeError: Request failed with status code 413: <html>
E       <head><title>413 Request Entity Too Large</title></head>

Adjusted e2e workflow + added sdk-user with limited permissions on KinD cluster to run the e2e tests.

Verification steps

This PR: [e2e] Add Host to Container port mapping for KinD Cluster codeflare-common#30 should be merged first to test the e2e workflow as the KinD cluster is created based on that configuration.
This PR: Add Host to Container port mapping for KinD Cluster codeflare-operator#456 should be merged first for testing the e2e test locally (or add changes in codeflare-operator kind.sh script).

To run the e2e test locally on KinD cluster, follow these steps:

Note: If not already done so, please add in your local /etc/hosts file 127.0.0.1 kind. This will map your localhost IP address to the KinD cluster's hostname. This is already performed on GitHub Actions

Setup Phase:

Pull the codeflare-operator repo and run the following make targets:

make kind-e2e
export CLUSTER_HOSTNAME=kind
export CODEFLARE_TEST_TIMEOUT_LONG=20m
make deploy -e IMG=quay.io/project-codeflare/codeflare-operator:v1.1.0
make setup-e2e

Test Phase:
- Once we have the codeflare-operator and kuberay-operator running and ready, we can run the e2e test on the codeflare-sdk repo:
```
poetry install --with test,docs
poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_test.py
```

To run the e2e test locally on OpenShift, follow these steps:

Setup Phase:

Pull the codeflare-operator repo and run the following make targets:

make deploy -e IMG=quay.io/project-codeflare/codeflare-operator:v1.1.0
make setup-e2e

Test Phase:
- Once we have the codeflare-operator and kuberay-operator running and ready, we can run the e2e test on the codeflare-sdk repo:
```
poetry install --with test,docs
poetry run pytest -v -s ./tests/e2e/mnist_raycluster_sdk_test.py
```

Checks

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

tests/e2e/mnist_raycluster_sdk_test.py

tests/e2e/support.py

Bobbins228

/lgtm

tests/e2e/mnist_raycluster_sdk_test.py

sutaakar

/lgtm

VanillaSpoon · 2024-02-01T15:45:51Z

This is great :) Ran the tests locally also and they run through as expected
\lgtm

ChristianZaccaria · 2024-02-02T14:20:23Z

I mistakenly pushed to this branch by accident, reverted changes. No new changes have been made - good to merge.

astefanutti

What user is impersonated to run the tests? It seems it's using the default user that's cluster admin, in which case that's a regression from the existing e2e tests?

sutaakar · 2024-02-05T17:21:00Z

project-codeflare/codeflare-common#33 is adding support for custom user. That can be used to specify user with specific allowed roles.
For usage see project-codeflare/codeflare-operator#462

ChristianZaccaria · 2024-02-06T09:56:02Z

@astefanutti @sutaakar , I was told to keep the issues separated where after merging we would add the unprivileged user to run the test: https://issues.redhat.com/browse/RHOAIENG-60

We could add it in the scope of this PR too if preferred.

ChristianZaccaria · 2024-02-06T19:05:12Z

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

astefanutti · 2024-02-07T08:35:12Z

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

For local testing, I'd avoid mixing provisioning and testing logic, so an option would be to document how to setup the cluster (even specific to KinD would be fine) and run the tests with the correct user.

astefanutti · 2024-02-07T08:35:17Z

/lgtm

ChristianZaccaria · 2024-02-07T10:42:10Z

I added in the e2e workflow an sdk-user with limited RBAC permissions who runs the e2e tests. This is currently only performed in the e2e test workflow. For local testing, should I add in the test a script that creates the sdk-user + permissions if it doesn't already exist in the cluster, or should we use the cluster admin for local tests? Thanks @sutaakar @astefanutti

For local testing, I'd avoid mixing provisioning and testing logic, so an option would be to document how to setup the cluster (even specific to KinD would be fine) and run the tests with the correct user.

That makes sense, thanks Antonin! I added e2e documentation there now and updated the readme.

sutaakar

/lgtm

astefanutti · 2024-02-07T16:43:08Z

/approve

Great job!

Bobbins228

/approve

openshift-ci · 2024-02-07T16:46:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti, Bobbins228

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:

~~OWNERS~~ [Bobbins228]

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

ChristianZaccaria requested review from astefanutti, sutaakar and Bobbins228 January 30, 2024 17:56

openshift-ci bot requested review from dimakis and Maxusmusti January 30, 2024 17:56

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024

ChristianZaccaria added 5 commits January 30, 2024 17:58

Convert Go e2e test to Python Test

6cb33d2

Added SDK script directly to e2e test file

d75f4c2

Clean-up e2e test files

d30833e

Update e2e workflow for Poetry PyTest run

7fef05a

Adjust proxy-body-size value

8e29291

ChristianZaccaria force-pushed the python-test branch from 28efa45 to 8e29291 Compare January 30, 2024 17:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024

sutaakar reviewed Jan 31, 2024

View reviewed changes

tests/e2e/mnist_raycluster_sdk_test.py Outdated Show resolved Hide resolved

sutaakar reviewed Jan 31, 2024

View reviewed changes

tests/e2e/support.py Outdated Show resolved Hide resolved

ChristianZaccaria force-pushed the python-test branch from 7df7353 to febd339 Compare January 31, 2024 11:47

Env var for custom Ray Image

b8caf9c

ChristianZaccaria force-pushed the python-test branch 2 times, most recently from a16887b to 61a01df Compare January 31, 2024 15:28

ChristianZaccaria mentioned this pull request Jan 31, 2024

WIP - Convert e2e test to Python test #442

Closed

4 tasks

ChristianZaccaria force-pushed the python-test branch from 0c630a0 to e8ff630 Compare January 31, 2024 16:26

ChristianZaccaria requested a review from sutaakar January 31, 2024 17:09

Bobbins228 reviewed Jan 31, 2024

View reviewed changes

openshift-ci bot assigned Bobbins228 Jan 31, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2024

sutaakar reviewed Feb 1, 2024

View reviewed changes

tests/e2e/mnist_raycluster_sdk_test.py Outdated Show resolved Hide resolved

ChristianZaccaria force-pushed the python-test branch from e8ff630 to f624d23 Compare February 1, 2024 09:48

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024

sutaakar reviewed Feb 1, 2024

View reviewed changes

openshift-ci bot assigned sutaakar Feb 1, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2024

Srihari1192 mentioned this pull request Feb 2, 2024

Add RayCLuster SDK Oauth Authentication test #449

Merged

4 tasks

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2024

Adjust e2e workflow output logs

c53db36

ChristianZaccaria force-pushed the python-test branch from 11e757d to c53db36 Compare February 2, 2024 14:19

astefanutti reviewed Feb 5, 2024

View reviewed changes

Add sdk-user with limited permissions to run e2e test

56e400a

ChristianZaccaria requested review from astefanutti and sutaakar February 6, 2024 18:54

openshift-ci bot assigned astefanutti Feb 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

ChristianZaccaria added 2 commits February 7, 2024 10:27

Local e2e Testing documentation

7117bd8

Add sdk-user to e2e documentation

4a0f95c

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

sutaakar reviewed Feb 7, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

Bobbins228 approved these changes Feb 7, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024

openshift-merge-bot bot merged commit 660f328 into project-codeflare:main Feb 7, 2024

Bobbins228 mentioned this pull request Mar 6, 2024

Create test case for unprivileged user execution #441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Go e2e test to Python Test to leverage SDK directly #451

Convert Go e2e test to Python Test to leverage SDK directly #451

ChristianZaccaria commented Jan 30, 2024 •

edited

Loading

Bobbins228 left a comment

sutaakar left a comment

VanillaSpoon commented Feb 1, 2024

ChristianZaccaria commented Feb 2, 2024

astefanutti left a comment

sutaakar commented Feb 5, 2024

ChristianZaccaria commented Feb 6, 2024

ChristianZaccaria commented Feb 6, 2024

astefanutti commented Feb 7, 2024

astefanutti commented Feb 7, 2024

ChristianZaccaria commented Feb 7, 2024

sutaakar left a comment

astefanutti commented Feb 7, 2024

Bobbins228 left a comment

openshift-ci bot commented Feb 7, 2024

Convert Go e2e test to Python Test to leverage SDK directly #451

Convert Go e2e test to Python Test to leverage SDK directly #451

Conversation

ChristianZaccaria commented Jan 30, 2024 • edited Loading

Issue link

What changes have been made

Verification steps

Checks

Bobbins228 left a comment

Choose a reason for hiding this comment

sutaakar left a comment

Choose a reason for hiding this comment

VanillaSpoon commented Feb 1, 2024

ChristianZaccaria commented Feb 2, 2024

astefanutti left a comment

Choose a reason for hiding this comment

sutaakar commented Feb 5, 2024

ChristianZaccaria commented Feb 6, 2024

ChristianZaccaria commented Feb 6, 2024

astefanutti commented Feb 7, 2024

astefanutti commented Feb 7, 2024

ChristianZaccaria commented Feb 7, 2024

sutaakar left a comment

Choose a reason for hiding this comment

astefanutti commented Feb 7, 2024

Bobbins228 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 7, 2024

ChristianZaccaria commented Jan 30, 2024 •

edited

Loading