Skip to content
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

Support external tempest plugins #103

Conversation

lpiwowar
Copy link
Contributor

This patch adds support for the tempest container to be able to install external plugins that are not part of the .rpms the container is built with.

When a user defines the TEMPEST_EXTERNAL_PLUGIN_GIT_URLS then plugin(s) defined by this variable will be downloaded and executed. The user can also define TEMPEST_EXTERNAL_PLUGIN_CHANGE_URLS and TEMPEST_EXTERNAL_PLUGIN_REFSPEC variable to download a change(s) for a given plugin. This can be used for testing tests under developoment.

TEMPEST_EXTERNAL_PLUGIN_GIT_URLS
- URL of a git repository that holds the plugin

TEMPEST_EXTERNAL_PLUGIN_CHANGE_URLS
- URL from which a change should be downloaded for a given plugin

TEMPEST_EXTERNAL_PLUGIN_REFSPEC
- A change that should be downloaded for a given plugin

Copy link
Contributor

openshift-ci bot commented Nov 21, 2023

Hi @lpiwowar. Thanks for your PR.

I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lpiwowar lpiwowar force-pushed the feature/tempest-url-download branch 2 times, most recently from 41f7c9d to 64dbb0a Compare November 23, 2023 11:27
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/c8ca506a74d04355a1ab99f7b45cf57a

tcib-build-containers NODE_FAILURE Node request 200-0006664926 failed in 0s
⚠️ tcib-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job tcib-build-containers
⚠️ tcib-podified-multinode-edpm-deployment-crc SKIPPED Skipped due to failed job tcib-build-containers

@kopecmartin
Copy link
Contributor

/ok-to-test

@kopecmartin
Copy link
Contributor

for anyone looking at this PR, the documentation on how this feature would be used is added by this PR (and still being discussed):
openstack-k8s-operators/test-operator#12

@kopecmartin
Copy link
Contributor

is this rebased on the latest changes?

@lpiwowar lpiwowar force-pushed the feature/tempest-url-download branch from 4b15f91 to 81930b2 Compare December 14, 2023 09:05
@lpiwowar
Copy link
Contributor Author

@kopecmartin, yes, it should be. Do you think something is missing?

@lpiwowar lpiwowar requested a review from kopecmartin December 14, 2023 09:08
popd
fi

pip install ./$plugin_name
Copy link
Contributor

Choose a reason for hiding this comment

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

pip/pip3? does it make a difference? I guess pip points to pip3 so it doesn't matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that using pip should be ok here. We are inside a virtual environment.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/5ae50b225cf84b7a82052251cf59d286

✔️ tcib-build-containers SUCCESS in 1h 58m 44s
tcib-crc-podified-edpm-baremetal RETRY_LIMIT in 13m 51s
✔️ tcib-podified-multinode-edpm-deployment-crc SUCCESS in 1h 06m 52s

@kopecmartin kopecmartin requested a review from raukadah December 18, 2023 13:38
@kopecmartin
Copy link
Contributor

/lgtm

Copy link
Contributor

@kopecmartin kopecmartin left a comment

Choose a reason for hiding this comment

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

lgtm

@kopecmartin
Copy link
Contributor

recheck

Copy link

@marios marios left a comment

Choose a reason for hiding this comment

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

is this tested somewhere (eg with testproject) with passing the new vars in?

feels like run_tempest.sh needs a README or at least some comments at the top (like the information you have in your commit message) to document what these vars are/do

git clone $git_url

if [[ ! -z $change_url ]] && [[ ! -z $refspec ]] || \
[[ $change_url != "-" ]] && [[ $refspec != "-" ]]; then
Copy link

Choose a reason for hiding this comment

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

maybe a comment would help but i'm trying to imagine what $change_url looks like here and why we need to guard for != "-" (and failing to imagine 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.

I explained the usage of the variables used here in the comment at the beginning of the script. I can imagine that the "-" is a little bit confusing. I think that it would be used only in edge cases when someone would want to download multiple changes.

I think that the conversation here openstack-k8s-operators/test-operator#12 can be helpful as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it. Not sure if there is a better way to implement it, but I see that you need "-" because your loop gets values based on indexes.
A good documentation was added to the script itself. Is there another place that we can add this? So everybody would know how to use this?

@lpiwowar lpiwowar force-pushed the feature/tempest-url-download branch from 81930b2 to 1531afa Compare December 19, 2023 14:44
@openshift-ci openshift-ci bot removed the lgtm label Dec 19, 2023
@lpiwowar
Copy link
Contributor Author

I answered your questions @marios here:) 👇

is this tested somewhere (eg with testproject) with passing the new vars in?

It feels like run_tempest.sh needs a README or at least some comments at the top (like the information you have in your commit message) to document what these vars are/do?

  • It's a good point. The run_tempest.sh script is slowly growing. I added a comment at the beginning of the script that documents the variables consumed by the script.

@lpiwowar lpiwowar force-pushed the feature/tempest-url-download branch 2 times, most recently from f5719b3 to a8082d8 Compare December 19, 2023 15:08
This patch adds support for the tempest container to be able to install
external plugins that are not part of the .rpm the container is
built with.

When a user defines the TEMPEST_EXTERNAL_PLUGIN_GIT_URLS then plugin(s)
defined by this variable will be downloaded and executed. The user
can also define TEMPEST_EXTERNAL_PLUGIN_CHANGE_URLS and
TEMPEST_EXTERNAL_PLUGIN_REFSPEC variable to download a change(s) for a
given plugin. This can be used for testing tests under developoment.

TEMPEST_EXTERNAL_PLUGIN_GIT_URLS
    - URL of a git repository that holds the plugin

TEMPEST_EXTERNAL_PLUGIN_CHANGE_URLS
    - URL from which a change should be downloaded for a given plugin

TEMPEST_EXTERNAL_PLUGIN_REFSPEC
    - A change that should be downloaded for a given plugin
@lpiwowar lpiwowar force-pushed the feature/tempest-url-download branch from a8082d8 to d5c2bff Compare December 19, 2023 15:57
@kopecmartin kopecmartin requested a review from arxcruz December 20, 2023 12:19
Copy link
Contributor

@kopecmartin kopecmartin left a comment

Choose a reason for hiding this comment

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

lgtm, thanks Lukas

@openshift-ci openshift-ci bot added the lgtm label Dec 20, 2023
@son-vyas
Copy link
Contributor

any issue in testing with testproject ? like without testing this its hard to approve

@son-vyas son-vyas added the question Further information is requested label Dec 20, 2023
@kopecmartin
Copy link
Contributor

any issue in testing with testproject ? like without testing this its hard to approve

tested locally, no testproject, these changes are required in test-opeartor - openstack-k8s-operators/test-operator#12 - which isn't used anywhere yet as we're writing an ansible role to run it in CI, still in progress. In this review it's enough if we check only that the image works with current CI jobs. I guess some have been executed when this was proposed. Based on my code review, the changes in the code don't affect the CI jobs as well as don't affect the current way how we use this image.

@viroel
Copy link
Contributor

viroel commented Dec 20, 2023

I don't really remember where we replace the tempest image with the one built in content-provider. So yesterday I created a testproject to force tempest image to use the one built by the job. Job continues to pass.
https://review.rdoproject.org/r/c/testproject/+/48781/50#message-e9e7035f564f851a1df4e1cb7df9565a6f205b0a
controller | 38.102.83.241:5001/podified-antelope-centos9/openstack-tempest 8fcc848d6c766b48142f0ffef9e34937 89390c62ba30 About an hour ago 535 MB
Trying to run now again, without tempest_image var to see the output from podman images.

@viroel
Copy link
Contributor

viroel commented Dec 20, 2023

/approve

based on https://review.rdoproject.org/zuul/build/d2b3ab07e8f3416087de6da7f2cc9194 too. Since doesn't break edpm-multinode jobs.

Copy link
Contributor

openshift-ci bot commented Dec 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kopecmartin, lpiwowar, viroel

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-bot openshift-merge-bot bot merged commit fe9fcf0 into openstack-k8s-operators:main Dec 20, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants