Feature #14720

Fix self.opts.partition vs. self.force_reinstall semantics

Added by anonym 2017-09-25 16:53:29 . Updated 2018-09-05 16:23:22 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Installation
Target version:
Tails_3.9
Start date:
2017-09-25
Due date:
% Done:

100%

Feature Branch:
bugfix/15590-14849-14720-14810-installer-fixes-for-3.9, installer:bugfix/15590-14849-14720-14810-installer-fixes-for-3.9
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Installer
Deliverable for:

Description

This is a follow-up on Feature #8859 and Feature #8860. Details will follow in the comments.

Subtasks

Related issues

Blocked by Tails - Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks Resolved 2017-10-07

History

#1 Updated by anonym 2017-09-25 16:57:12

From intrigeri’s code review (Feature #8860#note-40):

I’m a bit confused by the self.opts.partition vs. self.force_reinstall semantics. I understand we need to treat them differently in gui.py, but I wonder why it has to leak into creator.py. But perhaps I’m just confused because some other areas where force_reinstall is used are unclear to me:

  • I’ve not tested yet but the code suggests that “If there is valid Tails partition”, we’ll display an “Upgrade” button while we’re going to partition the device and thus delete any existing persistent volume. That feels weird to me, but I don’t have the big picture in mind. If that’s what sajolida wants, fine. If that was unspecified, then I don’t know :)
  • We now seem to do “Remove parent drives if a valid partition exists” even for initial installation, while we previously did that only for upgrades. That feels weird at first glance. If this is correct, a comment explaining why would be welcome.

#2 Updated by anonym 2017-09-25 17:08:13

From Feature #8860#note-47:

To me it seems like all this is baggage from when Tails Installer only had one mode, and didn’t switch between them depending on the target device, like now. In fact, I feel the proper fix is to drop self.opts.partition, self.force_reinstall etc. from TailsInstallerWindow and LinuxTailsInstallerCreator, and make these properties of each drive, probably set when they are scanned in detect_supported_drives().

#3 Updated by kurono 2017-10-11 16:22:40

anonym wrote:
> From intrigeri’s code review (Feature #8860#note-40):
>
> I’m a bit confused by the self.opts.partition vs. self.force_reinstall semantics. I understand we need to treat them differently in gui.py, but I wonder why it has to leak into creator.py. But perhaps I’m just confused because some other areas where force_reinstall is used are unclear to me:
>
> * I’ve not tested yet but the code suggests that “If there is valid Tails partition”, we’ll display an “Upgrade” button while we’re going to partition the device and thus delete any existing persistent volume. That feels weird to me, but I don’t have the big picture in mind. If that’s what sajolida wants, fine. If that was unspecified, then I don’t know :)
> * We now seem to do “Remove parent drives if a valid partition exists” even for initial installation, while we previously did that only for upgrades. That feels weird at first glance. If this is correct, a comment explaining why would be welcome.

The general idea is:

  • We have a drop down list with all the available USB sticks, we can use for installing Tails.
  • The selected device comes with all the relevant data for installing or upgrading Tails on it, like size, mount path, etc.
  • If the device has tails, we show in the list the relevant partition, for example /dev/sdc1
  • If not, we show the full device, for example /dev/sdc
  • if the device has tails, we delete the parent, otherwise we would have both /dev/sdc1 and /dev/sdc in the list, which is confusing and prone to errors
  • if the selected device has Tails, lets say /dev/sdc1, we show the upgrade button. If it is pressed, it keeps the partitions unchanged and including the persistent. It just deletes the files and copy the new ones. But in addition to the upgrade button, there is a link called reinstall, which forces to delete the partitions and create new ones (exactly like installing from scratch).
  • So the tricky part, is when the user selects reinstall, because we need to come back to the parent device /dev/sdc, that we have deleted. That is way we rescan the devices and leak force_reinstall to creator.py,
    so we can get the info from the parent.
  • On the other hand elf.opts.partition is an option that comes from the command line and self.force_reinstall is something I have add to seal to this special situation, that we did not have before, because we knew before hand exactly what operation we were going to apply. Now it becomes dinamic, we can switch from install to upgrade in the same device
  • Maybe there is a more elegant solution, to get the parent’s info when a partition “/dev/sdc1” is going to be reinstalled.

#4 Updated by kurono 2017-10-13 16:09:17

  • QA Check set to Ready for QA
  • Feature Branch set to feature/9005-Improve-tails-installer

I have found a better solution for the “leaking force_reinstall” problem.
Now the installer stores the parent information of valid tails partitions,
so if the user selects the reinstall link, we can get that info without having to make more extra weird tricks at creator.py

For the second point, as @anonym says we need to remove the parents for all the devices now, since we are relaying in the previous splash options. So, if we leave that as it was before, it will list valid partitions and their parents in the dropdown list, which is confusing.

#5 Updated by intrigeri 2017-11-06 15:30:14

  • Assignee changed from anonym to intrigeri

#6 Updated by intrigeri 2017-11-08 20:57:27

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

Looks good to me. So code review passes for Feature #14720, Feature #14721, Bug #14722 and Feature #14723, thanks! :)

Next step is to build & upload a Debian package and see what our test suite thinks of these changes. I’ll do that tomorrow unless anonym takes this ticket back in the meantime.

#7 Updated by intrigeri 2017-11-09 14:25:00

  • Feature Branch changed from feature/9005-Improve-tails-installer to kurono:installer/feature/9005-Improve-tails-installer, installer:debian_feature/9005-post-5.0.1-fixes, feature/9005-post-5.0.1-fixes

Uploaded a package, let’s see what Jenkins thinks.

#8 Updated by intrigeri 2017-11-09 19:22:11

  • % Done changed from 50 to 60

Two partial test suite runs passed 100% on Jenkins, but to be on the safe side I’ll do a full run before merging.

#9 Updated by intrigeri 2017-11-10 11:05:48

Full test suite passed except 2 unrelated failures (1 known issue reported a few days ago, 1 probable local network problem).

#10 Updated by intrigeri 2017-11-10 12:00:02

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

I think I’ve found a regression introduced by this branch:

  1. I start Tails built from this branch
  2. I plug in a Tails 3.2 USB stick
  3. I start Tails Installer with DEBUG=1 in a Terminal
  4. I click “Reinstall”
  5. Here’s what I see:
2017-11-10 11:54:54,533 [creator.py:296 (popen)] DEBUG: /sbin/sgdisk --print /dev/sdc1
2017-11-10 11:54:54,555 [gui.py:264 (run)] ERROR: 'NoneType' object has no attribute '__getitem__'
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/tails_installer/gui.py", line 200, in run
    self.live.drives[parent_data['device']] = parent_data
TypeError: 'NoneType' object has no attribute '__getitem__'
2017-11-10 11:54:54,557 [gui.py:265 (run)] DEBUG: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/tails_installer/gui.py", line 200, in run
    self.live.drives[parent_data['device']] = parent_data
TypeError: 'NoneType' object has no attribute '__getitem__'

anonym says “actually, the way I’ll resolve the conflict for that code, this bug should be fixed” so I’ll let him merge the 3 topic branches into his, make sure it is fixed, and then reassign this + Bug #14755 to me for a final round of QA.

#11 Updated by anonym 2017-11-10 14:45:34

intrigeri wrote:
> anonym says “actually, the way I’ll resolve the conflict for that code, this bug should be fixed” so I’ll let him merge the 3 topic branches into his, make sure it is fixed, and then reassign this + Bug #14755 to me for a final round of QA.

I failed to combine commit 51b15fe237636f845c2230023a3eba393bfbf0c4 and my fix for Bug #14755, so my plan is to merge kurono’s feature/9005-Improve-tails-installer without that commit (its HEAD), postponing this ticket, but closing Feature #14721, Bug #14722, Feature #14723.

#12 Updated by intrigeri 2017-11-11 09:38:20

  • Target version changed from Tails_3.3 to Tails_3.5

Postponing, then. I’ll let you two decide who’s going to work on this during next cycle.

#13 Updated by kurono 2017-11-11 10:24:59

  • Assignee changed from anonym to kurono

intrigeri wrote:
> Postponing, then. I’ll let you two decide who’s going to work on this during next cycle.

I did not have enough time today to review this but I will take care for the next cycle.

#14 Updated by intrigeri 2017-11-11 11:09:33

> I did not have enough time today to review this but I will take care for the next cycle.

Well, for now there’s nothing to review: anonym didn’t manage to integrate this change with his fix for Bug #14755.

#15 Updated by Anonymous 2018-01-15 16:02:07

anonym & kurono: apparently you need to decide who of you two will prepare the branch so that it’s mergeable, see https://labs.riseup.net/code/issues/14720#note-11 and/or anonym could merge this without the problematic commit.

#16 Updated by kurono 2018-01-15 18:06:55

  • Target version changed from Tails_3.5 to Tails_3.6

#17 Updated by bertagaz 2018-03-14 11:32:07

  • Target version changed from Tails_3.6 to Tails_3.7

#18 Updated by kurono 2018-03-17 13:00:04

  • related to Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#19 Updated by kurono 2018-03-17 13:00:27

  • related to Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#20 Updated by kurono 2018-03-17 13:00:33

  • related to deleted (Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks)

#21 Updated by bertagaz 2018-05-10 11:09:02

  • Target version changed from Tails_3.7 to Tails_3.8

#22 Updated by kurono 2018-06-15 21:07:56

  • Assignee changed from kurono to anonym
  • QA Check changed from Dev Needed to Ready for QA

u wrote:
> anonym & kurono: apparently you need to decide who of you two will prepare the branch so that it’s mergeable, see https://labs.riseup.net/code/issues/14720#note-11 and/or anonym could merge this without the problematic commit.

Finally I managed to clean this mess a bit. Take a look of the branch: feature/9005-Improve-tails-installer
It passed all the the test scenarios in my local machine.

#23 Updated by intrigeri 2018-06-16 06:15:15

  • Assignee changed from anonym to kurono

> Finally I managed to clean this mess a bit. Take a look of the branch: feature/9005-Improve-tails-installer
> It passed all the the test scenarios in my local machine.

Thanks, I’ll look into it! Depending on the nature of the changes this might be suitable for 3.8 or 3.9, we’ll see.

#24 Updated by kurono 2018-06-20 16:56:53

  • Assignee changed from kurono to intrigeri

#25 Updated by intrigeri 2018-06-22 21:48:55

  • Target version changed from Tails_3.8 to Tails_3.9

#26 Updated by intrigeri 2018-06-28 16:28:38

  • Assignee changed from intrigeri to kurono
  • QA Check changed from Ready for QA to Info Needed

Please rebase your work on top of the current upstream master branch, as opposed to the packaging (tails/master) branch.

#27 Updated by kurono 2018-06-29 00:25:37

  • Assignee changed from kurono to intrigeri
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> Please rebase your work on top of the current upstream master branch, as opposed to the packaging (tails/master) branch.

Done.

#28 Updated by intrigeri 2018-06-29 15:52:27

Code looks good to me. Depending on the 2 other reviews I still have to do, I may test this alone or together with other proposed branches. I’ll be back here shortly.

#29 Updated by intrigeri 2018-06-29 15:58:47

I’ll test in a batch with Bug #15590 and Bug #14810 once the latter is ready.

#30 Updated by intrigeri 2018-06-29 16:00:14

  • related to deleted (Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks)

#31 Updated by intrigeri 2018-06-29 16:00:20

  • blocked by Bug #14810: Tails Installer allows buggy "Reinstall (delete all data)" on too small USB sticks added

#32 Updated by intrigeri 2018-07-10 12:34:27

  • Feature Branch changed from kurono:installer/feature/9005-Improve-tails-installer, installer:debian_feature/9005-post-5.0.1-fixes, feature/9005-post-5.0.1-fixes to kurono:installer/feature/9005-Improve-tails-installer

#33 Updated by intrigeri 2018-07-10 13:06:03

Merged into bugfix/15590-14849-14720-14810-installer-fixes-for-3.9 (both liveusb-creator.git and tails.git) except the logo change (quilt won’t take binary stuff), built and uploaded a package.

#34 Updated by intrigeri 2018-07-11 08:38:11

Test suite passes on Jenkins. Next step: test manually to confirm the bugs the branch is meant to fix are gone and that nothing else is broken.

#35 Updated by intrigeri 2018-07-11 19:32:13

  • % Done changed from 60 to 90
  • QA Check changed from Ready for QA to Pass
  • Feature Branch changed from kurono:installer/feature/9005-Improve-tails-installer to bugfix/15590-14849-14720-14810-installer-fixes-for-3.9, installer:bugfix/15590-14849-14720-14810-installer-fixes-for-3.9

#36 Updated by intrigeri 2018-07-11 19:46:23

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

Merged into devel!

#37 Updated by intrigeri 2018-09-05 16:23:23

  • Status changed from Fix committed to Resolved