Bug #15983

ASP asks me if I want to persist my already configured ASP

Added by intrigeri 2018-09-28 08:00:09 . Updated 2018-10-31 07:40:09 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Persistence
Target version:
Tails_3.11
Start date:
2018-09-28
Due date:
% Done:

100%

Feature Branch:
bugfix/15838-asp-fix-non-blocking-issues
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Additional Software Packages
Deliverable for:
299

Description

Once logged in and once my ASP have been installed, I get a notification asking me if I want to persist some of them.

======= content of /live/persistence/TailsData_unlocked/live-additional-software.conf =======
backupninja
freeipmi-tools
libdata-dumper-concise-perl
libtemplate-perl
libyaml-tiny-perl
make
rdiff-backup
ssmtp

And then:

Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] Building dependency tree...
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] Reading state information...
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] The following additional packages will be installed:
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   bsd-mailx dialog freeipmi-common libappconfig-perl libfreeipmi16
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libgnutls-openssl27 libipmiconsole2 libipmidetect0 librsync1
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] Suggested packages:
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   debconf-utils duplicity hwinfo mdadm subversion trickle wodim
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   freeipmi-ipmidetect freeipmi-bmc-watchdog libtemplate-perl-doc
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libtemplate-plugin-gd-perl libtemplate-plugin-xml-perl make-doc
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] Recommended packages:
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libdevel-argnames-perl python-pylibacl python-pyxattr
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO] The following NEW packages will be installed:
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   backupninja bsd-mailx dialog freeipmi-common freeipmi-tools
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libappconfig-perl libdata-dumper-concise-perl libfreeipmi16
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libgnutls-openssl27 libipmiconsole2 libipmidetect0 librsync1
Sep 28 07:51:47 amnesia tails-additiona[14961]: [INFO]   libtemplate-perl libyaml-tiny-perl make rdiff-backup ssmtp
[…]
Sep 28 07:52:31 amnesia tails-additiona[7788]: [INFO] Examining package changes
Sep 28 07:52:32 amnesia tails-additiona[7788]: [INFO] New packages manually installed: {'libtemplate-perl', 'freeipmi-tools', 'make', 'backupninja', 'libyaml-tiny-perl', 'ssmtp'}
Sep 28 07:52:32 amnesia sudo[7791]:     root : TTY=unknown ; PWD=/tmp ; USER=amnesia ; COMMAND=/usr/local/lib/tails-additional-software-notify Add backupninja, freeipmi-tools, libtemplate-perl, libyaml-tiny-perl, make and ssmtp to your additional software? To install it automatically from your persistent storage when starting Tails. Install Every Time Install Only Once  urgent

Subtasks

History

#1 Updated by blue9 2018-10-12 03:07:12

This happens to me on every boot. (Note that it’s a subset of packages already in live-additional-software.conf that are listed.)

Something to do with how the lambda on line 473 of `/usr/local/sbin/tails-additional-software` interacts with `additional_packages_names`? Or, anyway, I added `additional_packages_names_set = set(additional_packages_names)` and referenced that set (rather than `additional_packages_names`) on lines 474 and 481. As far as I can tell, it now behaves as expected when I start the systemd service manually and when I add or remove packages using apt.

Also, things fail pretty catastrophically for me if I ever accidentally click “Install Every Time” in this situation. No idea what that’s about.

#2 Updated by alant 2018-10-18 19:46:41

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 30

I implemented the suggested fix, which seems to solve the issue. However, I can’t find a way to deterministically reproduce the issue which seems to happen “sometimes”, so I’m unsure how to decide the bug is actually fixed.

#3 Updated by blue9 2018-10-19 03:14:07

In order to be more memory efficient, the map() function in Python 3 returns an iterator, not a list. (You probably know this. I did not.) As a result, we can iterate through additional_packages_names only once. My best guess is that the …not in… operation of the lambda function supplied to the filter on line 474 tries to iterate through the map object for each package name in packages[“installed”]. The first such package name that’s already in live-additional-software.conf will be correctly filtered out of the resulting new_manually_installed_packages set. Subsequent items will not be filtered out, because additional_packages_names will be empty by the time we go to check them.

This matches my experience in that I can reliably trigger this bug, any time I have more than a single package in live-additional-software.conf, by

  1. Restarting, setting an admin passphrase and unlocking persistence; or
  2. Removing two or more of the packages in live-additional-software.conf using sudo apt remove and then running sudo systemctl start tails-additional-software-install.

To be clear, I have no idea whether or not preemptively converting additional_packages_names to a set is the best fix for this. The above is just meant to help you test whatever fix you come up with.

(Out of curiosity, has anyone tried clicking “Install Every time” when this happens? Things fail hard for me, when I do, and in a way that I cannot explain.)

#4 Updated by blue9 2018-10-19 04:01:31

Er, actually… I guess this could produce inconsistent results. Just like you said. :) Depending on the order in which the filter() function tests the elements in packages[“installed”]. And on the sort order of additional_packages_names? Assuming the reverse membership operator in the lambda function stops iterating when it finds a match, we could end up failing to filter out anywhere between zero and n-1 packages that are already present in live-additional-software.conf.

#5 Updated by alant 2018-10-19 22:45:11

  • Assignee changed from alant to segfault
  • % Done changed from 30 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/15838-asp-fix-non-blocking-issues

blue9 wrote:
> Er, actually… I guess this could produce inconsistent results. Just like you said. :) Depending on the order in which the filter() function tests the elements in packages[“installed”]. And on the sort order of additional_packages_names?

additional_packages_names is a map object on a set (which is a hashlist), so it doesn’t really has an order.

> Assuming the reverse membership operator in the lambda function stops iterating when it finds a match, we could end up failing to filter out anywhere between zero and n-1 packages that are already present in live-additional-software.conf.

I tried to understand the issue for some time yesterday, and I’m not sure of the details. I’m not sure why the map object iterator doesn’t produce consistent results, and I can randomly reproduce the issue in that case. But after converting into a set, I can never reproduce the issue, which is what we want.

#6 Updated by segfault 2018-10-20 12:38:46

  • Assignee changed from segfault to intrigeri
  • % Done changed from 50 to 60
  • QA Check changed from Ready for QA to Pass

alant wrote:
> blue9 wrote:
> > Er, actually… I guess this could produce inconsistent results. Just like you said. :) Depending on the order in which the filter() function tests the elements in packages[“installed”]. And on the sort order of additional_packages_names?
>
> additional_packages_names is a map object on a set (which is a hashlist), so it doesn’t really has an order.

It is not sorted and doesn’t have a deterministic order, and that is the reason why the issue does not always occur.

>
> > Assuming the reverse membership operator in the lambda function stops iterating when it finds a match, we could end up failing to filter out anywhere between zero and n-1 packages that are already present in live-additional-software.conf.
>
> I tried to understand the issue for some time yesterday, and I’m not sure of the details. I’m not sure why the map object iterator doesn’t produce consistent results, and I can randomly reproduce the issue in that case. But after converting into a set, I can never reproduce the issue, which is what we want.

I think blue9 is right. You can only iterate through the iterator once. When evaluating the pkg not in additional_packages_names expression, elements are requested from the iterator until pkg is found. In the case that the iterator and set(packages["installed"]) have the same order, all is good, because each element requested from the iterator does match pkg in the expression. If they don’t have the same order, at some point an element is returned which does not match pkg, which is still removed from the iterator, and later when this pkg is looked up in the iterator, it’s not found, so it ends up in new_manually_installed_packages.

So yeah, I think converting the iterator to a set should fix this, because you can iterate a set more than once.

I pushed commit d72ee99922b53312f18750e34f8c33741e880dcf to the branch, in an attempt to make this code a bit more readable and pythonic.

#7 Updated by intrigeri 2018-10-21 09:16:30

  • % Done changed from 60 to 90

Assuming segfault has tested his last commit: LGTM, merged locally, will batch-push once I’ve tested Bug #16029 in order to avoid overloading Jenkins with builds+tests of stable & devel (I expect we’ll need it for other stuff today).

#8 Updated by intrigeri 2018-10-21 09:45:32

  • Status changed from In Progress to Fix committed
  • Assignee deleted (intrigeri)
  • % Done changed from 90 to 100

#9 Updated by alant 2018-10-22 11:56:33

  • Status changed from Fix committed to In Progress

Applied in changeset commit:fb5fc1c7ac00f372ab4172efe131d47ed9388869.

#10 Updated by alant 2018-10-23 17:55:05

  • Assignee set to alant
  • QA Check changed from Pass to Ready for QA

segfault wrote:
> I pushed commit d72ee99922b53312f18750e34f8c33741e880dcf to the branch, in an attempt to make this code a bit more readable and pythonic.

This commit broke the 80 charachers line length, I pushed a fix (to be tested in Jenkins).

#11 Updated by segfault 2018-10-23 20:00:01

alant wrote:
> segfault wrote:
> > I pushed commit d72ee99922b53312f18750e34f8c33741e880dcf to the branch, in an attempt to make this code a bit more readable and pythonic.
>
> This commit broke the 80 charachers line length, I pushed a fix (to be tested in Jenkins).

Mmh, I don’t think we had this discussion yet, but I usually find code more readable with a line length of 100, which prevents a lot of line wrapping (especially if there are many long variable names, like in the ASP code).

You set it back to RfQA - who is supposed to review this? Me?

#12 Updated by intrigeri 2018-10-23 22:43:47

  • Target version changed from Tails_3.10.1 to Tails_3.11

#13 Updated by alant 2018-10-27 12:53:52

  • Assignee changed from alant to segfault
  • Target version deleted (Tails_3.11)

> >
> > This commit broke the 80 charachers line length, I pushed a fix (to be tested in Jenkins).
>
> Mmh, I don’t think we had this discussion yet, but I usually find code more readable with a line length of 100, which prevents a lot of line wrapping (especially if there are many long variable names, like in the ASP code).
>
We didn’t have the discussion, and I understand your point. But :

  • all ASP code is 80 char long, so unless someone reformats everything lets stay to this for consistency
  • my syntax checker strickly follows PEP8 and doesn’t like lines > 80 chars so it sees errors. I could probably configure it if we decide to change the line length but a few exceptions are highly visible to me

> You set it back to RfQA - who is supposed to review this? Me?

RfQA but assigned to me = trigger a build on jenkins, that I have to check

I reassigned it to you for review if possible.

Thanks

#14 Updated by Anonymous 2018-10-29 16:03:08

hi segfault: do you think you can review this before Wednesday? If not, just let me know. Thanks!

#15 Updated by segfault 2018-10-29 17:52:13

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Ready for QA to Pass

u wrote:
> hi segfault: do you think you can review this before Wednesday? If not, just let me know. Thanks!

Of course.

alant wrote:
> > >
> > > This commit broke the 80 charachers line length, I pushed a fix (to be tested in Jenkins).
> >
> > Mmh, I don’t think we had this discussion yet, but I usually find code more readable with a line length of 100, which prevents a lot of line wrapping (especially if there are many long variable names, like in the ASP code).
> >
> We didn’t have the discussion, and I understand your point. But :
>
> * all ASP code is 80 char long, so unless someone reformats everything lets stay to this for consistency

Makes sense.

> * my syntax checker strickly follows PEP8 and doesn’t like lines > 80 chars so it sees errors. I could probably configure it if we decide to change the line length but a few exceptions are highly visible to me

OK, got it. I’d suggest we be consistent in the ASP code (i.e. 80 chars), but not that strict in other Python code in Tails.

> > You set it back to RfQA - who is supposed to review this? Me?
>
> RfQA but assigned to me = trigger a build on jenkins, that I have to check
>
> I reassigned it to you for review if possible.

Sure, LGTM.

#16 Updated by intrigeri 2018-10-31 07:35:08

  • Target version set to Tails_3.11

#17 Updated by intrigeri 2018-10-31 07:40:09

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)

Merged!