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

SEV CI PR test for SNP host and guest on the self-hosted runner #233

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LakshmiSaiHarika
Copy link

Created new sev library manual pr test GH Action script on self-hosted runner

@tylerfanelli
Copy link
Member

@larrydewey @DGonzalezVillal can you review?

Copy link
Member

@DGonzalezVillal DGonzalezVillal left a comment

Choose a reason for hiding this comment

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

Some general things to fix.

I think the most important thing is that this is not the desired workflow. We want this to be run automatically and not in steps but everything at once (still to discuss about how often we want to update kernel and things like that).

We talked about maybe setting up a webhook to trigger this workflow automatically. Maybe look into that!

Copy link
Member

Choose a reason for hiding this comment

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

Could you rename the file to something like:
sev-ci-pr-testing.yaml

Comment on lines 32 to 37
# Commented these, as these are already installed on self-hosted runner
# - name: Install Dependencies
# run: |
# sudo dnf update -y
# sudo dnf clean packages -y
# sudo dnf install -y wget git curl
Copy link
Member

Choose a reason for hiding this comment

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

if you don't need this just delete it instead...

@@ -0,0 +1,138 @@
name: Manual sev PR test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: Manual sev PR test
name: SEV CI PR test


- name: Execute Command
run: |
case "${{ github.event.inputs.command }}" in
Copy link
Member

Choose a reason for hiding this comment

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

so from my understanding you have to trigger this action and give it the command that you want it to execute?

Copy link
Author

Choose a reason for hiding this comment

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

Done, added PR request webhook into my script to automate this action whenever new PR gets raised

Comment on lines 42 to 53
install-snp-on-the-host)
echo "Installing SNP on the host..."
wget https://raw.githubusercontent.com/LakshmiSaiHarika/sev-utils/Fedora-Latest-SNP-kernel-Upstream/tools/snp.sh
chmod +x snp.sh
./snp.sh setup-host
echo "The host must be rebooted for changes to take effect."
;;

reboot-host)
echo "Rebooting the host..."
sudo reboot
;;
Copy link
Member

Choose a reason for hiding this comment

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

We agreed that for PRs we want to use upstream kernel. Do we want to build the kernel every time a PR is submitted? Do we manually update the host kernel every few months? Do we check for a new kernel every time a PR is submitted? I don't know what you guys want to do for this workflow. @larrydewey @tylerfanelli

Copy link
Author

Choose a reason for hiding this comment

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

I guess an alternative to manual SNP kernel update is to update host kernel on a schedule basis. If we are interested in this alternative approach, how often we perform this update ? like every day, every month, etc

Comment on lines 55 to 62
verify-snp-on-host)
echo "Verifying SNP on the host..."
if ! sudo dmesg | grep -i "SEV-SNP enabled" 2>&1 >/dev/null; then
echo "SEV-SNP not enabled on the host."
exit 1
fi
echo "SEV-SNP is enabled on the host."
;;
Copy link
Member

Choose a reason for hiding this comment

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

Could we do here the same verify snp as we have on the snp.sh script?

Copy link
Author

Choose a reason for hiding this comment

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

These lines are same as verify snp in snp.sh script. I added extra lines as the separate command for SNP verification via snp.sh is not present in sev-utils upstream.

source "${HOME}/.cargo/env" 2>/dev/null
fi

cargo test -- --skip snp
Copy link
Member

Choose a reason for hiding this comment

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

we added a fix to the snp launch, so we actually don't want to skip it anymore

Suggested change
cargo test -- --skip snp
cargo test

Copy link
Member

Choose a reason for hiding this comment

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

We might also want to pass some parameters in so that certain tests that have dependencies do not get skipped.

Comment on lines 88 to 89
wget https://raw.githubusercontent.com/LakshmiSaiHarika/sev-utils/Fedora-Latest-SNP-kernel-Upstream/tools/snp.sh
chmod +x snp.sh
Copy link
Member

Choose a reason for hiding this comment

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

Since I would like it to be a single workflow, we wouldn't want to do this more than once.

Copy link
Author

Choose a reason for hiding this comment

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

Done, performed cleanup, please review the updated and let me know more about further changes

fi
}

verify_snp_guest
Copy link
Member

Choose a reason for hiding this comment

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

Use the MSR check you developed

Copy link
Author

Choose a reason for hiding this comment

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

Added bash commands to verify SNP via MSR

Comment on lines 128 to 129
git fetch origin pull/${{ github.event.inputs.pr_number }}/head:${{ github.event.inputs.pr_branch }}
git switch ${{ github.event.inputs.pr_branch }}
Copy link
Member

Choose a reason for hiding this comment

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

Here you're passing the pr_number and pr_branch into the guest. What was the issue you were mentioning that was stopping you from doing this?

Copy link
Author

Choose a reason for hiding this comment

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

Updated script to add PR webhooks

@LakshmiSaiHarika LakshmiSaiHarika changed the title Manual PR test for sev library for host and guest on self-hosted runner SEV CI PR test for SNP host and guest on the self-hosted runner Oct 21, 2024
@tylerfanelli
Copy link
Member

tylerfanelli commented Nov 8, 2024

Is this ready for review? There's some unresolved conversations so just making sure everything has been addressed.

@LakshmiSaiHarika
Copy link
Author

Hi Tyler, I am still editing this PR, I addressed few of the above comments and I am still adding fixes for the multiple SNP guest launch issues for SEV CI PR test for all the open PRs in parallel.

I will let all the reviewers know once these changes are finalized

@tylerfanelli
Copy link
Member

Great, thanks for letting me know.

@AdithyaKrishnan
Copy link

@LakshmiSaiHarika Please convert this PR to draft if you're still working and revert when it's ready for review

LakshmiSaiHarika and others added 5 commits November 21, 2024 20:09
Added this tool to trigger Virtee CI PR test GH workflows(sev, snphost, snpguest) for a manual event(event after SNP host kernel updateon the self-hosted runner) using Github Action API and the Github PAT.

Signed-off-by: Harika Nittala <[email protected]>
…t SNP upstream host kernel

Added new workflow for manual setup of the self-hosted runner with the given latest SNP kernel tag version(ex: v6.10)
This workflow installs and updates self-hosted runner with the given SNP tag version from upstream kernel.

Signed-off-by: Harika Nittala <[email protected]>
…ted runner.

Added a new helper script having function definition for SNP related tasks(verify if SNP is enabled for the SNP host kernel, run a command on an active guest) and host dependencies(check rust dependency) on the self-hosted runner.

Signed-off-by: Harika Nittala <[email protected]>
…ithout network port conflict

Added a new tool to assign network ports to multiple SNP guest without network port conflict and cleanup inactive guest network port in the inventory file tracked by GH Action workflow

Signed-off-by: Harika Nittala <[email protected]>
… runner

This workflow performs SEV cargo tests on the SNP host followed by cargo unit tests on  SNP guest(without flags).

Signed-off-by: Harika Nittala <[email protected]>
- name: Launch SNP enabled guest
run: |
set -e
wget https://raw.githubusercontent.com/LakshmiSaiHarika/sev-utils/Fedora-Latest-SNP-kernel-Upstream/tools/snp.sh
Copy link
Author

@LakshmiSaiHarika LakshmiSaiHarika Jan 14, 2025

Choose a reason for hiding this comment

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

Change into sev-utils upstream URL

upstream link

export GUEST_NAME=${{ env.guest_name }}
export HOST_SSH_PORT=${{ env.guest_port_in_use }}

./snp.sh launch-guest
Copy link
Author

Choose a reason for hiding this comment

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

Can use
./snp.sh launch-guest --guest-name "${{ env.guest_name }}" --guest-port "${{ env.guest_port_in_use }}"

export GUEST_NAME=${{ env.guest_name }}
export HOST_SSH_PORT=${{ env.guest_port_in_use }}

./snp.sh stop-guests
Copy link
Author

Choose a reason for hiding this comment

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

Can use:

./snp.sh stop-guests --guest-name "${{ env.guest_name }}"

id: install-snp-host-kernel
run: |
# Downloads sev utility script to setup host SNP kernel
wget https://raw.githubusercontent.com/LakshmiSaiHarika/sev-utils/Fedora-Latest-SNP-kernel-Upstream/tools/snp.sh
Copy link
Author

Choose a reason for hiding this comment

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

Yet to update to upstream link

upstream link here

# Sets up the user given latest upstream SNP host kernel/master branch
kernel_host_guest_branch_tag="${{ github.event.inputs.snp-kernel-host-guest-tag }}"
if [[ ! -z "${kernel_host_guest_branch_tag}" ]]; then
./snp.sh --kernel-tag "${kernel_host_guest_branch_tag}" setup-host
Copy link
Author

Choose a reason for hiding this comment

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

./snp.sh setup-host --kernel-tag "${kernel_host_guest_branch_tag}

./snp.sh --kernel-tag "${kernel_host_guest_branch_tag}" setup-host
else
# Uses kernel upstream default master branch for kernel installation
./snp.sh setup-host
Copy link
Author

Choose a reason for hiding this comment

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

./snp.sh setup-host --upstr-kernel

export GUEST_NAME="sample-snp-guest"
export HOST_SSH_PORT=${{ env.guest_port_in_use }}

./snp.sh launch-guest
Copy link
Author

Choose a reason for hiding this comment

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

./snp.sh launch-guest --guest-name "sample-snp-guest" --guest-port "${{ env.guest_port_in_use }}"

export GUEST_NAME="sample-snp-guest"
export HOST_SSH_PORT=${{ env.guest_port_in_use }}

./snp.sh stop-guests
Copy link
Author

Choose a reason for hiding this comment

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

./snp.sh stop-guests --guest-name "sample-snp-guest"

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.

5 participants