Snapshot and migrate functionality.

[bjwrigh]

-29ea35c: Fixed some log levels causing unnecessary noise when using Phenix
-379c4bf: Snapshot now only does snapshot. Migrate does migrate (and snapshot, because they are often linked). Will also take care of multiple disks automatically. [Note: vm snapshot previously paused VM because of calling migrate. This is no longer the case. vm migrate still pauses the VM for the operation, but does not restart the VM]

[jacdavi]

Overall looks good. Tested that everything functioned as expected and it did. Just two things

1. Could you run scripts/all.bash I saw a missing curly brace and some formatting errors that should be fixed
2. Is this code block still relevant for snapshots? If not can we remove it and the associated part of the help output? https://github.com/sandia-minimega/minimega/pull/1540/files#diff-3e02ecaf1183b3cbe8d2924d7ff4d9bf7e0b0763d897cd7f98b65b02994fdf9dR798-R818

[glattercj]

This will break the ns snapshot command. I recommend adding updates to support it before merging this. @jacdavi your question about the relevant code block is related to this command. PR where this was all added here.

[jacdavi]

@glattercj thanks for the heads up!

If I'm tracking everything correctly, it seems that ns snapshot should be renamed or at least call vm migrate. The main purpose of this PR is to flip the meaning of snapshot and migrate to match QEMU's naming (snapshot makes a new disk backed by your current disk, while migration is for state/memory).

[glattercj]

Maybe keep each command atomic without hidden side effects?

• vm snapshot = only disk
• vm migrate = only memory state
• ns save calls each of the above to create a saved namespace 'snapshot' (replaces ns snapshot)

[jacdavi]

Based on code comments, it seems unlikely that anyone would want to save just the memory state, since it's probably not bootable without the disk at that point as well. I think that's why @bjwrigh kept them coupled.

[glattercj]

In that case why not just combine the 2 and have:

• vm save (what this PR is proposing as 'vm migrate')
• ns save (calls ^)

I added vm snapshot specifically for ns snapshot, so if migrate is doing both, just have them both named "save" and remove the vm snapshot command entirely. I'm mostly arguing for sane naming since "save" is sort of self-descriptive of what the command is doing for normal users, where "migrate" doesn't mean much unless you really know qemu and dig into the commit history to find this PR and find the reasoning. But at the end of the day I'll defer to you :)

[jacdavi]

I appreciate the input. Given how long it's taken me to wrap my head around all the disk commands in minimega/phenix, I understand how important naming is.

I agree that migrate really doesn't give a good sense of what's happening since we're not using the function to move a vm across hardware. save seems more appropriate, so I'm good to switch to that

[sdelliot]

I am interested in taking over this PR and will be submitting one that addresses the main requested changes. However, I was wondering how far down the refactor rabbit-hole should I go? It sounds like the changes needed are:

• Remove "vm snapshot"
• Rename "vm migrate" to "vm save"
• Rename "ns snapshot" to "ns save"

This will also need modifications to:

• "vm configure migration" -> "vm configure state"

I think that is the minimum changes needed. However, there are a lot of references to both "migrate" and "snapshot". It feels reasonable to rename these function calls/debug statements/variables to use the new syntax. While this will make the new PR much more complicated to review/test, it will also improve maintainability in the future. Do you have opinions on this? It could be split into two PRs as well, if desired.

[jacdavi]

I am interested in taking over this PR and will be submitting one that addresses the main requested changes. However, I was wondering how far down the refactor rabbit-hole should I go? It sounds like the changes needed are:

• Remove "vm snapshot"
• Rename "vm migrate" to "vm save"
• Rename "ns snapshot" to "ns save"

This will also need modifications to:

• "vm configure migration" -> "vm configure state"

I think that is the minimum changes needed. However, there are a lot of references to both "migrate" and "snapshot". It feels reasonable to rename these function calls/debug statements/variables to use the new syntax. While this will make the new PR much more complicated to review/test, it will also improve maintainability in the future. Do you have opinions on this? It could be split into two PRs as well, if desired.

That all sounds correct to me, and thanks for picking this up! If you're willing to make those extra updates, I'd be good with everything in 1 PR.

[jacdavi]

closing in favor of #1555 which builds on this PR