test: Support SEV baremetal tests via system tests #8386
Conversation
When configured with the `TRUSTED_EXECUTION_ENVIRONMENT` env variable, the test runs on our bare-metal instance. This only works if the credentials are available via an env variable therefore these tests can only be run manually (or later via a github action).
| system_test_nns( | ||
| name = "guestos_upgrade_from_latest_release_to_current_sev", | ||
| env = MAINNET_ENV | { | ||
| "NODE_OPERATOR_PRIV_KEY_PATH": "$(rootpath //ic-os/setupos:config/node_operator_private_key.pem)", |
There was a problem hiding this comment.
you can use runtime_deps below and drop the rootpath part!
| uses_hostos_update: the test uses the branch HostOS update image | ||
| uses_hostos_test_update: the test uses the branch HostOS update-test image | ||
| uses_hostos_mainnet_latest_update: the test uses the latest release mainnet HostOS update image | ||
| uses_bare_metal: the test runs on a bare metal instance |
There was a problem hiding this comment.
to clarify, does this mean that an extra test will be created to run on bare metal? I'm surprised not to see an option being passed to the test runner from here but maybe I'm missing context
| # Tests if "latest_release" in mainnet-icos-revisions.json can successfully guestos upgrade to current commit | ||
| # on SEV-enabled nodes | ||
| # This test requires manually injecting the baremetal configuration | ||
| # ict test //rs/tests/nested:guestos_upgrade_from_latest_release_to_current_sev -- --test_env=BARE_METAL_HOST_SECRETS="$(cat zh2-dll01.ini)" | ||
| # zh2-dll01.ini can be retrieved from our 1password |
There was a problem hiding this comment.
Very cool! Though it's odd that this test is in the nested folder because there is no nested virtualization. Should we just add a note:
NOTE: This is a bare metal test (not nested virtualization), but lives in this directory to share the deployment test infrastructure with nested tests.
That, or just rename this folder "ic_os" or "deployment"
There was a problem hiding this comment.
There is not nested virtuazation but these are still nested tests (we test HostOS + GuestOS together like the other nested tests). Perhaps we could use a different word but otherwise the only new thing is that the test doesn't run inside a VM. We can run any of the existing nested tests on baremetal, so I didn't want to move/change them.
There was a problem hiding this comment.
Okay sounds good. I always thought of nested as specifically nested virtualization, but that makes sense. Whatever you think best regarding naming. Feel free to close! 👍
| /// Minimal setup that sets up a bare metal instance without any IC infrastructure. | ||
| /// This is much faster than the full setup() setup. | ||
| pub fn simple_bare_metal_with_trusted_execution_environment_setup(env: TestEnv) { |
There was a problem hiding this comment.
I don't think this needs to be public. And if so, then the comment
This is much faster than the full setup() setup.
Is misleading, because the function is called by setup()
| let mut nodes = NestedNodes::single_bare_metal( | ||
| HOST_VM_NAME, | ||
| bare_metal.hostos_address(), | ||
| bare_metal.mgmt_mac(), | ||
| /*enable_trusted_execution_environment*/ true, | ||
| ); |
There was a problem hiding this comment.
Similar to my other comment, it's confusing that we have single_bare_metal in the NestedNodes struct. This can happen in a follow-up (which I'm happy to take 🙂), but I think we should rename NestedNodes or create a new type for the bare metal node.
| if node.get_vm()?.bare_metal { | ||
| setup_baremetal_instance(&t_env, &node, &config_image) | ||
| .context("Setting up baremetal instance failed") |
There was a problem hiding this comment.
Last time I'll comment on this, it's weird that setup_and_start_nested_vms, we are setting up a bare metal node
But this can be cleaned up in a follow-up
There was a problem hiding this comment.
What about we rename "nested" to "hostos"? "Nesting" is really an implementation detail, the important thing is this is testing the SetupOS & HostOS rather than just the GuestOS.
This renaming should then be done throughout including the rs/tests/nested/ directory,NestedNode, NestedNodes, NestedNodeSpec, etc.
There was a problem hiding this comment.
NestedNode does not have to mean Nested VM, it means it's a node with nested setup (hostos > guestos), instead of just guestos. But nested_vms are indeed confusing, maybe setup_and_start_nested_nodes?
There was a problem hiding this comment.
Aha ok yeah that interpretation makes sense.
| use ic_system_test_driver::driver::test_env::{SshKeyGen, TestEnvAttribute}; | ||
|
|
||
| pub const HOST_VM_NAME: &str = "host-1"; | ||
| const BARE_METAL_HOST_SECRETS: &str = "BARE_METAL_HOST_SECRETS"; |
There was a problem hiding this comment.
I like this name better than ZH2_DLL01_INI_SECRETS. What about we rename the latter into the former for consistency and easy grepping? Fine to do that in a follow up.
| "long_test", | ||
| "manual", |
There was a problem hiding this comment.
Is it the plan to run this test on CI later? Like as part of the bazel-test-bare-metal job
or as part of the release-system-tests by tagging the test with system_test_large?
If so, we need to remember to bring the secret into scope.
| uses_hostos_update: the test uses the branch HostOS update image | ||
| uses_hostos_test_update: the test uses the branch HostOS update-test image | ||
| uses_hostos_mainnet_latest_update: the test uses the latest release mainnet HostOS update image | ||
| uses_bare_metal: the test runs on a bare metal instance |
There was a problem hiding this comment.
I wonder if uses_hostos_img is not a better name here.
There was a problem hiding this comment.
We should also consider if we can consolidate all the uses_ parameters above into:
setupos: False | True (HEAD) | "mainnet_latest"hostos_update: False | True (HEAD) | "mainnet_latest" | "test"
Like what @nmattia did for the guestos and guestos_update parameters.
(I'm not entirely sure how to fit the bare-metal stuff into those).
4 participants
When configured with the
TRUSTED_EXECUTION_ENVIRONMENTenv variable, the test runs on our bare-metal instance. This only works if the credentials are available via an env variable therefore these tests can only be run manually (or later via a github action).