Feature #17152

Port tails-iuk to a more lightweight set of dependencies

Added by intrigeri 2019-10-14 07:58:34 . Updated 2019-11-27 08:37:05 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Target version:
Tails_4.2
Start date:
Due date:
% Done:

100%

Feature Branch:
feature/17152-port-to-moo, iuk:feature/17152-port-to-moo, perl5lib:feature/17152-port-to-moo
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Upgrader
Deliverable for:

Description

tails-iuk and a couple of tails-perl5lib Perl modules it uses are the last pieces of Tails code we have that are implemented using Moose + Method::Signatures::Simple + Path::Class. Everything else was ported to a more lightweight stack (Moo + Function::Parameters + Path::Tiny) already.

Porting this remaining code to the stack we’re using in the rest of our codebase has a number of advantages:

  • less RAM consumption at runtime: I’ve measured 30MB saving just for the frontend process; presumably every child process sees similar gains; at most stages of the upgrade we run 2 such processes at any given time, so this translates to 50+ MB savings.
  • shorter feedback loop while working on this codebase: I’ve measured 25-45% shorter test suite runs in iuk.git, keeping in mind that a lot of the time spent that’s left is spent on the test suite machinery (generating SquashFS, tarballs, etc.) and not in the code paths that our users run
  • more consistent codebase, so I don’t have to wonder how subtle differences between Moo and Moose impact the Perl code I’m modifying
  • slightly smaller ISO and USB images: Moose and friends pull a lot of dependencies in our images; in 4.0~rc1, APT tells me that recursively deinstalling this set of dependencies saves 8.2 MB (uncompressed); granted, this probably does not translate to significant savings once SquashFS’ed but every byte counts
  • tighter scope for my volunteer work in the Debian Perl group: I can focus on the Moo ecosystem instead of Moo + Moose

That’s something I’ve been wanting to do for 5 years (Feature #6876#note-1) and given I’ll soon have to work quite a bit on this codebase for Feature #15281, I figured I would spend a rainy Sunday implementing it on my copious volunteer free time ().

Subtasks

Related issues

Blocks Tails - Feature #9373: Make tails-iuk support overlayfs Resolved

History

#1 Updated by intrigeri 2019-10-14 08:00:23

  • Feature Branch set to feature/17152-port-to-moo, iuk:feature/17152-port-to-moo, perl5lib:feature/17152-port-to-moo

#2 Updated by intrigeri 2019-10-14 08:38:13

So, the diff is huge, it would have been much nicer to see this go in 4.0~betaN, and we lack reviewers for our Perl code, but:

  • It’s mostly about mechanical changes.
  • tails-iuk has its own, pretty exhaustive, test suite.
  • We have a test scenario for automatic upgrades in the Tails test suite.
  • As usual for Upgrader changes, shipping them in the RC does not help much: we’ll only notice regressions when folks use the new code to upgrade to the final release, and by then it’s too late to fix them in the final release, so the only safe way to ship such changes is by having several beta/RC’s and hope folks actually use them and upgrade using automatic upgrades.
  • I did some manual tests in scenarios as close as possible to real-world ones (see below).
  • I’d live to have this merged before I start working on the Upgrader for Feature #15281, so the diff for that upcoming work is easier to review.

⇒ I’ll propose this for 4.0 once I’ve seen the corresponding test scenario pass on my local Jenkins.

I’ve tested the upgrade process (and the check for upgrades when there’s none) like this:

  1. start 4.0~beta2 from a virtual USB drive
  2. login in French (to exercise the l10n code)
  3. wait until I’m proposed to upgrade to rc1, refuse
  4. apply config/chroot_local-patches/*17152.diff
  5. restart tails-upgrade-frontend.service
  6. go through the automatic upgrade process
  7. restart and verify that I’m correctly running 4.0~rc1 (SquashFS diff is mounted, tails-version reports rc1, version of the running kernel is the expected one)
  8. wait until tails-upgrade-frontend.service exits because no upgrade is available
  9. apply config/chroot_local-patches/*17152.diff
  10. restart tails-upgrade-frontend.service
  11. wait until tails-upgrade-frontend.service exits and verify in the logs that it noticed that no upgrade is available

#3 Updated by intrigeri 2019-10-14 08:43:13

intrigeri wrote:
> ⇒ I’ll propose this for 4.0 once I’ve seen the corresponding test scenario pass on my local Jenkins.

+ the persistence scenarios, as some of the tails-perl5lib code I’ve touched is also used by tails-persistence-setup.

#4 Updated by intrigeri 2019-10-14 12:01:31

  • Status changed from In Progress to Needs Validation
  • Assignee changed from intrigeri to segfault

These features fully pass on my machine: features/usb_upgrade.feature features/additional_software_packages.feature features/persistence.feature.

For the context needed to understand where I’m speaking from when proposing this for 4.0, see the ticket description and my comments above.

@segfault, I assume you could give it a cursory look, but perhaps not much more, due to the implementation language. Alternatively, I could ask kibi who’s experienced in Perl, but I’m not sure whether he has time to put into FT work this week, and regardless, the kind of changes done here (Modern Perl OOP) are not the aspect of Perl he’s most at ease with. So I’m afraid that in the end, just like for the Greeter refactoring, we’ll have little choice but to trust me personally (for not having sneaked a subtle backdoor in the code) and our CI (to spot regressions).

I’d be interested to read your thoughts about this.

(And yeah, long term, it would probably be better to 1. rewrite all this in Python or Ruby, so we have one less main language in use throughout our code base; 2. give me the time I’ll need to become proficient at Python or Ruby as I’m not there yet at all. Anyway, I’m not in a hurry given the IUK bits might go away if we switch to a non-NIH upgrader. But it could be that Wayland gives us an opportunity to at least decouple things a bit and port some of the GUI frontend to Python, who knows. We’ll see :)

#5 Updated by intrigeri 2019-10-14 12:05:15

> These features fully pass on my machine: features/usb_upgrade.feature features/additional_software_packages.feature features/persistence.feature.

… and the rest of the (non-fragile) test suite passed on my local Jenkins, modulo Bug #17104.

#6 Updated by intrigeri 2019-10-18 08:53:33

  • Assignee deleted (segfault)

If I understood Bug #17142#note-15 correctly, segfault won’t have time to handle this in time for 4.0. @anonym, maybe? (See above comments for context.)

#7 Updated by anonym 2019-10-18 09:44:55

  • Assignee set to intrigeri
  • % Done changed from 0 to 90

intrigeri wrote:
> segfault, I assume you could give it a cursory look, but perhaps not much more, due to the implementation language. Alternatively, I could ask kibi who’s experienced in Perl, but I’m not sure whether he has time to put into FT work this week, and regardless, the kind of changes done here (Modern Perl OOP) are not the aspect of Perl he’s most at ease with. So I’m afraid that in the end, just like for the Greeter refactoring, we’ll have little choice but to trust me personally (for not having sneaked a subtle backdoor in the code) and our CI (to spot regressions).

In the end I did this cursory review. I spent 20 minutes reading the diff carefully, chunk by chunk, and convinced myself that they are just rewrites. Most changes are mechanical transformations, so this was pretty easy. Essentially no logic has changed (mainly some warnings were dropped in the test suite), so I’m very confident this didn’t introduce any backdoors, and even fairly confident it should be functionally equivalent to how it was before! :D

> I’d be interested to read your thoughts about this.

In my ignorance of perl I’m just a wee bit worried of this (seemingly) big change before the 4.0 release, without any testing among users in their multitude of different contexts. This code has previously had very “opaque” bugs (like the write caching issue for Tails.module) which I guess is where my reaction comes from.

So I’m wondering if this couldn’t wait until Feature #15281 (I know: “Boooooooo, the code is written, what are we waiting for!?!?”) or the next release candidate. The two user facing advantages, “less RAM usage” and “less image size”, don’t seem compelling enough to me to take the risk since they don’t solve any specific problems at the moment.

Do you have any argument that could calm me down and feel better about this, @intrigeri? :P

#8 Updated by intrigeri 2019-10-18 09:54:39

  • Assignee changed from intrigeri to anonym

Hi @anonym,

anonym wrote:
> In the end I did this cursory review. I spent 20 minutes reading the diff carefully, chunk by chunk, and convinced myself that they are just rewrites. Most changes are mechanical transformations, so this was pretty easy. Essentially no logic has changed (mainly some warnings were dropped in the test suite), so I’m very confident this didn’t introduce any backdoors, and even fairly confident it should be functionally equivalent to how it was before! :D

Thanks a lot :)

> In my ignorance of perl I’m just a wee bit worried of this (seemingly) big change before the 4.0 release, without any testing among users in their multitude of different contexts. This code has previously had very “opaque” bugs (like the write caching issue for Tails.module) which I guess is where my reaction comes from.

I totally share these concerns.

> So I’m wondering if this couldn’t wait until Feature #15281 (I know: “Boooooooo, the code is written, what are we waiting for!?!?”) or the next release candidate. The two user facing advantages, “less RAM usage” and “less image size”, don’t seem compelling enough to me to take the risk since they don’t solve any specific problems at the moment.

I’m personally totally fine with seeing this branch merged in a few months instead of right now, if this buys us anything — which is not obvious to me, see bellow.

> Do you have any argument that could calm me down and feel better about this, @intrigeri? :P

I have no better answer to provide than what I wrote earlier here: “As usual for Upgrader changes, shipping them in the RC does not help much: we’ll only notice regressions when folks use the new code to upgrade to the final release, and by then it’s too late to fix them in the final release, so the only safe way to ship such changes is by having several beta/RC’s and hope folks actually use them and upgrade using automatic upgrades.”

Given this:

  • Our next chance to have several beta/RC’s in a row, that may give this code exposure to real-world usage in time for us to fix, in the corresponding final release, regressions spotted by testers, will be 5.0~betaN/rcN. That’s in a really long time.
  • What confidence would we gain, in your opinion, by waiting until Feature #15281 or a RC?

#9 Updated by intrigeri 2019-10-18 10:32:23

  • Status changed from Needs Validation to In Progress
  • Assignee changed from anonym to intrigeri
  • Target version changed from Tails_4.0 to Tails_4.2

Let’s minimize the number of cases when we take the risk to break the Upgrader, and to have to either document extra manual “automatic” upgrade steps, or force users to go through a manual upgrades. As such, we’ll bundle this branch with Feature #15283, and if on that one we manage to implement a good testing strategy, then the branches this ticket are about will also benefit from it.

#10 Updated by intrigeri 2019-11-23 09:42:12

  • blocks Feature #8473: Add support to live-boot to support multiple read-only lower layers with overlayfs added

#11 Updated by intrigeri 2019-11-23 10:15:38

  • blocked by deleted (Feature #8473: Add support to live-boot to support multiple read-only lower layers with overlayfs)

#12 Updated by intrigeri 2019-11-23 10:15:48

#13 Updated by intrigeri 2019-11-27 08:37:06

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

I’ve merged this into the branches for Feature #15283 and Feature #9373, so I don’t think we need a dedicated ticket to track this anymore.