Bug #16461

Backup persistence.conf before modifying it in t-p-s

Added by intrigeri 2019-02-12 18:41:37 . Updated 2019-03-20 14:26:18 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Persistence
Target version:
Tails_3.13
Start date:
2019-02-12
Due date:
% Done:

100%

Feature Branch:
bugfix/16461-lost-persistence-config, persistence-setup:bugfix/16461-lost-persistence-config
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

… as suggested in Bug #10976#note-27.

Even with the restore code suggested there, saving a backup should already be a net win:

  • for users who might notice the “.bak” file and recover themselves
  • for help desk when they help users recover from Bug #10976
  • for me when I get debugging data about Bug #10976 (depending on the mtime of the backup file, I’ll be able to tell for sure if t-p-s was used at all recently, regardless of what users remember: nothing else modifies this backup file).

Subtasks

Related issues

Related to Tails - Feature #14544: Spend software developer time on smallish UX improvements In Progress 2018-08-31
Related to Tails - Bug #16568: Make writing persistence.conf.bak more robust Resolved 2019-03-17
Blocks Tails - Feature #15507: Core work 2019Q1: Foundations Team Resolved 2018-04-08

History

#1 Updated by intrigeri 2019-02-12 18:42:03

#2 Updated by intrigeri 2019-02-12 18:55:41

I’ve implemented the backup part (not tested yet) and reimplemented the “save updated persistence.conf” code to make it more robust when bad things happen. In passing, I made it so changes made by other code (e.g. live-persist) to persistence.conf are written synchronously on the disk.

While doing this work, I’ve noticed an issue in the previous implementation, that could have lead to the updated file being saved (as in “CTRL+s”) but not sync’ed to disk (as in “sync”), so it could possibly have been left empty if the system was not shutdown properly. I don’t know if this was the root cause for Bug #10976 (some users say they did not use t-p-s at all), we’ll see.

For 3.13 I probably won’t have time to implement the “restore from persistence.conf.bak if persistence.conf is empty” part. But even without that part, I think the work I did will be useful already (see ticket description)… and I’m somewhat curious to see if that’ll be enough to make Bug #10976 disappear.

#3 Updated by intrigeri 2019-02-12 18:56:40

  • related to Feature #14544: Spend software developer time on smallish UX improvements added

#4 Updated by intrigeri 2019-02-12 19:01:59

  • Feature Branch changed from bugfix/10976-lost-config, persistence-setup:bugfix/10976-lost-config to bugfix/16461-lost-persistence-config, persistence-setup:bugfix/10976-lost-config

#5 Updated by intrigeri 2019-02-14 17:16:20

  • blocked by Feature #14596: Write automated tests for Additional Software GUI added

#6 Updated by intrigeri 2019-02-14 17:33:35

I’ve merged the branch for Feature #14596 into this one because on current stable, the test about persistence.conf is disabled (has been since the 1st iteration of ASP tests was merged) but that’s fixed on the branch for Feature #14596.

#7 Updated by intrigeri 2019-02-15 07:26:25

  • Feature Branch changed from bugfix/16461-lost-persistence-config, persistence-setup:bugfix/10976-lost-config to bugfix/16461-lost-persistence-config, persistence-setup:bugfix/16461-lost-persistence-config

#8 Updated by intrigeri 2019-02-15 17:12:29

Test suite passes without --old-iso locally. Waiting for Jenkins results since it does use --old-iso (and I’ve added code to treat both cases differently).

#9 Updated by intrigeri 2019-02-16 06:09:03

  • Assignee deleted (intrigeri)
  • % Done changed from 10 to 50
  • QA Check set to Ready for QA

intrigeri wrote:
> Test suite passes without --old-iso locally. Waiting for Jenkins results since it does use --old-iso (and I’ve added code to treat both cases differently).

Jenkins (with --old-iso) is as happy as one can expect.

#10 Updated by intrigeri 2019-02-18 15:24:43

  • Assignee set to anonym

@anonym, can you please review this one? Once happy, feel free to reassign to me so I release a new t-p-s and drop the temporary patch from tails.git (or do it yourself :)

Also, see the blocking relationship wrt. merging order.

#11 Updated by intrigeri 2019-03-11 14:41:09

  • blocks deleted (Feature #14596: Write automated tests for Additional Software GUI)

#12 Updated by intrigeri 2019-03-11 14:46:49

  • Assignee changed from anonym to CyrilBrulebois

@CyrilBrulebois, can you please review this one? Once happy, feel free to reassign to me so I release a new t-p-s and drop the temporary patch from tails.git.

The bulk of the work lives in https://git.tails.boum.org/persistence-setup/log/?h=bugfix/16461-lost-persistence-config — beware, Modern Perl ahead! To be clear about the expectations: until now this code got only very cursory reviews by anonym, who speaks little Perl, let alone Modern Perl; so if you take a quick look, in the hope you spot an obvious mistake, without bothering with understanding everything in great details, that’ll be good enough; OTOH if you have time and interest to dive into that codebase a little more and grasp it in more depth, this could be useful in the future (bus factor—).

The corresponding branch in tails.git only adds an automated test.

#13 Updated by CyrilBrulebois 2019-03-16 16:52:45

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

Thanks for the pointer in the commit message regarding where spew is defined, indirections in OOP sometimes makes it hard to spot where stuff is coming from…

The changes LGTM (hence QA pass); I’m just wondering whether it would make sense to apply (parts of the) sync/@chattr@ dance to the backup as well?

#14 Updated by intrigeri 2019-03-16 17:51:58

> The changes LGTM (hence QA pass);

Merged into both repos, I will release & upload a new t-p-s tomorrow to replace the patch in tails.git.

> I’m just wondering whether it would make sense to apply (parts of the) sync/@chattr@ dance to the backup as well?

I’ll take a look tomorrow and if needed, will file a dedicated ticket. Thanks!

#15 Updated by intrigeri 2019-03-17 08:24:50

  • Status changed from In Progress to Fix committed
  • % Done changed from 50 to 100

Applied in changeset commit:tails|e154a02603e004bdffd0831d345dc0b8dfd4359b.

#16 Updated by intrigeri 2019-03-17 08:39:50

  • related to Bug #16568: Make writing persistence.conf.bak more robust added

#17 Updated by intrigeri 2019-03-17 08:40:51

  • Assignee deleted (intrigeri)

> > I’m just wondering whether it would make sense to apply (parts of the) sync/@chattr@ dance to the backup as well?
>
> I’ll take a look tomorrow and if needed, will file a dedicated ticket. Thanks!

@CyrilBrulebois, yes, it would make sense, good catch! See you on Bug #16568 during the 3.14 cycle :)

#18 Updated by CyrilBrulebois 2019-03-20 14:26:18

  • Status changed from Fix committed to Resolved