Merge pull request #3764 from canonical/unify_invalid_state_to_failed…

…_precondition [daemon] unified restart, restore and snapshot to grpc::FAILED_PRECON…
canonical · Dec 2, 2024 · bf396bd · bf396bd
2 parents a5fefa4 + 19a50a2
commit bf396bd
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 9 deletions.
diff --git a/src/client/cli/cmd/clone.h b/src/client/cli/cmd/clone.h
@@ -37,5 +37,5 @@ class Clone final : public Command
 
     CloneRequest rpc_request;
 };
-} // namespace multipass
+} // namespace multipass::cmd
 #endif // MULTIPASS_CLONE_H
diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp
@@ -2498,6 +2498,10 @@ catch (const mp::UnrecognizedSettingException& e)
 {
     status_promise->set_value(grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, e.what(), ""));
 }
+catch (const mp::InstanceStateSettingsException& e)
+{
+    status_promise->set_value(grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what(), ""));
+}
 catch (const mp::InvalidSettingException& e)
 {
     status_promise->set_value(grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, e.what(), ""));
@@ -2586,7 +2590,7 @@ try
         using St = VirtualMachine::State;
         if (auto state = vm_ptr->current_state(); state != St::off && state != St::stopped)
             return status_promise->set_value(
-                grpc::Status{grpc::INVALID_ARGUMENT, "Multipass can only take snapshots of stopped instances."});
+                grpc::Status{grpc::FAILED_PRECONDITION, "Multipass can only take snapshots of stopped instances."});
 
         auto snapshot_name = request->snapshot();
         if (!snapshot_name.empty() && !mp::utils::valid_hostname(snapshot_name))
@@ -2641,7 +2645,7 @@ try
         using St = VirtualMachine::State;
         if (auto state = vm_ptr->current_state(); state != St::off && state != St::stopped)
             return status_promise->set_value(
-                grpc::Status{grpc::INVALID_ARGUMENT, "Multipass can only restore snapshots of stopped instances."});
+                grpc::Status{grpc::FAILED_PRECONDITION, "Multipass can only restore snapshots of stopped instances."});
 
         auto spec_it = vm_instance_specs.find(instance_name);
         assert(spec_it != vm_instance_specs.end() && "missing instance specs");
@@ -3246,7 +3250,7 @@ grpc::Status mp::Daemon::reboot_vm(VirtualMachine& vm)
         delayed_shutdown_instances.erase(vm.vm_name);
 
     if (!MP_UTILS.is_running(vm.current_state()))
-        return grpc::Status{grpc::StatusCode::INVALID_ARGUMENT,
+        return grpc::Status{grpc::StatusCode::FAILED_PRECONDITION,
                             fmt::format("Instance '{0}' is already running, but in an unknown state.\n"
                                         "Try to stop and start it instead.",
                                         vm.vm_name),
@@ -3670,7 +3674,8 @@ std::string mp::Daemon::dest_name_for_clone(const CloneRequest& request)
 {
     return request.has_destination_name()
                ? request.destination_name()
-               : generate_next_clone_name(vm_instance_specs[request.source_name()].clone_count, request.source_name());
+               : generate_next_clone_name(vm_instance_specs.at(request.source_name()).clone_count,
+                                          request.source_name());
 };
 
 grpc::Status mp::Daemon::validate_dest_name(const std::string& name)

diff --git a/src/daemon/instance_settings_handler.cpp b/src/daemon/instance_settings_handler.cpp
@@ -99,8 +99,9 @@ void check_state_for_update(mp::VirtualMachine& instance)
 {
     auto st = instance.current_state();
     if (st != mp::VirtualMachine::State::stopped && st != mp::VirtualMachine::State::off)
-        throw mp::InstanceSettingsException{operation_msg(Operation::Modify), instance.vm_name,
-                                            "Instance must be stopped for modification"};
+        throw mp::InstanceStateSettingsException{operation_msg(Operation::Modify),
+                                                 instance.vm_name,
+                                                 "Instance must be stopped for modification"};
 }
 
 mp::MemorySize get_memory_size(const QString& key, const QString& val)

diff --git a/src/daemon/instance_settings_handler.h b/src/daemon/instance_settings_handler.h
@@ -72,6 +72,12 @@ class InstanceSettingsException : public SettingsException
     InstanceSettingsException(const std::string& reason, const std::string& instance, const std::string& detail);
 };
 
+class InstanceStateSettingsException : public InstanceSettingsException
+{
+public:
+    using InstanceSettingsException::InstanceSettingsException;
+};
+
 class NonAuthorizedBridgeSettingsException : public InstanceSettingsException
 {
 public:

diff --git a/tests/test_daemon_snapshot_restore.cpp b/tests/test_daemon_snapshot_restore.cpp
@@ -131,7 +131,7 @@ TYPED_TEST(TestDaemonSnapshotRestoreCommon, failsOnActiveInstance)
     auto status =
         this->call_daemon_slot(*daemon, TypeParam::daemon_slot_ptr, request, typename TestFixture::MockServer{});
 
-    EXPECT_EQ(status.error_code(), grpc::INVALID_ARGUMENT);
+    EXPECT_EQ(status.error_code(), grpc::FAILED_PRECONDITION);
     EXPECT_THAT(status.error_message(), HasSubstr("stopped"));
 }
 

diff --git a/tests/test_instance_settings_handler.cpp b/tests/test_instance_settings_handler.cpp
@@ -572,7 +572,7 @@ TEST_P(TestInstanceModOnNonStoppedInstance, setRefusesToModifyNonStoppedInstance
     EXPECT_CALL(target_instance, current_state).WillOnce(Return(state));
 
     MP_EXPECT_THROW_THAT(make_handler().set(make_key(target_instance_name, property), "123"),
-                         mp::InstanceSettingsException,
+                         mp::InstanceStateSettingsException,
                          mpt::match_what(AllOf(HasSubstr("Cannot update"), HasSubstr("Instance must be stopped"))));
 
     EXPECT_EQ(original_specs, specs[target_instance_name]);