Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.55%. Comparing base (392ba43) to head (9f477bb).
⚠️ Report is 3410 commits behind head on main.

@@            Coverage Diff             @@
##             main    #4007      +/-   ##
==========================================
+ Coverage   89.52%   89.55%   +0.03%     
==========================================
  Files         260      260              
  Lines       14749    14757       +8     
==========================================
+ Hits        13204    13216      +12     
+ Misses       1545     1541       -4     

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

Hey @xmkg, is this still valid and is it ready for review?

Hey @xmkg, is this still valid and is it ready for review?

Hey @ricab, yes it's still valid but I need to take a look at the private side of the things. Feel free to review this one.

The MP #4010 has to be merged before this one.

Hi @xmkg
Thanks for the work. Basically, the change is about adding on_restart(name) call to e_state::running and e_state::starting cases. The unit tests also nicely covered different branches. However, I have a wondering about the real scenario for the new branch. The virtual machine class destructor always suspend the vm if it is running. So upon the server restart, the original branch is called if the original vm state is running. On the other side, if the original vm state is stopped or suspended, the daemon start should restore them to stop or suspended state. After that, a multipass start <vm_name> is needed to get them running, and the Daemon::startfunction call probably contains the steps in the on_restart. Therefore, I am just wondering in which scenario the new branch is needed.

However, I have a wondering about the real scenario for the new branch. The virtual machine class always suspend the vm if it is running.

The scenario in which this is needed is a daemon crash, where things are not nicely being wrapped up as they would in a graceful exit. The current implementation of the "on_restart" function initializes the daemon-side resources needed by a VM to function properly, e.g., a mount's daemon-side counterpart, as in SSHFS.

So, assume that we have an SSHFS-mounted VM in a running state, and the daemon crashes. When the daemon is restarted, the spec's state would be running, and the VM's state might be running or something else, depending on the backend. For LXC, the VM's state will be running since the VM process is not a subprocess of the daemon itself and it's externally managed. So for the LXC scenario, spec.state == running and vm.state == running, which will in turn cause the on_restart function not to be called, and the resources associated with the invocation of the on_restart won't be initialized -- in our scenario, that would mean the SSHFS server is not being spawned for the VM, therefore the mount won't work.

The patch fixes that by triggering the on_restart function for the vm.state == running and vm.state == starting. These two states won't require a vm->start() call, so it's being handled separately.

Thanks for the explanation. The code change does help in this corner case. Actually, this particular corner case further reminded us the necessity of the encapsulated vm which is the refactor we really need to do.
Apart from that, can we make a small change to the mock type of MockPlatform (test_daemon.cpp line 137) to NiceMock so we can get rid of the warnings from the three newly added unit tests.

We also need to do a rebase before we can start the merge.

Apart from, can we make a small change to the mock type of MockPlatform (test_daemon.cpp line 137) to NiceMock so we can get rid of the warnings from the three newly added unit tests.

Done, should be good to go.