[daemon] Handle cloud-init intentional shutdown#4695
Conversation
|
Hi @tobe2098 , I will required your feedback on this solution approach. |
|
Hi, it is a compilation error, unrelated to |
ab66f47 to
cd4916d
Compare
|
Hi @tobe2098 , |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4695 +/- ##
==========================================
- Coverage 87.64% 87.51% -0.13%
==========================================
Files 254 259 +5
Lines 14157 14155 -2
==========================================
- Hits 12407 12386 -21
- Misses 1750 1769 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tobe2098
left a comment
There was a problem hiding this comment.
The work shows promise, but I think we need to re-formulate the solution. Ideally, wait_* functions would return as if there were a success in the intentional shutdown case, since it is a success. Both wait_* functions were already re-formatted to accomodate restarts, so in the case where a restart would be detected within the try_to_ssh function due to state not being running, we could check if the state is shutdown and the intended state is shutdown as well and return TimeoutAction::Done.
What do you think about this alternative solution?
| if(vm_desc.user_data_config["power_state"]) | ||
| { | ||
| auto ps = vm_desc.user_data_config["power_state"]; | ||
| if(ps["mode"] && ps["mode"].as<std::string>() == "poweroff") | ||
| { | ||
| mpl::log(mpl::Level::error, name, "DETECTED POWEROFF IN CONFIG"); | ||
| vm_desc.expects_shutdown = true; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Since vm_desc already has access to the data, would it make more sense to just parse the power_state value in-situ? Once you enter the function call that requires the information it could be parsed there for the value, instead of adding an additional field that does not really correspond to "VMDescription", since it is just an option of the contained cloud-init (if we were talking about a ParsedCloudInit object I would agree with the approach).
If the field were in BaseVirtualMachine it would be more convenient, since that virtual class does not have access to the VMDescription. The default could be false, and the field could be set in the constructor of the derived classes, where there is access to the VMDescription. What do you think?
There was a problem hiding this comment.
Yes i think your approach make more sense.
My thinking was that parsing in the daemon would make the implementation work across all backends automatically. I will try implementing it with the VirtualBox backed first and update here.
| if ((state == State::delayed_shutdown && present_state == State::running) || | ||
| state == State::starting) | ||
| if (state == State::starting && present_state == State::stopped) | ||
| { | ||
| mpl::log(mpl::Level::info, | ||
| name.toStdString(), | ||
| "VM stopped during startup (cloud-init poweroff)"); | ||
|
|
||
| state = present_state; | ||
| return state; | ||
| } | ||
|
|
||
| if (state == State::delayed_shutdown && present_state == State::running) | ||
| return state; |
There was a problem hiding this comment.
Is this logic really necessary? Supporting the intentional shutdown should be backend-wide, and this provides no additional functionality. Let me know what your thinking process here was, there are other logic changes here (like what if starting && not stopped?), for which an explanation would make things easier.
There was a problem hiding this comment.
You are right, this does not support any logic. I added this only for debugging purpose while testing, to check the logs. I will remove this changes in the next commit. I will make sure that this type of mistake will not happen again.
| if (state == State::starting && present_state == State::stopped) | ||
| { | ||
| mpl::log(mpl::Level::info, | ||
| name.toStdString(), | ||
| "VM stopped during startup (cloud-init poweroff)"); | ||
|
|
||
| state = present_state; | ||
| return state; |
There was a problem hiding this comment.
This updates the state variable when starting when it used to not be the case. What was your reasoning for this change?
|
Hi @tobe2098, |
tobe2098
left a comment
There was a problem hiding this comment.
We are getting closer @deepakshirkem. It is not complete because it could be the case that the shutdown is detected on waiting for cloud-init, instead of on ssh up. That would be the case if the cloud-init takes long enough in longer cloud-init configurations.
We should also add the logic to the other backends. To avoid the repeated code, the yaml parsing code could be a function as well.
| if (vm_state == State::stopped || vm_state == State::off) | ||
| { | ||
| if (expected_shutdown) { | ||
| mpl::log(mpl::Level::info, vm_name, | ||
| "VM powered off as configured in cloud-init"); | ||
| return utils::TimeoutAction::done; | ||
| } else { | ||
| mpl::log(mpl::Level::error, vm_name, | ||
| "VM stopped unexpectedly"); | ||
| throw StartException(vm_name, "VM stopped unexpectedly"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe the exception could be thrown here instead of after the try_action_for.
There was a problem hiding this comment.
Hi @tobe2098, I tried throwing the exception as you suggested and removed the final state check, but after that I am not able to see the intentional exception or the start exception. I am not sure why this behavior changed. I will dig deeper into it and add logging to investigate.
There was a problem hiding this comment.
Remember that if the shutdown happens during the cloud-init after the ssh_up you are not throwing in that function yet.
There was a problem hiding this comment.
Yeah, you are right. But in my updated code, I am throwing the exception from that function. After some debugging, I think there may be a race condition issue—my vm_state is not updating inside the lambda.
So, can we do the same as in the old code where I added a final check? It introduces some delay, but it gives the expected results.
I wanted your feedback—does my observation about a race condition make sense?
There was a problem hiding this comment.
It makes sense, but if you check the vm_state=current_state(); the state is supposed to be properly updated. I can do more testing on that later on.
There was a problem hiding this comment.
Hi @tobe2098, You're right! I was testing the crash scenario incorrectly. I was stopping the VM before it reached the running state, which is why the state wasn't updating as expected in the lambda. That's why I thought there was a race condition.
|
Hi @tobe2098, I implemented the changes as you suggested and added the helper function. I was confused during testing because, while doing the crash test manually, I stopped the VM before it reached the running state. That’s why it never stopped as expected, and I was not getting the expected behavior. |
|
Hi @deepakshirkem, if there are conflicts do not merge main into the branch, do |
tobe2098
left a comment
There was a problem hiding this comment.
I made some minor comments. You are doing great work @deepakshirkem!
Additionally, there is something that must be taken care of. In daemon.cpp, wait_for_ssh_up is called in all started/restarted instances, not only the launched ones. When dealing with the IntentionalShutdownException, we must treat it as a StartException whenever it is not a LaunchRequest.
|
|
||
| if ((state == State::delayed_shutdown && present_state == State::running) || | ||
| state == State::starting) | ||
| if (state == State::delayed_shutdown && present_state == State::running) |
There was a problem hiding this comment.
Why is starting not checked now? Did you find something while testing?
There was a problem hiding this comment.
Hi @tobe2098, I have addressed this change.
| auto on_timeout = [] { | ||
| throw std::runtime_error("timed out waiting for initialization to complete"); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Could this newline be removed?
| catch (const SSHExecFailure& e) // transitioning away from catching generic runtime errors | ||
| { // TODO remove once we're confident this is an anomaly | ||
| return mpu::TimeoutAction::retry; | ||
| } | ||
| catch (const std::exception& e) // transitioning away from catching generic runtime errors | ||
| { // TODO remove once we're confident this is an anomaly | ||
| catch (const std::exception& e) | ||
| { |
There was a problem hiding this comment.
This comment was moved accidentally
| if (vm_state == State::stopped || vm_state == State::off) | ||
| { | ||
| if (expected_shutdown) { | ||
| mpl::log(mpl::Level::info, vm_name, | ||
| "VM powered off as configured in cloud-init"); | ||
| return utils::TimeoutAction::done; | ||
| } else { | ||
| mpl::log(mpl::Level::error, vm_name, | ||
| "VM stopped unexpectedly"); | ||
| throw StartException(vm_name, "VM stopped unexpectedly"); | ||
| } | ||
| } |
There was a problem hiding this comment.
It makes sense, but if you check the vm_state=current_state(); the state is supposed to be properly updated. I can do more testing on that later on.
|
Hi @tobe2098, I addressed all your review comments. Thank You ((: |
45d00d1 to
b6219ba
Compare
| { | ||
| throw StartException(name, "Unexpected shutdown during start"); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is an additional topic: when launching a vm, mount options can be provided, which should still be added to the VM even if it is shut off. The current approach would discard the mounts by returning early. Of course, the classic mounts need to check that the vm is running, similar as is done in the Daemon::mount function. What do you think?
There was a problem hiding this comment.
Hi @tobe2098, Your right. I have to check existing code and provide solution that will handle this case. Thank you.
c3efd4b to
5de7d61
Compare
|
Hi @tobe2098, Please go one more time on the changes and sorry for those test failure i tested locally now no test is failing. Please re-run the pipeline when you get a chance. Thank you ::) |
tobe2098
left a comment
There was a problem hiding this comment.
Sorry to not have been clear with the mounts activation. The mounts are activated in src/client/cli/launch.cpp when launching. The way it works is that if the launch returns an OK code, the mounts will be attempted on the VM as a separate command. The changes you added can be undone, simply test that then launching a VM that starts and shuts down with the cloud-init, that the mounts via mount flags are stored.
Detect when cloud-init uses power_state mode poweroff/halt and treat as successful launch instead of timeout. Created expects_shutdown_from_cloud_init() helper function. Implemented in all backend constructors and detect_aborted_start. Handles intentional shutdown in wait functions. Fixes VirtualBox current_state to detect stopped state immediately. Fixes #4456 [daemon] Move parsing to backend constructors [backends] Add shutdown detection to all Created expects_shutdown_from_cloud_init() helper function. Implemented in all backend constructors and detect_aborted_start. Handles intentional shutdown in wait functions. [review] Address review feedback - Restore missing 'starting' state check in VirtualBox current_state - Fix VirtualBox current_state to detect stopped state immediately - Remove duplicate TODO comment in wait_for_cloud_init - Remove extra newline in wait_for_cloud_init - Add if constexpr check to only treat IntentionalShutdownException as success for LaunchRequest, not StartRequest [review] Fix whitespace and test failure - Remove trailing whitespace (per GIT9) - Add expected_shutdown check to detect_aborted_start - Fixes BaseVM.waitForCloudInitVMDownReconnects test
- Remove early return for intentional shutdown during launch - Save mount configuration even when VM is stopped - Skip mount activation if VM not running - Fix missing braces in detect_aborted_start - Apply clang-format to all modified files Ensures mounts are ready when VM starts later. All tests passing including BaseVM.waitForCloudInitVMDownReconnects.
Add extra current_state() call expectation to match additional state check in wait_for_cloud_init.
5de7d61 to
b4e1a2e
Compare
| .WillOnce(Return(mp::VirtualMachine::State::running)) // when 1st waiting for cloud-init | ||
| .WillOnce(Return(mp::VirtualMachine::State::restarting)) // when retrying to ssh | ||
| .WillOnce(Return(mp::VirtualMachine::State::running)); // back waiting for cloud-init | ||
| .WillOnce(Return(mp::VirtualMachine::State::running)); // back to cloud-init |
There was a problem hiding this comment.
Added this call. Do you think this is the right approach? The test is passing locally, but I observed that it is failing in CI.
|
Very well done @deepakshirkem. Thank you for your patience :D. I think the solution is achieved well, but the second commit's changes are not necessary as I stated in my previous message. The mounts for |
There was a problem hiding this comment.
Pull request overview
This PR improves multipass launch behavior when user-provided cloud-init intentionally powers off the VM (via power_state), by detecting that intent and treating the resulting shutdown as a successful initialization outcome rather than waiting for SSH until timeout.
Changes:
- Add cloud-init YAML inspection to detect
power_state.mode: poweroff|halt(intentional shutdown expectation). - Propagate the “expected shutdown” behavior into backend VMs and
BaseVirtualMachinestartup/wait logic via a newIntentionalShutdownException. - Teach the daemon’s launch flow to treat
IntentionalShutdownExceptionas success for launch (while still failing non-launch starts).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_base_virtual_machine.cpp | Updates unit test expectations related to cloud-init wait behavior. |
| src/utils/utils.cpp | Adds helper to detect expected shutdown from cloud-init YAML. |
| include/multipass/utils.h | Exposes the new cloud-init shutdown detection helper in the public utils API. |
| src/platform/backends/shared/base_virtual_machine.h | Adds an expected_shutdown flag to the base VM implementation. |
| src/platform/backends/shared/base_virtual_machine.cpp | Throws IntentionalShutdownException on shutdown during startup when expected; adjusts SSH/cloud-init wait paths. |
| include/multipass/exceptions/intentional_shutdown_exception.h | Introduces a dedicated exception type for intentional shutdown during initialization. |
| src/daemon/daemon.cpp | Catches IntentionalShutdownException and treats it as success for launch operations. |
| src/platform/backends/qemu/qemu_virtual_machine.cpp | Initializes expected_shutdown from cloud-init YAML in QEMU backend. |
| src/platform/backends/virtualbox/virtualbox_virtual_machine.cpp | Initializes expected_shutdown from cloud-init YAML in VirtualBox backend. |
| src/platform/backends/hyperv/hyperv_virtual_machine.cpp | Initializes expected_shutdown from cloud-init YAML in Hyper-V backend. |
| src/platform/backends/applevz/applevz_virtual_machine.cpp | Initializes expected_shutdown from cloud-init YAML in AppleVZ backend (and adds required include). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EXPECT_CALL(vm, current_state) | ||
| .WillOnce(Return(mp::VirtualMachine::State::running)) // when 1st waiting for cloud-init | ||
| .WillOnce(Return(mp::VirtualMachine::State::restarting)) // when retrying to ssh | ||
| .WillOnce(Return(mp::VirtualMachine::State::running)); // back waiting for cloud-init | ||
| .WillOnce(Return(mp::VirtualMachine::State::running)); // back to cloud-init |
| auto action = [this]() -> utils::TimeoutAction { | ||
| auto vm_state = current_state(); | ||
|
|
||
| if (vm_state == State::stopped || vm_state == State::off) | ||
| { | ||
| if (expected_shutdown) | ||
| { | ||
| mpl::log(mpl::Level::info, vm_name, "VM powered off as configured in cloud-init"); | ||
| throw IntentionalShutdownException(vm_name); | ||
| } | ||
| else | ||
| { | ||
| mpl::log(mpl::Level::error, vm_name, "VM stopped unexpectedly"); | ||
| throw StartException(vm_name, "VM stopped unexpectedly"); | ||
| } | ||
| } | ||
|
|
||
| return try_to_ssh(); | ||
| }; |
| detect_aborted_start(); | ||
|
|
||
| auto vm_state = current_state(); | ||
| if (vm_state == State::stopped || vm_state == State::off) | ||
| { | ||
| if (expected_shutdown) | ||
| { | ||
| mpl::log(mpl::Level::error, vm_name, "VM powered off as configured in cloud-init"); | ||
| throw IntentionalShutdownException(vm_name); |
| bool mp::utils::expects_shutdown_from_cloud_init(const YAML::Node& user_data_config) | ||
| { | ||
| if (user_data_config["power_state"]) | ||
| { | ||
| auto ps = user_data_config["power_state"]; | ||
| if (ps["mode"]) | ||
| { | ||
| std::string mode = ps["mode"].as<std::string>(); | ||
| return (mode == "poweroff" || mode == "halt"); | ||
| } | ||
| } | ||
| return false; | ||
| } |
| namespace | ||
| { | ||
| constexpr static auto log_category = "applevz-vm"; | ||
| } // namespace | ||
| #include <multipass/utils.h> | ||
|
|
| #pragma once | ||
|
|
||
| #include <stdexcept> | ||
| #include <string> | ||
|
|
||
| namespace multipass | ||
| { | ||
|
|
||
| class IntentionalShutdownException : public std::runtime_error | ||
| { | ||
| public: | ||
| explicit IntentionalShutdownException(const std::string& name) | ||
| : std::runtime_error{ | ||
| "Instance '" + name + | ||
| "' stopped intentionally during initialization " | ||
| "(cloud-init power_state directive)"} {} | ||
| }; | ||
|
|
||
| } No newline at end of file |
| if (expected_shutdown) | ||
| { | ||
| throw IntentionalShutdownException(vm_name); | ||
| } |
tobe2098
left a comment
There was a problem hiding this comment.
Hi again @deepakshirkem, I just noticed that the approach we went for has a small flaw. The issue itself is more complex than expected. Would you mind if I do some additional work on top of your commits? I believe the problem will require more extensive thought but I want to keep your contributions. What do you think?
| @@ -314,8 +337,23 @@ void mp::BaseVirtualMachine::wait_until_ssh_up(std::chrono::milliseconds timeout | |||
|
|
|||
| void mp::BaseVirtualMachine::wait_for_cloud_init(std::chrono::milliseconds timeout) | |||
| { | |||
| auto action = [this] { | |||
| auto action = [this]() -> mpu::TimeoutAction { | |||
| detect_aborted_start(); | |||
|
|
|||
| auto vm_state = current_state(); | |||
| if (vm_state == State::stopped || vm_state == State::off) | |||
| { | |||
| if (expected_shutdown) | |||
| { | |||
| mpl::log(mpl::Level::error, vm_name, "VM powered off as configured in cloud-init"); | |||
| throw IntentionalShutdownException(vm_name); | |||
| } | |||
| else | |||
| { | |||
| mpl::log(mpl::Level::error, vm_name, "VM stopped unexpectedly during cloud-init"); | |||
| throw StartException(vm_name, "VM Stopped unexpectedly during cloud-init"); | |||
| } | |||
| } | |||
| try | |||
| { | |||
| ssh_exec("[ -e /var/lib/cloud/instance/boot-finished ]"); | |||
There was a problem hiding this comment.
I just noticed an additional problem. expected_shutdown remains as true after launching and stopping which means that on a second start, a failed start would appear as a intentional shutdown.
Hi @tobe2098, Of course! You can build on top of these commits. I'm always available if you need anything from my side |
|
Hi @tobe2098, I accidentally deleted my fork repository, which caused this PR to be closed. I still have all the changes locally. Would you like me to create a new PR with the same changes? |
|
Yes, if possible do so please. I will pull them into another branch |
Description
What does this PR do?
This PR fixes the timeout issue when cloud-init uses
power_state: mode: poweroffto shutdown the VM. Insted of waiting for the full timeout period, Multipass now:power_stateis configured.Why is this change needed?
Currently, when a user configures cloud-init to shut down the VM after initialization,
multipass launchwaits for SSH and times out after 300 seconds, even though the VM has completed initialization successfully.This creates a poor user experience:
Implementation Details
power_statefield from user-provided cloud-init YAML during VM creationexpects_shutdownflag inVirtualMachineDescriptionand pass to VMwait_until_ssh_up(), Ifexpects_shutdown = true: throwIntentionalShutdownException(success) and Ifexpects_shutdown = false: throwStartException(failure)IntentionalShutdownExceptionand treats launch as successfulRelated Issue(s)
May be Closes #4456
Testing
Manual testing:
Test 1: Intentional shutdown (with power_state)
Result: Launch completes in ~30-40 seconds with success message, VM in Stopped state
Test 2: Normal VM (no power_state)
Result: Launch completes normally, VM in Running state with SSH available
Test 3: Unexpected crash (VM stopped without power_state)
Manually stop VM during launch:
Result: Launch fails with "VM stopped unexpectedly" error
Screenshots (if applicable)
N/A
Checklist
Additional Notes
N/A