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

tests/e2e: Jenkins to GHA migration for SEV and SNP #352

Closed

Conversation

AdithyaKrishnan
Copy link
Contributor

The ccruntime-pr.yaml file has been modified and works only for amd specific tests currently. Work adds on to operator pull 295

.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
@AdithyaKrishnan AdithyaKrishnan changed the title WIP - Jenkins to GHA migration for SEV and SNP Jenkins to GHA migration for SEV and SNP Mar 1, 2024
@AdithyaKrishnan
Copy link
Contributor Author

@portersrc @fitzthum DCO passing finally.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

A few comments

Have you tried adding the ok-to-test?

timeout-minutes: 45
run: |
cd tests/e2e
export PATH="$PATH:/usr/local/bin"
Copy link
Member

Choose a reason for hiding this comment

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

I think you can specify this as part of the env below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @fitzthum - The ccruntime_e2e_amd.yaml file is a replica of the ccruntime_e2e.yaml file created by @BbolroC
For consistency, I've kept many of the lines same as Choi's and changed only the ones that require the AMD Self Hosted runner.

- name: Run e2e tests
timeout-minutes: 45
run: |
cd tests/e2e
Copy link
Member

Choose a reason for hiding this comment

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

You can use the working-directory field to specify where this step runs rather than doing a cd

steps:
- name: Take a pre-action for self-hosted runner
run: |
if [ -f ${HOME}/script/pre_action.sh ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This is your own script that lives on the CI machine? What will happen if it isn't there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pre-action script is not mine and I believe it's Choi's per my understanding. It would be used by both ccruntime_e2e_amd.yaml as well as the ccruntime_e2e.yaml.

- name: Install deps
run: |
sudo apt-get update -y
sudo apt-get install -y ansible python-is-python3
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to install these packages every time? I don't see them being uninstalled.

workflow_call:

permissions:
contents: read
Copy link
Member

Choose a reason for hiding this comment

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

What do we need this for?

.github/workflows/ccruntime_e2e_amd.yaml Outdated Show resolved Hide resolved
@AdithyaKrishnan AdithyaKrishnan changed the title Jenkins to GHA migration for SEV and SNP tests/e2e: Jenkins to GHA migration for SEV and SNP Mar 8, 2024
@fitzthum
Copy link
Member

fitzthum commented Mar 8, 2024

Ok, higher level question here. Maybe @wainersm will have some good advice.

Do you think we should have a separate workflow for SEV? I'm not sure if this is going to scale very well as we introduce more tests. Can we instead modify the existing e2e workflow to add SEV/SNP support?

@ldoktor
Copy link
Contributor

ldoktor commented Mar 11, 2024

Ok, higher level question here. Maybe @wainersm will have some good advice.

Do you think we should have a separate workflow for SEV? I'm not sure if this is going to scale very well as we introduce more tests. Can we instead modify the existing e2e workflow to add SEV/SNP support?

Well the workflow should be similar, shouldn't it? Only the set of tests and perhaps some setup parts will differ and that should be part of the scripts but IMO the workflows should be shared as much as possible.

@wainersm
Copy link
Member

Ok, higher level question here. Maybe @wainersm will have some good advice.
Do you think we should have a separate workflow for SEV? I'm not sure if this is going to scale very well as we introduce more tests. Can we instead modify the existing e2e workflow to add SEV/SNP support?

Well the workflow should be similar, shouldn't it? Only the set of tests and perhaps some setup parts will differ and that should be part of the scripts but IMO the workflows should be shared as much as possible.

Yes, that's how I see it as well.

@stevenhorsman
Copy link
Member

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.

6 participants