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

preparations for e2e tests on baremetal SNP #730

Merged
merged 15 commits into from
Aug 14, 2024
Merged

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Jul 16, 2024

This PR implements all the necessary changes to run CI on our baremetal machine in the office.

@Freax13 Freax13 added the no changelog PRs not listed in the release notes label Jul 16, 2024
@Freax13 Freax13 force-pushed the tom/snp-baremetal-ci branch 27 times, most recently from f98a8f9 to f740fbd Compare July 23, 2024 05:41
@Freax13 Freax13 changed the title CI: run e2e test on baremetal SNP preparations for e2e test son baremetal SNP Jul 23, 2024
@Freax13 Freax13 marked this pull request as ready for review July 23, 2024 05:48
@Freax13 Freax13 force-pushed the tom/snp-baremetal-ci branch 4 times, most recently from 2e4cce5 to a4546ef Compare August 13, 2024 14:27
@Freax13 Freax13 marked this pull request as ready for review August 13, 2024 14:48
@Freax13 Freax13 requested a review from katexochen August 14, 2024 06:00
@Freax13 Freax13 force-pushed the tom/snp-baremetal-ci branch from a4546ef to afa1b22 Compare August 14, 2024 08:29
Freax13 added 11 commits August 14, 2024 10:33
Unfortunately the launch measurement is vCPU type dependent (it doesn't
have to be that way, but QEMU sets up different vCPU types
differently, so thanks, QEMU?!). Assume Genoa for now.
This will allow us to test on platforms other than aks-clh-snp.
The generic default manifest (not the default for AKS) isn't valid
because it's missing TCB values. We want to emit invalid manifests and
it's up to the user to fill in the missing values. Instead of failing,
we now tell the user to fix the reference values for the selected
platform.
We use this kernel module with `dm-mod.create="dm-verit...` to protect
the image file.
The calculation of the launch measurement has been adjusted
accordingly.
The previous rc0 had a bug somewhere that influenced the launch measurement.
This bug has been fixed in rc1.
@Freax13 Freax13 force-pushed the tom/snp-baremetal-ci branch from afa1b22 to 00dc2c6 Compare August 14, 2024 08:33
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just some minor things.

jobs:
test:
runs-on:
labels: snp
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a GHA shortcoming, but it'd be very nice if we could make this test work on both SNP and TDX, without another duplication. But afaict, you cannot have dynamic values (e.g. an input) in runs-on. Not saying this PR should or can do anything about that, but just keeping it here as a note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand this section in the docs correctly, this might work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would enable us to create tests that run on all platforms unconditionally, but still not one test that runs on one, selectable platform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can also run steps conditionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but this won't be a step. We could execute everything but the actual step on GH-hosted runners and then transfer files over, but I fear that this is going to have a higher total cost in the end.

cli/cmd/generate.go Show resolved Hide resolved
e2e/internal/kubeclient/deploy.go Show resolved Hide resolved
e2e/openssl/openssl_test.go Outdated Show resolved Hide resolved
nodeinstaller/internal/constants/constants.go Show resolved Hide resolved
packages/by-name/kata/snp-launch-digest/package.nix Outdated Show resolved Hide resolved
When the node-installer restarts K3s, the watch call fails. Watch has a retry
loop internally, but it only retries starting the request, once it has
established a request and that request dies spuriously, watch, doesn't
reconnect.
@Freax13 Freax13 force-pushed the tom/snp-baremetal-ci branch from 00dc2c6 to 825b761 Compare August 14, 2024 11:35
@Freax13 Freax13 requested a review from msanft August 14, 2024 11:35
@Freax13 Freax13 merged commit f6e54af into main Aug 14, 2024
12 checks passed
@Freax13 Freax13 deleted the tom/snp-baremetal-ci branch August 14, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants