libvirt: Add automatic mounting of bind mounts#93
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic mounting of bind mounts in libvirt using SMBIOS injection and systemd mount units. It adds new --bind and --bind-ro flags to the libvirt run command, updates the --bind-storage-ro functionality, and includes a new integration test for bind mounts. The code changes involve modifications to crates/integration-tests/src/tests/libvirt_verb.rs, crates/kit/src/libvirt/run.rs, crates/kit/src/sshcred.rs, and docs/src/man/bcvk-libvirt-run.md. I have provided review comments to address potential issues and improve the code.
| println!("Waiting for VM to boot and automatic mount to complete..."); | ||
| std::thread::sleep(std::time::Duration::from_secs(10)); |
There was a problem hiding this comment.
Consider adding a more descriptive message to the println! macro to provide more context about what the sleep is for. This can help with debugging and understanding the test's execution flow.
| // Generate SMBIOS credential for mount unit (without dropin) | ||
| let unit_name = crate::sshcred::guest_path_to_unit_name(&guest_path); | ||
| let mount_unit_content = crate::sshcred::generate_mount_unit(&tag, &guest_path, readonly); | ||
| let encoded_mount = data_encoding::BASE64.encode(mount_unit_content.as_bytes()); | ||
| let mount_cred = format!( | ||
| "io.systemd.credential.binary:systemd.extra-unit.{unit_name}={encoded_mount}" | ||
| ); | ||
| mount_unit_smbios_creds.push(mount_cred); | ||
| mount_unit_names.push(unit_name); | ||
| } |
There was a problem hiding this comment.
The code generates SMBIOS credentials for mount units. However, it does not handle potential errors during the credential generation process. If the credential generation fails, the VM creation might succeed, but the automatic mounting will not work. Consider adding error handling to ensure that the SMBIOS credentials are generated successfully.
| let mut tmpfiles_content = crate::sshcred::key_to_root_tmpfiles_d(&public_key_content); | ||
| tmpfiles_content.push_str(&crate::sshcred::storage_opts_tmpfiles_d_lines()); | ||
| let encoded = data_encoding::BASE64.encode(tmpfiles_content.as_bytes()); | ||
| let smbios_cred = format!("io.systemd.credential.binary:tmpfiles.extra={encoded}"); |
There was a problem hiding this comment.
The SMBIOS credential string is constructed directly using string formatting. If the encoded value contains special characters, it could lead to parsing issues. Consider using a safer method to construct the SMBIOS credential string, such as using a dedicated library for handling SMBIOS data.
| pub fn key_to_root_tmpfiles_d(pubkey: &str) -> String { | ||
| let buf = data_encoding::BASE64.encode(pubkey.as_bytes()); | ||
| format!("d /root/.ssh 0750 - - -\nf+~ /root/.ssh/authorized_keys 700 - - - {buf}") | ||
| format!("d /root/.ssh 0750 - - -\nf+~ /root/.ssh/authorized_keys 700 - - - {buf}\n") |
There was a problem hiding this comment.
The key_to_root_tmpfiles_d function creates a tmpfiles.d configuration to set up SSH access. However, it does not handle potential errors during the file creation process. If the file creation fails, the SSH access might not work as expected. Consider adding error handling to ensure that the tmpfiles.d configuration is created successfully.
/// Uses `f+~` to append to existing authorized_keys files.
41:pub fn key_to_root_tmpfiles_d(pubkey: &str) -> String {
42: let buf = data_encoding::BASE64.encode(pubkey.as_bytes());
43: format!("d /root/.ssh 0750 - - -\nf+~ /root/.ssh/authorized_keys 700 - - - {buf}\n")| concat!( | ||
| "f /etc/environment.d/90-bcvk-storage.conf 0644 root root - STORAGE_OPTS=additionalimagestore=/run/host-container-storage\n", | ||
| "d /etc/systemd/system.conf.d 0755 root root -\n", | ||
| "f /etc/systemd/system.conf.d/90-bcvk-storage.conf 0644 root root - [Manager]\\nDefaultEnvironment=STORAGE_OPTS=additionalimagestore=/run/host-container-storage\n" | ||
| ).to_string() |
There was a problem hiding this comment.
The storage_opts_tmpfiles_d_lines function generates tmpfiles.d lines for STORAGE_OPTS. However, it uses concat! to create the string, which can be less readable and maintainable than using format! or a similar approach. Consider using format! to improve the readability and maintainability of the code.
87: concat!(
88: "f /etc/environment.d/90-bcvk-storage.conf 0644 root root - STORAGE_OPTS=additionalimagestore=/run/host-container-storage\n",
89: "d /etc/systemd/system.conf.d 0755 root root -\n",
90: "f /etc/systemd/system.conf.d/90-bcvk-storage.conf 0644 root root - [Manager]\\nDefaultEnvironment=STORAGE_OPTS=additionalimagestore=/run/host-container-storage\n"
91: ).to_string()Rules References
| pub fn guest_path_to_unit_name(guest_path: &str) -> String { | ||
| let path = guest_path.strip_prefix('/').unwrap_or(guest_path); | ||
|
|
||
| // Escape dashes in path components, then replace slashes with dashes | ||
| let escaped = path | ||
| .split('/') | ||
| .map(|component| component.replace('-', "\\x2d")) | ||
| .collect::<Vec<_>>() | ||
| .join("-"); | ||
|
|
||
| format!("{}.mount", escaped) | ||
| } |
There was a problem hiding this comment.
The guest_path_to_unit_name function converts a guest mount path to a systemd unit name. However, it does not handle potential errors during the string manipulation process. If the string manipulation fails, the unit name might not be generated correctly. Consider adding error handling to ensure that the unit name is generated successfully.
| pub fn generate_mount_unit(virtiofs_tag: &str, guest_path: &str, readonly: bool) -> String { | ||
| let options = if readonly { "Options=ro" } else { "Options=rw" }; | ||
|
|
||
| format!( | ||
| "[Unit]\n\ | ||
| Description=Mount virtiofs tag {tag} at {path}\n\ | ||
| ConditionPathExists=!/etc/initrd-release\n\ | ||
| DefaultDependencies=no\n\ | ||
| Conflicts=umount.target\n\ | ||
| Before=local-fs.target umount.target\n\ | ||
| After=systemd-remount-fs.service\n\ | ||
| \n\ | ||
| [Mount]\n\ | ||
| What={tag}\n\ | ||
| Where={path}\n\ | ||
| Type=virtiofs\n\ | ||
| {options}\n", | ||
| tag = virtiofs_tag, | ||
| path = guest_path, | ||
| options = options | ||
| ) |
There was a problem hiding this comment.
The generate_mount_unit function generates a systemd mount unit for virtiofs. However, it uses string formatting to create the unit content, which can be less readable and maintainable than using a template engine or a similar approach. Consider using a template engine to improve the readability and maintainability of the code.
| pub fn smbios_creds_for_mount_unit( | ||
| virtiofs_tag: &str, | ||
| guest_path: &str, | ||
| readonly: bool, | ||
| ) -> Result<Vec<String>> { | ||
| let unit_name = guest_path_to_unit_name(guest_path); | ||
| let mount_unit_content = generate_mount_unit(virtiofs_tag, guest_path, readonly); | ||
| let encoded_mount = data_encoding::BASE64.encode(mount_unit_content.as_bytes()); | ||
|
|
||
| let mount_cred = | ||
| format!("io.systemd.credential.binary:systemd.extra-unit.{unit_name}={encoded_mount}"); | ||
|
|
||
| // Create a dropin for local-fs.target that wants this mount | ||
| let dropin_content = format!( | ||
| "[Unit]\n\ | ||
| Wants={unit_name}\n" | ||
| ); | ||
| let encoded_dropin = data_encoding::BASE64.encode(dropin_content.as_bytes()); | ||
| let dropin_cred = format!( | ||
| "io.systemd.credential.binary:systemd.unit-dropin.local-fs.target~bcvk-mounts={encoded_dropin}" | ||
| ); |
There was a problem hiding this comment.
The smbios_creds_for_mount_unit function generates SMBIOS credentials for a systemd mount unit. However, it duplicates code from the generate_mount_unit function. Consider refactoring the code to avoid code duplication and improve maintainability.
|
(OT)
Only tangentially related, but this is such a cool mechanism, I wish it were supported on baremetal machines by way of BMC & Redfish. For anyone interested in that, see (or comment on) https://redfishforum.com/thread/718/support-configuration-smbios-type-strings |
cgwalters
force-pushed
the
smbios
branch
2 times, most recently
from
e6e9f2c to
3bd9b25
Compare
Keep the raw functionality to set up mounts with --volume, but add new --bind and --bind-ro flags that use SMBIOS injection to do automatic mounting via systemd mount units. This also updates the --bind-storage-ro functionality to use automatic mounting at /run/host-container-storage. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
In a fresh CI environment, these tests all race to create a base disk which could cause slowdown. Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Colin Walters <walters@verbum.org>
|
|
||
| # Per-test overrides for libvirt tests that create VMs | ||
| # These tests race on creating libvirt base disks, so they need to run serially | ||
| # VMs get 4 vcpus by default, so require 4 threads to prevent any parallelism |
There was a problem hiding this comment.
OK this definitely improves our CI reliability...but it's still quite slow. I'll chase that separately.
| } | ||
| // Wait for VM to boot and automatic mount to complete | ||
| println!("Waiting for VM to boot and automatic mount to complete..."); | ||
| std::thread::sleep(std::time::Duration::from_secs(10)); |
There was a problem hiding this comment.
10 seconds is probably enough but this might become a source of flakiness. Maybe check every 10 seconds for a minute or so to mitigate? Not a blocker.
|
|
||
| // Wait for VM to boot and mounts to be ready | ||
| println!("Waiting for VM to boot and mounts to be ready..."); | ||
| std::thread::sleep(std::time::Duration::from_secs(15)); |
There was a problem hiding this comment.
similar to previous comment, the sleep might cause flakiness and would probably be better to loop for a minute or so.
3 participants
Keep the raw functionality to set up mounts with --volume, but add new --bind and --bind-ro flags that use SMBIOS injection to do automatic mounting via systemd mount units.
This also updates the --bind-storage-ro functionality to use automatic mounting at /run/host-container-storage.
Assisted-by: Claude Code (Sonnet 4.5)