From a935fc566fc0d7566f6edc27e0004a932a250072 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 15:19:45 +0100 Subject: [PATCH 01/10] [cli] Add a `--snapshots` flag to `info` --- src/client/cli/cmd/info.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/client/cli/cmd/info.cpp b/src/client/cli/cmd/info.cpp index 7d618b9ad3b..8691c769c87 100644 --- a/src/client/cli/cmd/info.cpp +++ b/src/client/cli/cmd/info.cpp @@ -68,13 +68,17 @@ mp::ParseCode cmd::Info::parse_args(mp::ArgParser* parser) "no-runtime-information", "Retrieve from the daemon only the information obtained without running commands on the instance"); noRuntimeInfoOption.setFlags(QCommandLineOption::HiddenFromHelp); - QCommandLineOption formatOption( + QCommandLineOption snapshots_option{"snapshots", + "Display detailed information about the snapshots of specified instances. This " + "option has no effect on snapshot arguments. Omit instance/snapshot arguments " + "to obtain detailed information on all the snapshots of all instances."}; + QCommandLineOption format_option( format_option_name, "Output info in the requested format.\nValid formats are: table (default), json, csv and yaml", format_option_name, "table"); - parser->addOptions({all_option, noRuntimeInfoOption, formatOption}); + parser->addOptions({all_option, noRuntimeInfoOption, snapshots_option, formatOption}); auto status = parser->commandParse(this); if (status != ParseCode::Ok) @@ -103,7 +107,8 @@ mp::ParseCode cmd::Info::parse_args(mp::ArgParser* parser) request.set_no_runtime_information(parser->isSet(noRuntimeInfoOption)); - if (instance_found && snapshot_found && parser->value(format_option_name) == "csv") + if (instance_found && snapshot_found && parser->value(format_option_name) == "csv" && + !parser->isSet(snapshots_option)) { cerr << "Mixed snapshot and instance arguments are not supported with CSV format\n"; return ParseCode::CommandLineError; From 031f1d8cd707548e88d818573df05cb016e8e957 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 12:38:30 +0100 Subject: [PATCH 02/10] [cli] Use consistent form in `info` options Add a dot at the end of options descriptions, where missing, to make them consistent with the new `--snapshots` option and follow the advise in Qt docs. Use camel case for `format_option`. --- src/client/cli/cmd/info.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/cli/cmd/info.cpp b/src/client/cli/cmd/info.cpp index 8691c769c87..b06e09655fb 100644 --- a/src/client/cli/cmd/info.cpp +++ b/src/client/cli/cmd/info.cpp @@ -62,11 +62,11 @@ mp::ParseCode cmd::Info::parse_args(mp::ArgParser* parser) "Names of instances or snapshots to display information about", "[.snapshot] [[.snapshot] ...]"); - QCommandLineOption all_option(all_option_name, "Display info for all instances"); + QCommandLineOption all_option(all_option_name, "Display info for all instances."); all_option.setFlags(QCommandLineOption::HiddenFromHelp); QCommandLineOption noRuntimeInfoOption( "no-runtime-information", - "Retrieve from the daemon only the information obtained without running commands on the instance"); + "Retrieve from the daemon only the information obtained without running commands on the instance."); noRuntimeInfoOption.setFlags(QCommandLineOption::HiddenFromHelp); QCommandLineOption snapshots_option{"snapshots", "Display detailed information about the snapshots of specified instances. This " @@ -74,11 +74,11 @@ mp::ParseCode cmd::Info::parse_args(mp::ArgParser* parser) "to obtain detailed information on all the snapshots of all instances."}; QCommandLineOption format_option( format_option_name, - "Output info in the requested format.\nValid formats are: table (default), json, csv and yaml", + "Output info in the requested format.\nValid formats are: table (default), json, csv and yaml.", format_option_name, "table"); - parser->addOptions({all_option, noRuntimeInfoOption, snapshots_option, formatOption}); + parser->addOptions({all_option, noRuntimeInfoOption, snapshots_option, format_option}); auto status = parser->commandParse(this); if (status != ParseCode::Ok) From 5368010d7a7fcb720368b12de6682420e83f5259 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 15:21:00 +0100 Subject: [PATCH 03/10] [rpc] Add and set a snapshots flag in InfoRequest --- src/client/cli/cmd/info.cpp | 6 ++++-- src/rpc/multipass.proto | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/client/cli/cmd/info.cpp b/src/client/cli/cmd/info.cpp index b06e09655fb..22ab54133ba 100644 --- a/src/client/cli/cmd/info.cpp +++ b/src/client/cli/cmd/info.cpp @@ -107,8 +107,10 @@ mp::ParseCode cmd::Info::parse_args(mp::ArgParser* parser) request.set_no_runtime_information(parser->isSet(noRuntimeInfoOption)); - if (instance_found && snapshot_found && parser->value(format_option_name) == "csv" && - !parser->isSet(snapshots_option)) + const auto& snapshots_only = parser->isSet(snapshots_option); + request.set_snapshots(snapshots_only); + + if (instance_found && snapshot_found && parser->value(format_option_name) == "csv" && !snapshots_only) { cerr << "Mixed snapshot and instance arguments are not supported with CSV format\n"; return ParseCode::CommandLineError; diff --git a/src/rpc/multipass.proto b/src/rpc/multipass.proto index f21328314b5..7c1aa503dc7 100644 --- a/src/rpc/multipass.proto +++ b/src/rpc/multipass.proto @@ -169,6 +169,7 @@ message InfoRequest { repeated InstanceSnapshotPair instances_snapshots = 1; int32 verbosity_level = 3; bool no_runtime_information = 4; + bool snapshots = 5; } message IdMap { From ab6b659569c693a2c87f93c749e721d277478bd1 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 15:16:31 +0100 Subject: [PATCH 04/10] [daemon] Fill details on `info --snapshots` --- src/daemon/daemon.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index a46f4f64a0c..d478116ecae 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1706,6 +1706,7 @@ try // clang-format on InstanceSnapshotsMap instance_snapshots_map; bool have_mounts = false; bool deleted = false; + bool snapshots_only = request->snapshots(); auto fetch_detailed_report = [&](VirtualMachine& vm) { fmt::memory_buffer errors; @@ -1716,15 +1717,28 @@ try // clang-format on try { + // TODO@ricab streamline this code if (all_or_none) - populate_instance_info(vm, - response.add_details(), - request->no_runtime_information(), - deleted, - have_mounts); + { + if (snapshots_only) + for (const auto& snapshot : vm.view_snapshots()) + populate_snapshot_info(vm, + snapshot, + response.add_details(), + have_mounts); // TODO@snapshots have_mounts doesn't make much sense here + else + populate_instance_info(vm, + response.add_details(), + request->no_runtime_information(), + deleted, + have_mounts); + } for (const auto& snapshot : pick) - populate_snapshot_info(vm, vm.get_snapshot(snapshot), response.add_details(), have_mounts); + populate_snapshot_info(vm, + vm.get_snapshot(snapshot), + response.add_details(), + have_mounts); // TODO@snapshots have_mounts doesn't make much sense here } catch (const NoSuchSnapshot& e) { From 183ad6167f1d09730b953dfd869475fd43a57286 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 16:52:19 +0100 Subject: [PATCH 05/10] [daemon] Streamline populating info response --- src/daemon/daemon.cpp | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index d478116ecae..a4f019a3e89 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1708,6 +1708,14 @@ try // clang-format on bool deleted = false; bool snapshots_only = request->snapshots(); + auto populate_info = [&](VirtualMachine& vm, const std::shared_ptr& snapshot) { + auto* details = response.add_details(); + if (snapshot) + populate_snapshot_info(vm, snapshot, details, have_mounts); // TODO@snapshots remove have_mounts + else + populate_instance_info(vm, details, request->no_runtime_information(), deleted, have_mounts); + }; + auto fetch_detailed_report = [&](VirtualMachine& vm) { fmt::memory_buffer errors; const auto& name = vm.vm_name; @@ -1717,28 +1725,17 @@ try // clang-format on try { - // TODO@ricab streamline this code if (all_or_none) { if (snapshots_only) for (const auto& snapshot : vm.view_snapshots()) - populate_snapshot_info(vm, - snapshot, - response.add_details(), - have_mounts); // TODO@snapshots have_mounts doesn't make much sense here + populate_info(vm, snapshot); else - populate_instance_info(vm, - response.add_details(), - request->no_runtime_information(), - deleted, - have_mounts); + populate_info(vm, nullptr); } - for (const auto& snapshot : pick) - populate_snapshot_info(vm, - vm.get_snapshot(snapshot), - response.add_details(), - have_mounts); // TODO@snapshots have_mounts doesn't make much sense here + for (const auto& snapshot_name : pick) + populate_info(vm, vm.get_snapshot(snapshot_name)); } catch (const NoSuchSnapshot& e) { From caeec0a0383c729a518b8f84f407f502fcedc4e6 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Tue, 17 Oct 2023 19:11:13 +0100 Subject: [PATCH 06/10] [bash] Add `info --snapshots` to bash completion --- completions/bash/multipass | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/completions/bash/multipass b/completions/bash/multipass index 4b0493872f6..3e00201b044 100644 --- a/completions/bash/multipass +++ b/completions/bash/multipass @@ -200,7 +200,7 @@ _multipass_complete() opts="${opts} --working-directory --no-map-working-directory" ;; "info") - _add_nonrepeating_args "--format" + _add_nonrepeating_args "--format --snapshots" ;; "list"|"ls"|"networks"|"aliases") _add_nonrepeating_args "--format" From d09c9164891809854d0e77bb25cbaa76650421e2 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 19 Oct 2023 15:23:19 +0100 Subject: [PATCH 07/10] [daemon] Fix repeated snapshot info Fix snapshot1 repeated in `multipass info foo foo.snapshot1 --snapshots` --- src/daemon/daemon.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index a4f019a3e89..ab1684f67fa 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1728,14 +1728,22 @@ try // clang-format on if (all_or_none) { if (snapshots_only) + { + for (const auto& snapshot_name : pick) + vm.get_snapshot(snapshot_name); // still verify validity of explicit snapshot names for (const auto& snapshot : vm.view_snapshots()) populate_info(vm, snapshot); + } else + { populate_info(vm, nullptr); + for (const auto& snapshot_name : pick) + populate_info(vm, vm.get_snapshot(snapshot_name)); + } } - - for (const auto& snapshot_name : pick) - populate_info(vm, vm.get_snapshot(snapshot_name)); + else + for (const auto& snapshot_name : pick) + populate_info(vm, vm.get_snapshot(snapshot_name)); } catch (const NoSuchSnapshot& e) { From be06d01409edd77a04c79261664e553c0e4168b0 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 19 Oct 2023 16:43:16 +0100 Subject: [PATCH 08/10] [daemon] Extract processing of snapshot info --- src/daemon/daemon.cpp | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index ab1684f67fa..89c207b1f16 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1716,34 +1716,35 @@ try // clang-format on populate_instance_info(vm, details, request->no_runtime_information(), deleted, have_mounts); }; + auto process_snapshot_pick = + [populate_info](VirtualMachine& vm, const SnapshotPick& snapshot_pick, bool snapshots_only) { + for (const auto& snapshot_name : snapshot_pick.pick) + { + const auto snapshot = vm.get_snapshot(snapshot_name); // verify validity even if unused + if (!snapshot_pick.all_or_none || !snapshots_only) + populate_info(vm, snapshot); + } + }; + auto fetch_detailed_report = [&](VirtualMachine& vm) { fmt::memory_buffer errors; const auto& name = vm.vm_name; const auto& it = instance_snapshots_map.find(name); - const auto& [pick, all_or_none] = it == instance_snapshots_map.end() ? SnapshotPick{{}, true} : it->second; + const auto& snapshot_pick = it == instance_snapshots_map.end() ? SnapshotPick{{}, true} : it->second; + const auto& [pick, all_or_none] = snapshot_pick; try { + process_snapshot_pick(vm, snapshot_pick, snapshots_only); // TODO@ricab capture snapshots only if (all_or_none) { if (snapshots_only) - { - for (const auto& snapshot_name : pick) - vm.get_snapshot(snapshot_name); // still verify validity of explicit snapshot names for (const auto& snapshot : vm.view_snapshots()) populate_info(vm, snapshot); - } else - { populate_info(vm, nullptr); - for (const auto& snapshot_name : pick) - populate_info(vm, vm.get_snapshot(snapshot_name)); - } } - else - for (const auto& snapshot_name : pick) - populate_info(vm, vm.get_snapshot(snapshot_name)); } catch (const NoSuchSnapshot& e) { From 6863937e6addd3155c8994f59afd6271f357e78e Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 19 Oct 2023 16:41:10 +0100 Subject: [PATCH 09/10] [daemon] Replace lambda param with capture --- src/daemon/daemon.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 89c207b1f16..3be877c8d28 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1716,15 +1716,15 @@ try // clang-format on populate_instance_info(vm, details, request->no_runtime_information(), deleted, have_mounts); }; - auto process_snapshot_pick = - [populate_info](VirtualMachine& vm, const SnapshotPick& snapshot_pick, bool snapshots_only) { - for (const auto& snapshot_name : snapshot_pick.pick) - { - const auto snapshot = vm.get_snapshot(snapshot_name); // verify validity even if unused - if (!snapshot_pick.all_or_none || !snapshots_only) - populate_info(vm, snapshot); - } - }; + auto process_snapshot_pick = [populate_info, snapshots_only](VirtualMachine& vm, + const SnapshotPick& snapshot_pick) { + for (const auto& snapshot_name : snapshot_pick.pick) + { + const auto snapshot = vm.get_snapshot(snapshot_name); // verify validity even if unused + if (!snapshot_pick.all_or_none || !snapshots_only) + populate_info(vm, snapshot); + } + }; auto fetch_detailed_report = [&](VirtualMachine& vm) { fmt::memory_buffer errors; @@ -1736,7 +1736,7 @@ try // clang-format on try { - process_snapshot_pick(vm, snapshot_pick, snapshots_only); // TODO@ricab capture snapshots only + process_snapshot_pick(vm, snapshot_pick); if (all_or_none) { if (snapshots_only) From c0ad644abd928e3b32ce07ad1be08440099f80f9 Mon Sep 17 00:00:00 2001 From: Ricardo Abreu Date: Thu, 19 Oct 2023 16:45:56 +0100 Subject: [PATCH 10/10] [daemon] Get rid of superfluous structured binding --- src/daemon/daemon.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 3be877c8d28..62a47062a59 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1732,12 +1732,11 @@ try // clang-format on const auto& it = instance_snapshots_map.find(name); const auto& snapshot_pick = it == instance_snapshots_map.end() ? SnapshotPick{{}, true} : it->second; - const auto& [pick, all_or_none] = snapshot_pick; try { process_snapshot_pick(vm, snapshot_pick); - if (all_or_none) + if (snapshot_pick.all_or_none) { if (snapshots_only) for (const auto& snapshot : vm.view_snapshots())