Forgetting to stop TemplateVM after making changes

[Matt McCutchen]

I've lost count of the number of times I've done the following:

1. Work in AppVM and realize I need to make a change to TemplateVM.
2. Start TemplateVM and make change.
3. Restart AppVM.
4. Change is not visible in AppVM.
5. Realize I forgot to stop TemplateVM. Stop it and restart AppVM
again.

It might help to add a prompt to the Qubes manager, when starting an
AppVM, to stop its TemplateVM first. Another idea would be to show a
dimmed version of the current "outdated" icon on AppVMs that are up to
date with the committed version of their TemplateVM, but for which the
TemplateVM is currently running. Maintainers, are you interested in
either change? Otherwise I may not bother to implement anything just
for myself.

Matt

[Marek Marczykowski-Górecki]

I'd love to accept such patch. I'm slightly for a dimmed icon version, as it
is less intrusive.

--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[Matt McCutchen]

On Tue, 2015-02-03 at 15:25 +0100, Marek Marczykowski-Górecki wrote:
> On Mon, Feb 02, 2015 at 11:25:13PM -0500, Matt McCutchen wrote:
> > I've lost count of the number of times I've done the following:
> >
> > 1. Work in AppVM and realize I need to make a change to TemplateVM.
> > 2. Start TemplateVM and make change.
> > 3. Restart AppVM.
> > 4. Change is not visible in AppVM.
> > 5. Realize I forgot to stop TemplateVM. Stop it and restart AppVM
> > again.
> >
> > It might help to add a prompt to the Qubes manager, when starting an
> > AppVM, to stop its TemplateVM first. Another idea would be to show a
> > dimmed version of the current "outdated" icon on AppVMs that are up to
> > date with the committed version of their TemplateVM, but for which the
> > TemplateVM is currently running. Maintainers, are you interested in
> > either change? Otherwise I may not bother to implement anything just
> > for myself.
>
> I'd love to accept such patch. I'm slightly for a dimmed icon version, as it
> is less intrusive.

Here it is! If anyone has an idea for an icon that would better convey
the information and the skills to draw it, I invite them to do so.

Matt

[Matt McCutchen]

Updated patch including the binary content of the icon. Oops. Or if
it's easier, the icon file is attached.

Matt

[Matt McCutchen]

Apparently my testing was very poor, and the method by which I tried to
determine whether the TemplateVM has uncommitted changes does not
actually work. I checked whether root-cow.img has a positive number of
allocated blocks, but the snapshot header gets allocated when a
dependent AppVM starts.

A few options to proceed:

1. Show the "to-be-outdated" icon on a VM only when the TemplateVM is
running and live with the original problem I saw in which it disappears
for a few seconds between TemplateVM shutdown and commit.

2. Do not allow a running AppVM to transition from "to-be-outdated"
state to blank state, but keep it in "to-be-outdated" until it enters
"outdated". Arguably a hack.

3. Find another way to tell whether the TemplateVM has uncommitted
changes, or add code to QubesTemplateVm.start and qvm-template-commit to
keep track of this somewhere. This would give the correct answer in the
event of a system failure that results in a TemplateVM stopping without
committing, which could be seen as an advantage, though the manager
currently doesn't give the user much help recovering from such a state.
It might be enough to add an icon to a TemplateVM in such a state with a
tooltip advising the user to start and stop it to commit changes, if I
knew how to make a suitable icon.

Marek, what's your preference? If it is #3, what mechanism would you
suggest?

I regret the large number of emails.

Matt

[Marek Marczykowski-Górecki]

I think #1 is just ok. Eventually we will improve is_outdated
implementation and then Qubes Manager will simply work.

[Matt McCutchen]

OK, here's the revised patch for qubes-manager. Patching core-admin is
no longer necessary.

Matt

[Marek Marczykowski-Górecki]

Some comments inline in the patch.

> Matt
>
> From Mon Sep 17 00:00:00 2001
> From: Matt McCutchen <ma...@mattmccutchen.net>
> Date: Sun, 8 Feb 2015 20:21:43 -0500
>
> Show an icon for VMs whose TemplateVM is running.
>
> https://groups.google.com/d/topic/qubes-users/woHD9RaHvF8/discussion
> ---
> icons/to-be-outdated.png | Bin 0 -> 2468 bytes
> qubesmanager/table_widgets.py | 31 ++++++++++++++++++++++---------
> resources.qrc | 1 +
> 3 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/icons/to-be-outdated.png b/icons/to-be-outdated.png
> new file mode 100644
> index 0000000000000000000000000000000000000000..42634dd35d378f38ae76018b4213cb27c130b912
> GIT binary patch

(...)

> diff --git a/qubesmanager/table_widgets.py b/qubesmanager/table_widgets.py
> index 258dd3a..5fe317b 100644
> --- a/qubesmanager/table_widgets.py
> +++ b/qubesmanager/table_widgets.py
> @@ -459,7 +459,7 @@ class VmUpdateInfoWidget(QWidget):
> self.set_value(value)
>
> def set_value(self, value):
> - if value == "outdated":
> + if value in ("outdated", "to-be-outdated"):
> self.value = 30
> elif value == "update":
> self.value = 20
> @@ -484,7 +484,7 @@ class VmUpdateInfoWidget(QWidget):
> layout.addWidget(self.icon, alignment=Qt.AlignCenter)
> self.setLayout(layout)
>
> - self.previous_outdated = False
> + self.previous_outdated_state = ""

Why "" here, while the other place uses None?

> self.previous_update_recommended = None
> self.value = None
> self.tableItem = VmUpdateInfoWidget.VmUpdateInfoItem(self.value, vm)
> @@ -493,13 +493,22 @@ class VmUpdateInfoWidget(QWidget):
> if vm.type == "HVM":
> return
>
> - outdated = vm.is_outdated()
> - if outdated and not self.previous_outdated:
> - self.update_status_widget("outdated")
> - elif not outdated and self.previous_outdated:
> - self.update_status_widget(None)
> + if vm.is_outdated():
> + outdated_state = "outdated"
> + # During TemplateVM shutdown, there's an interval of a few seconds
> + # during which vm.template.is_running() returns false but
> + # vm.is_outdated() does not yet return true, so the icon disappears.
> + # This looks goofy, but we've decided not to fix it at this time
> + # (2015-02-09).
> + elif vm.template and vm.template.is_running():
> + outdated_state = "to-be-outdated"
> + else:
> + outdated_state = None

Here is that "other place".

> +
> + if outdated_state != self.previous_outdated_state:
> + self.update_status_widget(outdated_state)
>
> - self.previous_outdated = outdated
> + self.previous_outdated_state = outdated_state
>
> if not vm.is_updateable():
> return
> @@ -551,7 +560,11 @@ class VmUpdateInfoWidget(QWidget):
> elif state == "outdated":
> label_text = "<font color=\"red\">VM outdated</font>"
> icon_path = ":/outdated.png"
> - tooltip_text = "The VM must be restarted for its filesystem to reflect the template's recent changes."
> + tooltip_text = "The VM must be restarted for its filesystem to reflect the template's recent committed changes."
> + elif state == "to-be-outdated":
> + label_text = "<font color=\"#800000\">TemplateVM running</font>"
> + icon_path = ":/to-be-outdated.png"
> + tooltip_text = "The TemplateVM must be stopped before changes from its current session can be picked up by this VM."
> elif state == None:
> label_text = ""
> icon_path = None
> diff --git a/resources.qrc b/resources.qrc
> index 664f506..c9f9172 100644
> --- a/resources.qrc
> +++ b/resources.qrc
> @@ -8,6 +8,7 @@
> <file alias="global-settings.png">icons/global-settings.png</file>
> <file alias="off.png">icons/on-icon/off.png</file>
> <file alias="outdated.png">icons/outdated.png</file>
> + <file alias="to-be-outdated.png">icons/to-be-outdated.png</file>
> <file alias="update-recommended.png">icons/update-recommended.png</file>
> <file alias="show-all-running.png">icons/show-all-running.png</file>
> <file alias="mount.png">icons/mount.png</file>

[Matt McCutchen]

On Mon, 2015-02-09 at 17:48 +0100, Marek Marczykowski-Górecki wrote:
> > diff --git a/qubesmanager/table_widgets.py b/qubesmanager/table_widgets.py
> > index 258dd3a..5fe317b 100644
> > --- a/qubesmanager/table_widgets.py
> > +++ b/qubesmanager/table_widgets.py
> > @@ -459,7 +459,7 @@ class VmUpdateInfoWidget(QWidget):
> > self.set_value(value)
> >
> > def set_value(self, value):
> > - if value == "outdated":
> > + if value in ("outdated", "to-be-outdated"):
> > self.value = 30
> > elif value == "update":
> > self.value = 20
> > @@ -484,7 +484,7 @@ class VmUpdateInfoWidget(QWidget):
> > layout.addWidget(self.icon, alignment=Qt.AlignCenter)
> > self.setLayout(layout)
> >
> > - self.previous_outdated = False
> > + self.previous_outdated_state = ""
>
> Why "" here, while the other place uses None?

Just a mistake (though it didn't appear to have any effect), good catch.
The corrected patch is attached.

Matt

[Marek Marczykowski-Górecki]

Thanks, applied to master branch.