Skip to content
41 changes: 38 additions & 3 deletions src/daemon/daemon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,8 @@ mp::InstanceStatus::Status grpc_instance_status_for(const mp::VirtualMachine::St
return mp::InstanceStatus::SUSPENDING;
case mp::VirtualMachine::State::suspended:
return mp::InstanceStatus::SUSPENDED;
case mp::VirtualMachine::State::unavailable:
return mp::InstanceStatus::UNAVAILABLE;
case mp::VirtualMachine::State::unknown:
default:
return mp::InstanceStatus::UNKNOWN;
Expand Down Expand Up @@ -2083,6 +2085,12 @@ try
continue;
}

if (vm->current_state() == VirtualMachine::State::unavailable)
{
add_fmt_to(errors, "instance '{}' is not available", name);
continue;
}

auto& vm_mounts = mounts[name];
if (vm_mounts.find(target_path) != vm_mounts.end())
{
Expand Down Expand Up @@ -2259,9 +2267,12 @@ try
continue;
}
case VirtualMachine::State::suspending:
case VirtualMachine::State::unavailable:
fmt::format_to(std::back_inserter(start_errors),
"Cannot start the instance '{}' while suspending.",
name);
"Cannot start the instance '{}' while {}.",
name,
vm.current_state() == VirtualMachine::State::suspending ? "suspending"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be easier once the Hyper-V branch is in: https://github.com/canonical/multipass/pull/4080/files#diff-d847b18fbc46d755306da3eb6cac52de71043c1cc87ff345cdf8b034ad141ffaR158

You could add a TODO here, or convince @xmkg to split that off 😉

: "unavailable");
continue;
case VirtualMachine::State::delayed_shutdown:
delayed_shutdown_instances.erase(name);
Expand Down Expand Up @@ -2364,6 +2375,14 @@ try
if (status.ok())
{
status = cmd_vms(instance_selection.operative_selection, [this](auto& vm) {
if (vm.current_state() == VirtualMachine::State::unavailable)
{
mpl::log(mpl::Level::info,
vm.vm_name,
"Ignoring suspend since instance is unavailable.");
return grpc::Status::OK;
}
Comment on lines +2378 to +2384
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem: shouldn't this be handled by the VM? It would go in the direction of relieving the daemon from such concerns.


stop_mounts(vm.vm_name);

vm.suspend();
Expand Down Expand Up @@ -2485,6 +2504,14 @@ try
{
const auto& instance_name = vm_it->first;

if (vm_it->second->current_state() == VirtualMachine::State::unavailable)
{
mpl::log(mpl::Level::info,
instance_name,
"Ignoring delete since instance is unavailable.");
continue;
}

auto snapshot_pick_it = instance_snapshots_map.find(instance_name);
const auto& [pick, all] = snapshot_pick_it == instance_snapshots_map.end()
? SnapshotPick{{}, true}
Expand Down Expand Up @@ -2533,12 +2560,19 @@ try
const auto target_path =
QDir::cleanPath(QString::fromStdString(path_entry.target_path())).toStdString();

if (operative_instances.find(name) == operative_instances.end())
auto vm = operative_instances.find(name);
if (vm == operative_instances.end())
{
add_fmt_to(errors, "instance '{}' does not exist", name);
continue;
}

if (vm->second->current_state() == VirtualMachine::State::unavailable)
{
mpl::log(mpl::Level::info, name, "Ignoring umount since instance unavailable.");
continue;
}

auto& vm_spec_mounts = vm_instance_specs[name].mounts;
auto& vm_mounts = mounts[name];

Expand Down Expand Up @@ -3730,6 +3764,7 @@ error_string mp::Daemon::async_wait_for_ssh_and_start_mounts_for(
return fmt::to_string(errors);
}
const auto vm = it->second;

vm->wait_until_ssh_up(timeout);

if (std::is_same<Reply, LaunchReply>::value)
Expand Down
4 changes: 4 additions & 0 deletions src/platform/backends/hyperv/hyperv_virtual_machine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,10 @@ void mp::HyperVVirtualMachine::suspend()
{
mpl::info(vm_name, "Ignoring suspend issued while stopped");
}
else if (present_state == State::unavailable)
{
mpl::log(mpl::Level::info, vm_name, "Ignoring suspend since instance is unavailable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem: merge with the above?

Otherwise, use mpl::info?

}

monitor->on_suspend();
}
Expand Down
6 changes: 4 additions & 2 deletions src/platform/backends/qemu/qemu_virtual_machine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -426,9 +426,11 @@ void mp::QemuVirtualMachine::suspend()

vm_process.reset(nullptr);
}
else if (state == State::off || state == State::suspended)
else if (state == State::off || state == State::suspended || state == State::unavailable)
{
mpl::info(vm_name, "Ignoring suspend issued while stopped/suspended");
mpl::log(mpl::Level::info,
vm_name,
"Ignoring suspend issued while stopped/suspended/unavailable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem: state formatting would help

monitor->on_suspend();
}
}
Expand Down
28 changes: 26 additions & 2 deletions src/platform/backends/shared/base_availability_zone.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,32 @@ void BaseAvailabilityZone::set_available(const bool new_available)
m.available = new_available;
serialize();

for (auto& vm : m.vms)
vm.get().set_available(m.available);
try
{
for (auto& vm : m.vms)
vm.get().set_available(new_available);
}
catch (...)
{
// if an error occurs fallback to available.
m.available = true;
serialize();
Comment on lines +128 to +129
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just do this at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the catch is rethrowing, control flow would never reach the end of the function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. You could use a scope guard, but no need really.


// make sure nothing is still unavailable.
for (auto& vm : m.vms)
{
// setting the state here breaks encapsulation, but it's already broken.
std::unique_lock vm_lock{vm.get().state_mutex};
if (vm.get().current_state() == VirtualMachine::State::unavailable)
{
vm.get().state = VirtualMachine::State::off;
vm.get().update_state();
}
Comment on lines +134 to +140
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered force-stop instead? That would avoid breaking encapsulation yet again and make sure there were no leftover processes.

}

// rethrow the error so something else can deal with it.
throw;
}
}

void BaseAvailabilityZone::add_vm(VirtualMachine& vm)
Expand Down
27 changes: 27 additions & 0 deletions src/platform/backends/shared/base_virtual_machine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ void mp::BaseVirtualMachine::check_state_for_shutdown(ShutdownPolicy shutdown_po
{
throw VMStateIdempotentException{"Ignoring shutdown since instance is already stopped."};
}
if (state == State::unavailable)
{
throw VMStateIdempotentException{"Ignoring shutdown since instance is unavailable."};
}
Comment on lines +207 to +210
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about merging with the above branch here as well. Again, the formatter would help. If you want to simplify, I wouldn't be shocked if the message read "Ignoring shutdown since instance is already unavailable".


if (shutdown_policy == ShutdownPolicy::Poweroff)
{
Expand Down Expand Up @@ -238,6 +242,29 @@ void mp::BaseVirtualMachine::check_state_for_shutdown(ShutdownPolicy shutdown_po
}
}

void mp::BaseVirtualMachine::set_available(bool available)
{
if (available)
{
state = State::off;
update_state();
if (was_running)
{
start();

// normally the daemon sets the state to running...
state = State::running;
update_state();
}
return;
}

was_running = state == State::running || state == State::starting || state == State::restarting;
shutdown(ShutdownPolicy::Poweroff);
state = State::unavailable;
update_state();
}

std::string mp::BaseVirtualMachine::ssh_exec(const std::string& cmd, bool whisper)
{
std::unique_lock lock{state_mutex};
Expand Down
8 changes: 2 additions & 6 deletions src/platform/backends/shared/base_virtual_machine.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,7 @@ class BaseVirtualMachine : public VirtualMachine

virtual std::string ssh_exec(const std::string& cmd, bool whisper = false) override;

void set_available(bool available) override
{
// TODO make vm unavailable by force stopping if running or available by starting again if
// it was running
throw NotImplementedOnThisBackendException("unavailability");
}
void set_available(bool available) override;

void wait_until_ssh_up(std::chrono::milliseconds timeout) override;
void wait_for_cloud_init(std::chrono::milliseconds timeout) override;
Expand Down Expand Up @@ -181,6 +176,7 @@ class BaseVirtualMachine : public VirtualMachine
std::shared_ptr<Snapshot> head_snapshot = nullptr;
int snapshot_count = 0; // tracks the number of snapshots ever taken (regardless of deletes)
mutable std::recursive_mutex snapshot_mutex;
bool was_running{false};
};

} // namespace multipass
Expand Down
2 changes: 1 addition & 1 deletion src/platform/platform_linux.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ mp::VirtualMachineFactory::UPtr mp::platform::vm_backend(const mp::Path& data_di

#if VIRTUALBOX_ENABLED
if (driver == QStringLiteral("virtualbox"))
return std::make_unique<VirtualBoxVirtualMachineFactory>(data_dir);
return std::make_unique<VirtualBoxVirtualMachineFactory>(data_dir, az_manager);
#endif

throw std::runtime_error(fmt::format("Unsupported virtualization driver: {}", driver));
Expand Down
1 change: 1 addition & 0 deletions src/rpc/multipass.proto
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ message InstanceStatus {
DELAYED_SHUTDOWN = 6;
SUSPENDING = 7;
SUSPENDED = 8;
UNAVAILABLE = 9;
}
Status status = 1;
}
Expand Down
34 changes: 34 additions & 0 deletions tests/test_base_virtual_machine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ struct MockBaseVirtualMachine : public mpt::MockVirtualMachineT<mp::BaseVirtualM
get_snapshot,
mp::BaseVirtualMachine,
(A<const std::string&>()));
MP_DELEGATE_MOCK_CALLS_ON_BASE_WITH_MATCHERS(*this,
set_available,
mp::BaseVirtualMachine,
(A<bool>()));
}

MOCK_METHOD(std::shared_ptr<mp::Snapshot>,
Expand Down Expand Up @@ -1441,4 +1445,34 @@ TEST_F(BaseVM, sshExecRethrowsSSHExceptionsWhenConnected)
mpt::match_what(HasSubstr("intentional")));
}

TEST_F(BaseVM, setUnavailableShutsdownRunning)
{
ON_CALL(vm, current_state).WillByDefault(Return(St::running));
EXPECT_CALL(vm, shutdown(mp::VirtualMachine::ShutdownPolicy::Poweroff)).Times(1);

vm.set_available(false);
}

TEST_F(BaseVM, setAvailableRestartsRunning)
{
StubBaseVirtualMachine base_vm(zone, St::running);

base_vm.set_available(false);
ASSERT_EQ(base_vm.current_state(), St::unavailable);

base_vm.set_available(true);
EXPECT_EQ(base_vm.current_state(), St::running);
}

TEST_F(BaseVM, setAvailableKeepsOffOff)
{
StubBaseVirtualMachine base_vm(zone, St::off);

base_vm.set_available(false);
ASSERT_EQ(base_vm.current_state(), St::unavailable);

base_vm.set_available(true);
EXPECT_EQ(base_vm.current_state(), St::off);
}

} // namespace
16 changes: 12 additions & 4 deletions tests/test_daemon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,9 @@ TEST_F(Daemon, ensureThatOnRestartFutureCompletes)

// This VM was running before, but not now.
auto mock_vm = std::make_unique<NiceMock<mpt::MockVirtualMachine>>("yakety-yak");
EXPECT_CALL(*mock_vm, current_state).WillOnce(Return(mp::VirtualMachine::State::stopped));
EXPECT_CALL(*mock_vm, current_state)
.Times(1)
.WillRepeatedly(Return(mp::VirtualMachine::State::stopped));
EXPECT_CALL(*mock_vm, start).Times(1);

mp::Signal sig;
Expand Down Expand Up @@ -471,7 +473,9 @@ TEST_F(Daemon, startsPreviouslyRunningVmsBack)

// This VM was running before, but not now.
auto mock_vm = std::make_unique<NiceMock<mpt::MockVirtualMachine>>(vm_props.name);
EXPECT_CALL(*mock_vm, current_state).WillOnce(Return(mp::VirtualMachine::State::stopped));
EXPECT_CALL(*mock_vm, current_state)
.Times(1)
.WillRepeatedly(Return(mp::VirtualMachine::State::stopped));
EXPECT_CALL(*mock_vm, start).Times(1);
EXPECT_CALL(*mock_vm, update_state).Times(1);
EXPECT_CALL(*mock_vm, wait_until_ssh_up).Times(1);
Expand All @@ -492,7 +496,9 @@ TEST_F(Daemon, callsOnRestartForAlreadyRunningVmsOnConstruction)

// This VM was running before, but not now.
auto mock_vm = std::make_unique<NiceMock<mpt::MockVirtualMachine>>(vm_props.name);
EXPECT_CALL(*mock_vm, current_state).WillOnce(Return(mp::VirtualMachine::State::running));
EXPECT_CALL(*mock_vm, current_state)
.Times(1)
.WillRepeatedly(Return(mp::VirtualMachine::State::running));
EXPECT_CALL(*mock_vm, start).Times(0);
EXPECT_CALL(*mock_vm, update_state).Times(1);
EXPECT_CALL(*mock_vm, wait_until_ssh_up).Times(1);
Expand All @@ -513,7 +519,9 @@ TEST_F(Daemon, callsOnRestartForAlreadyStartingVmsOnConstruction)

// This VM was running before, but not now.
auto mock_vm = std::make_unique<NiceMock<mpt::MockVirtualMachine>>(vm_props.name);
EXPECT_CALL(*mock_vm, current_state).WillOnce(Return(mp::VirtualMachine::State::starting));
EXPECT_CALL(*mock_vm, current_state)
.Times(1)
.WillRepeatedly(Return(mp::VirtualMachine::State::starting));
EXPECT_CALL(*mock_vm, start).Times(0);
EXPECT_CALL(*mock_vm, update_state).Times(1);
EXPECT_CALL(*mock_vm, wait_until_ssh_up).Times(1);
Expand Down
Loading