From 040fcaed92f7629f5a0d9ae57b9cb175a52e5e70 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Tue, 17 Mar 2026 11:11:54 +0100 Subject: [PATCH] vmm: add VmSendMigrationData::parse(); integrate with OptionParser This change prepares upcoming options (following commit) that are added to VmSendMigrationData. VmSendMigrationData is a special case as it is currently the only "rich configuration" type that lives outside `config.rs`, as it is purely API-facing. Therefore, it isn't integrated into the existing OptionParser infrastructure. We therefore introduce a `parse()` method to use that in `ch-remote` in the following. In `ch-remote`, we remove `--local` for `send-migration` and switch to the new option string parsing constructor (breaking change!). This prepares the addition of downtime and timeout options in the following and streamlines the `ch-remote` command line interface with other commands, such as `ch-remote add-net`. Lastly, this commit updates the integration tests. Signed-off-by: Philipp Schuster On-behalf-of: SAP philipp.schuster@sap.com --- cloud-hypervisor/src/bin/ch-remote.rs | 38 +++++++----------- cloud-hypervisor/tests/integration.rs | 13 +++---- vmm/src/api/mod.rs | 55 ++++++++++++++++++++++++++- 3 files changed, 73 insertions(+), 33 deletions(-) diff --git a/cloud-hypervisor/src/bin/ch-remote.rs b/cloud-hypervisor/src/bin/ch-remote.rs index 6939eaa38..afc41e7e9 100644 --- a/cloud-hypervisor/src/bin/ch-remote.rs +++ b/cloud-hypervisor/src/bin/ch-remote.rs @@ -16,7 +16,9 @@ use api_client::{ Error as ApiClientError, simple_api_command, simple_api_command_with_fds, simple_api_full_command, }; -use clap::{Arg, ArgAction, ArgMatches, Command}; +#[cfg(feature = "dbus_api")] +use clap::ArgAction; +use clap::{Arg, ArgMatches, Command}; use log::error; use option_parser::{ByteSized, ByteSizedParseError}; use thiserror::Error; @@ -69,6 +71,8 @@ enum Error { ReadingFile(#[source] std::io::Error), #[error("Invalid disk size")] InvalidDiskSize(#[source] ByteSizedParseError), + #[error("Error parsing send migration configuration")] + SendMigrationConfig(#[from] vmm::api::VmSendMigrationParseError), } enum TargetApi<'a> { @@ -519,11 +523,7 @@ fn rest_api_do_command(matches: &ArgMatches, socket: &mut UnixStream) -> ApiResu .unwrap() .get_one::("send_migration_config") .unwrap(), - matches - .subcommand_matches("send-migration") - .unwrap() - .get_flag("send_migration_local"), - ); + )?; simple_api_command(socket, "PUT", "send-migration", Some(&send_migration_data)) .map_err(Error::HttpApiClient) } @@ -743,11 +743,7 @@ fn dbus_api_do_command(matches: &ArgMatches, proxy: &DBusApi1ProxyBlocking<'_>) .unwrap() .get_one::("send_migration_config") .unwrap(), - matches - .subcommand_matches("send-migration") - .unwrap() - .get_flag("send_migration_local"), - ); + )?; proxy.api_vm_send_migration(&send_migration_data) } Some("receive-migration") => { @@ -953,13 +949,11 @@ fn receive_migration_data(url: &str) -> String { serde_json::to_string(&receive_migration_data).unwrap() } -fn send_migration_data(url: &str, local: bool) -> String { - let send_migration_data = vmm::api::VmSendMigrationData { - destination_url: url.to_owned(), - local, - }; - - serde_json::to_string(&send_migration_data).unwrap() +fn send_migration_data(config: &str) -> Result { + let send_migration_data = + vmm::api::VmSendMigrationData::parse(config).map_err(Error::SendMigrationConfig)?; + let send_migration_config = serde_json::to_string(&send_migration_data).unwrap(); + Ok(send_migration_config) } fn create_data(path: &str) -> Result { @@ -1141,13 +1135,7 @@ fn get_cli_commands_sorted() -> Box<[Command]> { .arg( Arg::new("send_migration_config") .index(1) - .help(""), - ) - .arg( - Arg::new("send_migration_local") - .long("local") - .num_args(0) - .action(ArgAction::SetTrue), + .help(vmm::api::VmSendMigrationData::SYNTAX), ), Command::new("shutdown").about("Shutdown the VM"), Command::new("shutdown-vmm").about("Shutdown the VMM"), diff --git a/cloud-hypervisor/tests/integration.rs b/cloud-hypervisor/tests/integration.rs index 4b8a6b2e3..5454cae6b 100644 --- a/cloud-hypervisor/tests/integration.rs +++ b/cloud-hypervisor/tests/integration.rs @@ -9891,17 +9891,16 @@ mod live_migration { thread::sleep(std::time::Duration::new(1, 0)); // Start to send migration from the source VM - let mut args = [ + let args = [ format!("--api-socket={}", &src_api_socket), "send-migration".to_string(), - format! {"unix:{migration_socket}"}, + format!( + "destination_url=unix:{migration_socket},local={}", + if local { "on" } else { "off" } + ), ] .to_vec(); - if local { - args.insert(2, "--local".to_string()); - } - let mut send_migration = Command::new(clh_command("ch-remote")) .args(&args) .stderr(Stdio::piped()) @@ -11066,7 +11065,7 @@ mod live_migration { .args([ &format!("--api-socket={src_api_socket}"), "send-migration", - &format!("tcp:{host_ip}:{migration_port}"), + &format!("destination_url=tcp:{host_ip}:{migration_port}"), ]) .stdin(Stdio::null()) .stderr(Stdio::piped()) diff --git a/vmm/src/api/mod.rs b/vmm/src/api/mod.rs index 17ba0011b..fd290a8a7 100644 --- a/vmm/src/api/mod.rs +++ b/vmm/src/api/mod.rs @@ -38,6 +38,7 @@ use std::sync::mpsc::{RecvError, SendError, Sender, channel}; use log::info; use micro_http::Body; +use option_parser::{OptionParser, OptionParserError, Toggle}; use serde::{Deserialize, Serialize}; use thiserror::Error; use vm_migration::MigratableError; @@ -266,7 +267,12 @@ pub struct VmReceiveMigrationData { pub receiver_url: String, } -#[derive(Clone, Deserialize, Serialize, Default, Debug)] +#[derive(Debug, Error)] +#[error("Error parsing send migration parameters")] +pub struct VmSendMigrationParseError(#[source] OptionParserError); + +/// Configuration for an outgoing migration. +#[derive(Clone, Deserialize, Serialize, Debug)] pub struct VmSendMigrationData { /// URL to migrate the VM to pub destination_url: String, @@ -275,6 +281,33 @@ pub struct VmSendMigrationData { pub local: bool, } +impl VmSendMigrationData { + pub const SYNTAX: &'static str = "VM send migration parameters \ + \"destination_url=[,local=on|off]\""; + + pub fn parse(migration: &str) -> Result { + let mut parser = OptionParser::new(); + parser.add("destination_url").add("local"); + parser.parse(migration).map_err(VmSendMigrationParseError)?; + + let destination_url = parser.get("destination_url").ok_or_else(|| { + VmSendMigrationParseError(OptionParserError::InvalidSyntax( + "destination_url is required".to_string(), + )) + })?; + let local = parser + .convert::("local") + .map_err(VmSendMigrationParseError)? + .unwrap_or(Toggle(false)) + .0; + + Ok(Self { + destination_url, + local, + }) + } +} + pub enum ApiResponsePayload { /// No data is sent on the channel. Empty, @@ -1541,3 +1574,23 @@ impl ApiAction for VmNmi { get_response_body(self, api_evt, api_sender, data) } } + +#[cfg(test)] +mod unit_tests { + use super::*; + + #[test] + fn test_vm_send_migration_data_parse() { + // Fully specified + let data = VmSendMigrationData::parse("destination_url=tcp://192.168.1.1:8080,local=on") + .expect("valid migration string should parse"); + assert_eq!(data.destination_url, "tcp://192.168.1.1:8080"); + assert!(data.local); + + // Unknown option is an error + VmSendMigrationData::parse("destination_url=unix:/tmp/sock,unknown_field=foo").unwrap_err(); + + // Invalid toggle value is an error + VmSendMigrationData::parse("destination_url=unix:/tmp/sock,local=yes").unwrap_err(); + } +}