Project

General

Profile

Bug #16461

Bug #10976: persistence.conf lost, recoverable by reconfiguring

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

Added by intrigeri 4 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Persistence
Target version:
Start date:
02/12/2019
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:

Description

… as suggested in #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 #10976
  • for me when I get debugging data about #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).

Related issues

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

Associated revisions

Revision 774fdc56 (diff)
Added by intrigeri 4 months ago

Test suite: also verify that persistence.conf.bak has safe access rights (refs: #16461).

In e7ebff6aabc53dbe80c2aab2e2538dc02d55091a I added a check
on its existence; let's also check it has safe access rights,
just like we do already for all other persistence configuration files.

Revision f6a3da1b (diff)
Added by intrigeri 4 months ago

Test suite: only check that persistence.conf.bak exists on recent enough Tails (refs: #16461).

This makes our test suite pass even when --old-iso points to Tails 3.12, by
skipping a new test that would not work there. More specifically, the "Writing
files to a read/write-enabled persistent partition with the old Tails USB
installation" scenario would fail.

Else, whenever --old-iso is not passed at all, or it points to an ISO built from
a branch that has the code which creates persistence.conf.bak (#16461), we'll
check for its existence and access rights.

Revision c8734d10 (diff)
Added by intrigeri 4 months ago

Import persistence-setup's bugfix/16461-lost-persistence-config branch (refs: #16461)

… at commit 4f591943ec2ec519d14533d2772cd12b7215c681

Revision ba139e44 (diff)
Added by intrigeri 4 months ago

Import persistence-setup's bugfix/16461-lost-persistence-config branch (refs: #16461)

… at commit 17ac976586a04e7f6ec7d9cf8f8541ac5c712fea.

Revision 3616be4c
Added by intrigeri 3 months ago

Merge branch 'bugfix/16461-lost-persistence-config' into stable (refs: #16461)

Revision e154a026 (diff)
Added by intrigeri 3 months ago

Remove tps-16461.diff: included in tails-persistence-setup 2.1.0 (Fix-committed: #16461)

History

#1 Updated by intrigeri 4 months ago

#2 Updated by intrigeri 4 months ago

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 #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 #10976 disappear.

#3 Updated by intrigeri 4 months ago

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

#4 Updated by intrigeri 4 months ago

  • 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 4 months ago

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

#6 Updated by intrigeri 4 months ago

I've merged the branch for #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 #14596.

#7 Updated by intrigeri 4 months ago

  • 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 4 months ago

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 4 months ago

  • 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 4 months ago

  • 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 3 months ago

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

#12 Updated by intrigeri 3 months ago

  • 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 3 months ago

  • 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 3 months ago

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 3 months ago

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

#16 Updated by intrigeri 3 months ago

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

#17 Updated by intrigeri 3 months ago

  • 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 #16568 during the 3.14 cycle :)

#18 Updated by CyrilBrulebois 3 months ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF