Add `--force` argument to the `stop` command #1946

luis4a0 · 2021-01-28T19:51:22Z

Add a new parameter to force the shutdown instances (i.e., power down them).

Fixes #1909, Fixes #2492

townsend2010 · 2021-01-28T20:10:31Z

I know this is still in draft, but I always envisioned just adding a bool force flag to shutdown() and stop() (it also can be argued that either shutdown() or stop() can be dropped altogether, but that's for a different day). That way, you are not having to duplicate any special handling or logic that you are currently doing if some of the force_shutdown() implementations.

codecov · 2021-01-28T20:13:13Z

Codecov Report

Attention: Patch coverage is 91.79104% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.90%. Comparing base (812f076) to head (f503968).

Files	Patch %	Lines
src/daemon/daemon.cpp	8.33%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1946      +/-   ##
==========================================
+ Coverage   88.81%   88.90%   +0.08%     
==========================================
  Files         253      254       +1     
  Lines       14121    14170      +49     
==========================================
+ Hits        12542    12598      +56     
+ Misses       1579     1572       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

luis4a0 · 2021-01-29T13:16:35Z

Hey @townsend2010, thanks a lot for your comment! What you propose makes complete sense, I'll finish testing if the commands I used indeed work to turn off VM's and then rework the code.

sharder996 · 2022-08-19T15:58:23Z

Added a few small changes; mostly refactoring with the current state of main and some wiring that got missed.

Fixes #2492

and

#2693 can be closed as its no longer needed

ricab · 2023-01-30T18:32:04Z

Once this is in, I think we should also change delete such that running instances get force-stopped if a regular stop is not enough. I don't think it makes a lot of sense for deleted instances to keep running...

ricab

Hey @townsend2010, thanks for the PR! Not a proper review, but I was looking in order to base my work on top, so leaving a couple of comments.

src/platform/backends/qemu/qemu_virtual_machine.cpp

src/client/cli/cmd/stop.cpp

src/daemon/daemon.cpp

This will make it explicit that the user wants to force stop an instance.

Change "Forced" to "Forcing" Co-authored-by: Ricardo Abreu <6698114+ricab@users.noreply.github.com> Signed-off-by: Chris Townsend <christopher.townsend@canonical.com>

If the instance is suspended when a force shutdown is issued, then delete the suspend image.

This will account for the case if an instance is starting and getting stuck when resuming from a suspend image.

This is fit in with the style used in other implementations.

andrei-toterman

Okay, the code looks pretty good IMO. It went through many iterations already after all.
In terms of functionality:

The main issue is QEMU deadlocking.
The other smaller issue is libvirt not removing the suspended state on force stop. But given that we're talking more and more about removing libvirt, I'm not sure if it's worth spending too much time on this issue.

andrei-toterman · 2024-06-14T13:22:21Z

src/platform/backends/shared/base_virtual_machine.cpp

+mp::BaseVirtualMachine::BaseVirtualMachine(VirtualMachine::State state,
+                                           const std::string& vm_name,
+                                           const SSHKeyProvider& key_provider,
+                                           const Path& instance_dir)


I would like an opinion on this change of removing the enclosing multipass namespace and adding mp:: to all the enclosed functions. IMO it only adds more width for no reason at all. Is it preferable?

I think this is largely a matter of preference. I personally prefer the fully qualified name in .cpps. Compilation units are usually long so it is easier understand functions' scope regardless of their position in the file. And when something is already declared in the header, I feel it is more intuitive to just refer to it with its full "path". But I also prefer indenting namespace contents (which we don't do), in which case this approach would tend to save space. In any case, consistency has its value too (it makes things more immediately recognizable) and I believe this is the de facto convention in Multipass.

andrei-toterman · 2024-06-14T14:27:49Z

src/platform/backends/qemu/qemu_virtual_machine.cpp

+        if (state != State::off)
+        {
+            state = State::off;
+        }


How is this different than just state = State::off?

andrei-toterman · 2024-06-20T13:28:07Z

src/platform/backends/qemu/qemu_virtual_machine.cpp

+            lock.unlock();
+            vm_process->kill();
+            lock.lock();
+            state_wait.wait(lock, [this] { return vm_process == nullptr; });


I think this leads to a deadlock if the callback for the signal emitted by kill() does no execute before the lock().
Adding a vm_process->wait_for_finished() before the lock.lock() seems to solve the issue, but I'm not sure if that's a good solution or just a quick fix. Maybe some restructuring of the code could avoid this problem entirely.

andrei-toterman · 2024-06-21T11:17:33Z

src/platform/backends/libvirt/libvirt_virtual_machine.cpp

libvirt does not remove suspended state on force stop

luis4a0 force-pushed the add-force-stop branch from 1f21eac to 3a7b09d Compare January 29, 2021 15:15

Base automatically changed from master to main March 3, 2021 13:41

luis4a0 force-pushed the add-force-stop branch from 3a7b09d to fecc2be Compare March 3, 2021 17:07

luis4a0 force-pushed the add-force-stop branch 2 times, most recently from 36d6cec to a4f04ff Compare May 26, 2021 14:30

luis4a0 force-pushed the add-force-stop branch from a4f04ff to b572ebe Compare May 28, 2021 18:59

luis4a0 force-pushed the add-force-stop branch from b572ebe to b10bc1a Compare July 1, 2022 15:11

luis4a0 force-pushed the add-force-stop branch from b10bc1a to 69ebeca Compare August 16, 2022 14:31

sharder996 marked this pull request as ready for review August 19, 2022 15:56

sharder996 mentioned this pull request Jan 30, 2023

delete should force-stop running instances when a regular stop isn't enough #2906

Open

ricab mentioned this pull request Jan 12, 2024

Unable to stop/shutdown primary instance #3370

Closed

townsend2010 force-pushed the add-force-stop branch from 65903a3 to 708bd3a Compare March 21, 2024 20:16

townsend2010 force-pushed the add-force-stop branch 7 times, most recently from 41f273e to b1a5b88 Compare April 5, 2024 20:27

ricab reviewed Apr 11, 2024

View reviewed changes

src/platform/backends/qemu/qemu_virtual_machine.cpp Outdated Show resolved Hide resolved

src/client/cli/cmd/stop.cpp Outdated Show resolved Hide resolved

townsend2010 force-pushed the add-force-stop branch from 4be3838 to 8ac8d93 Compare April 12, 2024 18:01

ricab reviewed Apr 15, 2024

View reviewed changes

src/daemon/daemon.cpp Show resolved Hide resolved

townsend2010 force-pushed the add-force-stop branch from 8ac8d93 to 45dcacd Compare April 15, 2024 20:22

townsend2010 linked an issue Apr 24, 2024 that may be closed by this pull request

[daemon] need to kill misbehaving instances on stop #1167

Open

townsend2010 force-pushed the add-force-stop branch from 45dcacd to 1861b89 Compare May 2, 2024 20:50

luis4a0 and others added 28 commits June 20, 2024 13:02

[lxd] Implement forced stop.

4cf351f

[libvirt] Implement forced stop.

3b3ae6d

refactor with main

6fba9c8

[cli/stop] Make force a long option only

981cb0a

This will make it explicit that the user wants to force stop an instance.

[virtual_machine] Make force shutdown bool const

fed86c3

[tests] Fix QEMU test for setting bool for shutdown()

15243e4

[tests/cli] Test the --force option for stop command

73258e6

[tests/qemu] Add tests for the force shutdown handling

1103476

Update src/platform/backends/qemu/qemu_virtual_machine.cpp

3cd2313

Change "Forced" to "Forcing" Co-authored-by: Ricardo Abreu <6698114+ricab@users.noreply.github.com> Signed-off-by: Chris Townsend <christopher.townsend@canonical.com>

[tests] Fix tests due to change in log string

d0c7570

[cli/stop] Be more specific with force stop help

d127526

[utils/qemu_img] Add general function for deleting a snapshot image

427edae

[backend/qemu] Delete suspend image on force shutdown

42281b2

If the instance is suspended when a force shutdown is issued, then delete the suspend image.

[tests] Add qemu test for deleting suspend on force shutdown

eee613f

[qemu] Check if there is a suspend image in force shutdown

199f576

This will account for the case if an instance is starting and getting stuck when resuming from a suspend image.

[backend/qemu] Redo the shutdown logic

45ab86e

[qemu] Redo some logic in how force shutdown is handled

c607af2

[vm] Don't enclose BaseVirtualMachine impl in multipass scope

a8aa8d0

This is fit in with the style used in other implementations.

[exceptions] Add new exception types for VirtualMachine states

f51c84b

[virtual_machine] Add method in base to check for states when stopping

66f0948

[backend/qemu] Add base VM state check handling for shutdown

8396f54

[tests] Fix/modify QEMU backend tests

1180b71

[qemu] Redo locking logic a bit to be more safe

aee9292

[libvirt] Redo shutdown logic based on new base method

35699ea

[tests] Fix/modify libvirt tests

299eb81

[tests] Add tests for libvirt shutdown

c2c0781

[lxd] Add call to base class for checkoing shutdown states

475015f

[tests] Adapt LXD backend tests for changes in shutdown

f503968

georgeliao force-pushed the add-force-stop branch from ece9a6c to f503968 Compare June 20, 2024 11:07

andrei-toterman requested changes Jun 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--force` argument to the `stop` command #1946

Add `--force` argument to the `stop` command #1946

luis4a0 commented Jan 28, 2021 •

edited by townsend2010

townsend2010 commented Jan 28, 2021

codecov bot commented Jan 28, 2021 •

edited

luis4a0 commented Jan 29, 2021

sharder996 commented Aug 19, 2022

ricab commented Jan 30, 2023 •

edited

ricab left a comment

andrei-toterman left a comment

andrei-toterman Jun 14, 2024

ricab Jun 21, 2024 •

edited

andrei-toterman Jun 14, 2024

andrei-toterman Jun 20, 2024

andrei-toterman Jun 21, 2024

Add --force argument to the stop command #1946

Are you sure you want to change the base?

Add --force argument to the stop command #1946

Conversation

luis4a0 commented Jan 28, 2021 • edited by townsend2010

townsend2010 commented Jan 28, 2021

codecov bot commented Jan 28, 2021 • edited

Codecov Report

luis4a0 commented Jan 29, 2021

sharder996 commented Aug 19, 2022

ricab commented Jan 30, 2023 • edited

ricab left a comment

Choose a reason for hiding this comment

andrei-toterman left a comment

Choose a reason for hiding this comment

andrei-toterman Jun 14, 2024

Choose a reason for hiding this comment

ricab Jun 21, 2024 • edited

Choose a reason for hiding this comment

andrei-toterman Jun 14, 2024

Choose a reason for hiding this comment

andrei-toterman Jun 20, 2024

Choose a reason for hiding this comment

andrei-toterman Jun 21, 2024

Choose a reason for hiding this comment

Add `--force` argument to the `stop` command #1946

Add `--force` argument to the `stop` command #1946

luis4a0 commented Jan 28, 2021 •

edited by townsend2010

codecov bot commented Jan 28, 2021 •

edited

ricab commented Jan 30, 2023 •

edited

ricab Jun 21, 2024 •

edited