Add bcvk project
#86
Add bcvk project
#86
Conversation
Summary of ChangesHello @cgwalters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a new project subcommand to bcvk, enabling Vagrant-like VM management scoped to a project directory. It includes subcommands for initializing, starting, SSHing into, stopping, and removing project VMs. The changes also add support for bind mounts, automatic updates, and lifecycle management. The code review focuses on correctness, maintainability, and potential security issues.
| pub fn compute_cache_hash(&self, install_options: &InstallOptions) -> String { | ||
| install_options.compute_hash(&self.digest) | ||
| } | ||
|
|
There was a problem hiding this comment.
This function delegates to InstallOptions::compute_hash(). It would be useful to add a comment here explaining why this delegation is necessary and what benefits it provides.
|
|
||
| /// Write metadata to a file using extended attributes via rustix | ||
| pub fn write_to_file(&self, file: &File) -> Result<()> { | ||
| pub fn write_to_file(&self, file: &File, install_options: &InstallOptions) -> Result<()> { |
There was a problem hiding this comment.
It's good that you're passing install_options here. It would be useful to add a comment explaining why install_options is needed here, and how it affects the cache hash.
| if opts.lifecycle_bind_parent { | ||
| spawn_lifecycle_monitor(&vm_name, connect_uri) | ||
| .with_context(|| "Failed to spawn lifecycle monitor")?; | ||
| println!("Lifecycle monitor started for domain '{}'", vm_name); | ||
| } |
There was a problem hiding this comment.
Consider adding a check to ensure that the lifecycle monitor is only spawned for persistent VMs (i.e., not transient VMs). Spawning it for transient VMs might not be useful, as they are meant to be short-lived.
crates/kit/src/libvirt/run.rs
Outdated
|
|
||
| // Validate that guest path is absolute | ||
| if !guest_path.starts_with('/') { | ||
| return Err(color_eyre::eyre::eyre!( | ||
| "Guest path '{}' must be an absolute path", | ||
| guest_path | ||
| )); |
There was a problem hiding this comment.
It would be useful to add a comment here explaining why the guest path must be an absolute path.
| // Add any additional metadata from caller | ||
| for (key, value) in &opts.metadata { | ||
| domain_builder = domain_builder.with_metadata(key, value); |
There was a problem hiding this comment.
Consider adding a comment explaining what the purpose of adding metadata from the caller is.
| for extra_cred in &opts.extra_smbios_credentials { | ||
| qemu_args.push("-smbios".to_string()); | ||
| qemu_args.push(format!("type=11,value={}", extra_cred)); |
There was a problem hiding this comment.
Consider adding a comment explaining what the purpose of adding extra SMBIOS credentials from opts is.
crates/kit/src/libvirt/run.rs
Outdated
| qemu_args.extend(vec![ | ||
| "-netdev".to_string(), | ||
| format!("user,id=ssh0,hostfwd=tcp::{}-:22", ssh_port), | ||
| "-device".to_string(), | ||
| "virtio-net-pci,netdev=ssh0,addr=0x3".to_string(), |
There was a problem hiding this comment.
Consider adding a comment explaining what the purpose of adding networking args is.
| metadata | ||
| .write_to_file(&file) | ||
| .write_to_file(&file, install_options) | ||
| .with_context(|| "Failed to write metadata to disk file")?; |
There was a problem hiding this comment.
It's good that you're passing install_options here. It would be useful to add a comment explaining why install_options is needed here, and how it affects the cache hash.
| pub fn smbios_creds_for_storage_opts() -> Result<Vec<String>> { | ||
| // Create systemd unit that conditionally appends to /etc/environment | ||
| let unit_content = r#"[Unit] | ||
| Description=Setup STORAGE_OPTS for bcvk | ||
| DefaultDependencies=no | ||
| Before=systemd-user-sessions.service | ||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/bin/sh -c 'grep -q STORAGE_OPTS /etc/environment || echo STORAGE_OPTS=additionalimagestore=/run/host-container-storage >> /etc/environment' | ||
| RemainAfterExit=yes | ||
| "#; | ||
| let encoded_unit = data_encoding::BASE64.encode(unit_content.as_bytes()); | ||
| let unit_cred = format!( | ||
| "io.systemd.credential.binary:systemd.extra-unit.bcvk-storage-opts.service={encoded_unit}" | ||
| ); | ||
|
|
||
| // Create dropin for sysinit.target to pull in our unit | ||
| let dropin_content = "[Unit]\nWants=bcvk-storage-opts.service\n"; | ||
| let encoded_dropin = data_encoding::BASE64.encode(dropin_content.as_bytes()); | ||
| let dropin_cred = format!( | ||
| "io.systemd.credential.binary:systemd.unit-dropin.sysinit.target~bcvk-storage={encoded_dropin}" | ||
| ); | ||
|
|
||
| Ok(vec![unit_cred, dropin_cred]) |
There was a problem hiding this comment.
This function creates a systemd unit and a dropin. It would be useful to add a comment explaining what the purpose of each of these is.
| 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.
This function generates tmpfiles.d lines for STORAGE_OPTS. It would be useful to add a comment explaining what the purpose of these lines is.
cgwalters
force-pushed
the
bind-units
branch
2 times, most recently
from
b7ceaf5 to
d5d6c6f
Compare
|
I have used this while developing a backup solution when migrating a database. Well i tried but i haven't written a single line of code. Because i was constantly debugging this project for this pr. Some of the things i tried to debug are changed in this pr. But the one that cost a lot of time is also on main. The issue is when i tried to run I can't find why it is stuck there, but if i accept the ai suggestion to don't propagate the log level environment in Full podman logThe reason i wanted the debug flag is to get more context for the following error. This was thrown when the config file only contains the name of the image. And if i add the filesystem used. The error goes away. There was a default right.... Well i think forcing the user to set this setting can be good. So adding the filesystem to project init is appreciated. I haven't started looking at how --auto-update works. Which i was hoping to test out. But it is now 02:01 AM and i really need to sleep. |
This matches bootc's setup (without the journal integration for now as we don't strictly need it). But specifically this fixes setting `RUST_LOG` and having that break the monitor-status JSON stream: bootc-dev#86 (comment) Signed-off-by: Colin Walters <[email protected]>
This matches bootc's setup (without the journal integration for now as we don't strictly need it). But specifically this fixes setting `RUST_LOG` and having that break the monitor-status JSON stream: bootc-dev#86 (comment) Signed-off-by: Colin Walters <[email protected]>
This matches bootc's setup (without the journal integration for now as we don't strictly need it). But specifically this fixes setting `RUST_LOG` and having that break the monitor-status JSON stream: bootc-dev#86 (comment) Signed-off-by: Colin Walters <[email protected]>
Ah yeah, I reproduced that. Nasty bug, I think that caused me confusion in the past. I ran out of time in debugging it. I put up #98 which mitigates it. |
This matches bootc's setup (without the journal integration for now as we don't strictly need it). But specifically this fixes setting `RUST_LOG` and having that break the monitor-status JSON stream: #86 (comment) Signed-off-by: Colin Walters <[email protected]>
|
This one is passing CI now, but there's like a lot of code here and it needs some more sanity checking. Leaving as draft for now. |
|
I have looked some time of what should be changed. And in this comment contains a patch, and below that there is a bullet list, containing what i have found. For the next time. Should i create a pr to your personal fork to suggest a code change? Or is a patch in the comments fine. you can add the patch with From e68fe416f500f92debfb87e690ab8a010d21ef98 Mon Sep 17 00:00:00 2001
From: Aron <[email protected]>
Date: Wed, 29 Oct 2025 17:33:47 +0100
Subject: [PATCH] project | libvirt: Minor config changes
I have changed some config changes like setting a default filesystem to
make it more aligned to what the code says. And what my expectations
are.
libvirt:
- add default filesystem var. (ext4 because it is allready used)
- libvirt run: use default filesystem (Add by ai. Not manualy checked. Maybe i shouldn't use it.)
project up:
- use default fs
- use relative path
project down:
- add --force flag that is passed down to libvirt functions
- inlined functions
Assisted-by: Cursor's automatic modelswicher
---
crates/kit/src/libvirt/mod.rs | 3 +++
crates/kit/src/libvirt/run.rs | 2 +-
crates/kit/src/project/down.rs | 44 +++++++++++++++-------------------
crates/kit/src/project/up.rs | 10 ++++----
4 files changed, 29 insertions(+), 30 deletions(-)
diff --git a/crates/kit/src/libvirt/mod.rs b/crates/kit/src/libvirt/mod.rs
index 34156e3..eabdb2c 100644
--- a/crates/kit/src/libvirt/mod.rs
+++ b/crates/kit/src/libvirt/mod.rs
@@ -23,6 +23,9 @@ pub const LIBVIRT_DEFAULT_MEMORY: &str = "4G";
/// Default disk size for libvirt base disks
pub const LIBVIRT_DEFAULT_DISK_SIZE: &str = "20G";
+/// Default filesystem for libvirt VMs
+pub const LIBVIRT_DEFAULT_FILESYSTEM: &str = "ext4";
+
pub mod base_disks;
pub mod base_disks_cli;
pub mod domain;
diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs
index 4a7c550..51fbffe 100644
--- a/crates/kit/src/libvirt/run.rs
+++ b/crates/kit/src/libvirt/run.rs
@@ -1003,7 +1003,7 @@ fn create_libvirt_domain_from_disk(
opts.install
.filesystem
.as_ref()
- .unwrap_or(&"ext4".to_string()),
+ .unwrap_or(&crate::libvirt::LIBVIRT_DEFAULT_FILESYSTEM.to_string()),
)
.with_metadata("bootc:network", &opts.network)
.with_metadata("bootc:ssh-generated", "true")
diff --git a/crates/kit/src/project/down.rs b/crates/kit/src/project/down.rs
index db10158..e799a00 100644
--- a/crates/kit/src/project/down.rs
+++ b/crates/kit/src/project/down.rs
@@ -19,6 +19,9 @@ pub struct ProjectDownOpts {
/// Remove the VM after shutting it down
#[clap(long)]
pub remove: bool,
+
+ #[clap(long)]
+ pub force: bool,
}
/// Run the project down command
@@ -38,18 +41,31 @@ pub fn run(opts: ProjectDownOpts) -> Result<()> {
};
if !check_vm_exists(&vm_name, &libvirt_opts)? {
- println!("Project VM '{}' does not exist", vm_name);
+ println!("Project is already down. vm_name: '{}'", vm_name);
return Ok(());
}
// Stop the VM
println!("Shutting down project VM '{}'...", vm_name);
- stop_vm(&vm_name, &libvirt_opts)?;
+
+ let stop_opts = libvirt::stop::LibvirtStopOpts {
+ name: vm_name.to_string(),
+ force: opts.force,
+ timeout: 60,
+ };
+
+ let _ = libvirt::stop::run(&libvirt_opts, stop_opts);
// Remove if requested
if opts.remove {
println!("Removing project VM '{}'...", vm_name);
- remove_vm(&vm_name, &libvirt_opts)?;
+ let rm_opts = libvirt::rm::LibvirtRmOpts {
+ name: vm_name.to_string(),
+ force: opts.force,
+ stop: false,
+ };
+
+ libvirt::rm::run(&libvirt_opts, rm_opts)?
}
Ok(())
@@ -68,25 +84,3 @@ fn check_vm_exists(name: &str, libvirt_opts: &LibvirtOptions) -> Result<bool> {
Ok(domains.iter().any(|d| d.name == name))
}
-
-/// Stop a VM
-fn stop_vm(name: &str, libvirt_opts: &LibvirtOptions) -> Result<()> {
- let stop_opts = libvirt::stop::LibvirtStopOpts {
- name: name.to_string(),
- force: false,
- timeout: 60,
- };
-
- libvirt::stop::run(libvirt_opts, stop_opts)
-}
-
-/// Remove a VM
-fn remove_vm(name: &str, libvirt_opts: &LibvirtOptions) -> Result<()> {
- let rm_opts = libvirt::rm::LibvirtRmOpts {
- name: name.to_string(),
- force: false,
- stop: false,
- };
-
- libvirt::rm::run(libvirt_opts, rm_opts)
-}
diff --git a/crates/kit/src/project/up.rs b/crates/kit/src/project/up.rs
index bfa403e..f1393be 100644
--- a/crates/kit/src/project/up.rs
+++ b/crates/kit/src/project/up.rs
@@ -219,9 +219,11 @@ fn create_vm(
extra_smbios_credentials: vec![],
};
- if let Some(ref fs) = vm.filesystem {
- run_opts.install.filesystem = Some(fs.clone());
- }
+ run_opts.install.filesystem = Some(
+ vm.filesystem
+ .clone()
+ .unwrap_or_else(|| crate::libvirt::LIBVIRT_DEFAULT_FILESYSTEM.to_string()),
+ );
// Bind project directory to /run/src read-only with auto-mount
// (will fall back to read-write if libvirt doesn't support readonly virtiofs)
@@ -233,7 +235,7 @@ fn create_vm(
// Add configured mounts using bind mount options
for mount in config.mounts.iter().flatten() {
- let mount_spec = format!("{}:{}", mount.host, mount.guest);
+ let mount_spec = format!("{}/{}:{}", project_dir.as_str(), mount.host, mount.guest);
let bind_mount = mount_spec
.parse()
.with_context(|| format!("Failed to parse mount spec: {}", mount_spec))?;
--
2.51.0
systemd is taking the OnCalendar Running
|
Yeah that's an obvious one, as is the VM sizes. So recently I came across https://devfile.io/ ...we could try to instead parse a subset of that. It already supports both CPU/memory and port forwarding and the basic one of a container image name. |
For my interpretation. Do you mean: Should we use the same configuration file as what devfile.io uses. So that We can have the advantage of maintainers who have something in the registry who maybe also gives support to make a bootable container. But then we have the disadvantage that we have another project that we need to keep track and change the configuration file to track what they are doing. |
The core idea here is to create an experience where the virtual machine is closely bound to a specific directory (a "project") - always always a git repository. Closes: bootc-dev#31 Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>
The project command was unconditionally creating readonly bind mounts, which caused failures on libvirt versions < 11.0 that don't support readonly virtiofs. This was breaking the project_upgrade_workflow test in CI (libvirt 10.0). Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>
I have changed some config changes like setting a default filesystem to make it more aligned to what the code says. And what my expectations are. libvirt: - add default filesystem var. (ext4 because it is allready used) - libvirt run: use default filesystem (Add by ai. Not manualy checked. Maybe i shouldn't use it.) project up: - use default fs - use relative path project down: - add --force flag that is passed down to libvirt functions - inlined functions Assisted-by: Cursor's automatic modelswicher Signed-off-by: Colin Walters <[email protected]>
(edit: I also rebased 🏄 ) |
|
It maybe also interesting to parse the config of bootc-image-builder for other parameters like which filesystem to use. Also creating users maybe also interesting |
2 participants
The core idea here is to create an experience where the virtual machine is closely bound to a specific directory (a "project") - always always a git repository.
Closes: #31
Assisted-by: Claude Code (Sonnet 4.5)