Project

General

Profile

Bug #17179

create-usb-image-from-iso maybe mixes syslinux data between Vagrant box and chroot

Added by anonym 5 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Build system
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
https://gitlab.com/johanbluecreek/tails.git:bugfix/17179-proper-chroot-for-syslinux
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

When working on migrating the Vagrant box to Debian buster some syslinux-related differences between the USB images were found.

The changes we did in #16748 were probably incomplete: for example, even if we run chroot/usr/bin/syslinux, that binary will probably look for its data files (MBR and such) in / and not in chroot/, so there's a change that we were still mixing up bits of Stretch (from the Vagrant box) with bits from Buster (from the chroot).

So, let's properly chroot into chroot/ when running syslinux. The comment #16748#note-12 might be a starting point.


Related issues

Related to Tails - Bug #17542: Check if IUK installation mixes syslinux data between the system partition and the running system's root filesystem Confirmed
Blocks Tails - Feature #16209: Core work: Foundations Team Confirmed

Associated revisions

Revision fa4ce1c8 (diff)
Added by johanbluecreek 3 months ago

create-usb-image-from-iso: Run syslinux within proper chroot (Will-fix: #17179)

Revision eabe1aef (diff)
Added by johanbluecreek about 2 months ago

Bind-mounting ISO image for hybriding instead of moving

Moving the iso can cost some time on systems that does not manage to
build in RAM (refs: #17179).

Revision 19420523 (diff)
Added by johanbluecreek about 2 months ago

Handling mounting/unmounting preparation for install of syslinux with contextmanager

This to clean up properly in case of errors (Refs: #17179)

Revision 44eaf19b
Added by intrigeri about 2 months ago

Merge remote-tracking branch 'johanbluecreek/bugfix/17179-proper-chroot-for-syslinux' into stable (Closes: #17179)

History

#1 Updated by anonym 5 months ago

#2 Updated by anonym 5 months ago

anonym wrote:

The changes we did in #16748 were probably incomplete: for example, even if we run chroot/usr/bin/syslinux, that binary will probably look for its data files (MBR and such) in / and not in chroot/, so there's a change that we were still mixing up bits of Stretch (from the Vagrant box) with bits from Buster (from the chroot).

We should verify this before we implement the suggested solution. I guess we could just run the same commands under strace or similar to record which files are actually accessed.

#3 Updated by anonym 5 months ago

  • Subject changed from create-usb-image-from-iso mixes syslinux data between Vagrant box and chroot to create-usb-image-from-iso maybe mixes syslinux data between Vagrant box and chroot

#4 Updated by intrigeri 3 months ago

anonym wrote:

We should verify this before we implement the suggested solution.

Absolutely!

I guess we could just run the same commands under strace or similar to record which files are actually accessed.

This, or… build in a basebox that has no syslinux bits at all, and see if the build fails :)

#5 Updated by johanbluecreek 3 months ago

  • Feature Branch set to https://gitlab.com/johanbluecreek/tails.git:bugfix/17179-proper-chroot-for-syslinux

In this branch syslinux is called from within the chroot env., and syslinux has been removed from the builder.

#6 Updated by johanbluecreek 3 months ago

  • Status changed from Confirmed to In Progress

#7 Updated by anonym 3 months ago

  • Assignee set to johanbluecreek
  • % Done changed from 0 to 40

johanbluecreek wrote:

In this branch syslinux is called from within the chroot env., and syslinux has been removed from the builder.

Thanks a lot! :)

I have pushed your branch to our CI infra to get your work automatically tested before marking this as Needs Validation.

#8 Updated by intrigeri 3 months ago

Woohoo!

First, welcome aboard, johanbluecreek :)

anonym wrote:

I have pushed your branch to our CI infra to get your work automatically tested before marking this as Needs Validation.

I understand that you expect johanbluecreek to manually test the image that will land in https://nightly.tails.boum.org/build_Tails_ISO_bugfix-17179-proper-chroot-for-syslinux/lastSuccessful within 24h after the 4.2.1 release.

johanbluecreek, once you've done so, please report back and deassign yourself, and then a Foundations Team member will check Jenkins results and review the code changes :)

#9 Updated by anonym 3 months ago

intrigeri wrote:

anonym wrote:

I have pushed your branch to our CI infra to get your work automatically tested before marking this as Needs Validation.

I understand that you expect johanbluecreek to manually test the image that will land in https://nightly.tails.boum.org/build_Tails_ISO_bugfix-17179-proper-chroot-for-syslinux/lastSuccessful within 24h after the 4.2.1 release.

From what I understand he has built it himself and tested it, but he hasn't run the automated test suite, so that's why I pushed it (and what we are waiting for)!

johanbluecreek, once you've done so, please report back and deassign yourself, and then a Foundations Team member will check Jenkins results and review the code changes :)

Since he does not have access to Jenkins' test results, I'll monitor this for him, and do those steps. Would be great if we could give read-only access ( publicly?) as a remedy.

#10 Updated by intrigeri 3 months ago

anonym wrote:

Since he does not have access to Jenkins' test results, I'll monitor this for him, and do those steps.

Great! :)

Would be great if we could give read-only access ( publicly?) as a remedy.

Yep (#6270). The main blocker we had for years (#10068) is gone. I don't know how much work #6270 would take.

Meanwhile, some of the results can be found on https://nightly.tails.boum.org/, but not everything is accessible, because log files contain passwords.

#11 Updated by anonym 2 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (johanbluecreek)
  • % Done changed from 40 to 50

Test results look identical to the stable branch's, so this is good to go! I was mentoring bluecreek at the time, so I probably should not review'n'merge this.

intrigeri wrote:

Meanwhile, some of the results can be found on https://nightly.tails.boum.org/, but not everything is accessible, because log files contain passwords.

I advised against this. If you already know roughly how Jenkins jobs work you can extract some crude information like "has this branch been successfully tested ever?", but given the current fragile state there's no successes at all, so no valuable info can be obtained.

#12 Updated by intrigeri 2 months ago

  • Assignee set to intrigeri

#13 Updated by intrigeri 2 months ago

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

Hi @johanbluecreek !

Great, thanks :)

My only significant concerns about this branch are:

  • The lack of cleanup in case of errors between the new mount and umount: if for whatever reason something fails in between, the build machine (e.g. a CI worker) could be stuck in a broken state until someone repairs it manually. AFAIK the Pythonic way to handle this is with a contextmanager, see e.g. how we do things with mount_iso in the same script.
  • The 2 × mv dance in auto/build, which may be costly on systems that don't build entirely in RAM, so if we can cheaply avoid it, I would prefer we do so. Couldn't we also use the bind-mount trick there? We can assume we're root in that script.

Nitpicking/FYI/FWIW/YMMV/$acronym:

  • Calling out to mkdir and touch feels slightly wrong in a Python script. OTOH they're in between shell commands and execute gives us some logging, so I'm fine with keeping it as-is.
  • Next time, please ensure you do atomic commits: e.g. the wiki/src/contribute/how/code/HACKING.mdwn file udpates don't belong to fa4ce1c8abf8786ad781c29c90f7e128ac71075a :)

#14 Updated by johanbluecreek about 2 months ago

Hi,

I pushed some new commits to my gitlab...

intrigeri wrote:

My only significant concerns about this branch are:

  • The lack of cleanup in case of errors between the new mount and umount: if for whatever reason something fails in between, the build machine (e.g. a CI worker) could be stuck in a broken state until someone repairs it manually. AFAIK the Pythonic way to handle this is with a contextmanager, see e.g. how we do things with mount_iso in the same script.

Fixed. There is now a contextmanager decorated function that handles the mounting/unmounting

  • The 2 × mv dance in auto/build, which may be costly on systems that don't build entirely in RAM, so if we can cheaply avoid it, I would prefer we do so. Couldn't we also use the bind-mount trick there? We can assume we're root in that script.

I replaced it with a bind-mount, as suggested.

With the new commits, it builds locally for me.

Nitpicking/FYI/FWIW/YMMV/$acronym:

  • Calling out to mkdir and touch feels slightly wrong in a Python script. OTOH they're in between shell commands and execute gives us some logging, so I'm fine with keeping it as-is.
  • Next time, please ensure you do atomic commits: e.g. the wiki/src/contribute/how/code/HACKING.mdwn file udpates don't belong to fa4ce1c8abf8786ad781c29c90f7e128ac71075a :)

OK, thanks. I will keep this in mind for the future.

#15 Updated by intrigeri about 2 months ago

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

Thank you for the quick fixes!

I understand that the branch is ready for a second round of review.

#16 Updated by johanbluecreek about 2 months ago

  • Assignee changed from intrigeri to johanbluecreek

intrigeri wrote:

I understand that the branch is ready for a second round of review.

Indeed!

#17 Updated by johanbluecreek about 2 months ago

  • Assignee changed from johanbluecreek to intrigeri

johanbluecreek wrote:

intrigeri wrote:

I understand that the branch is ready for a second round of review.

Indeed!

Sorry for making a mess. It appears I switched "Assignee" when I replied. That was not my intention. I'm switching back to you, correct me if wrong.

#18 Updated by intrigeri about 2 months ago

Sorry for making a mess. It appears I switched "Assignee" when I replied. That was not my intention.

I think that was not caused by a mistake on your side, but by a know Redmine UX bug related to caching. Thankfully in a few months Redmine will be a thing of the past for Tails :)

#19 Updated by intrigeri about 2 months ago

  • Target version set to Tails_4.3

#20 Updated by intrigeri about 2 months ago

Code review passes, build succeeds here as well once merged into current stable.

Manual testing:

  • The ISO boots fine from DVD in a VM.
  • I've dd'ed the ISO to a USB stick and it boots fine on a laptop I have here, that only supports legacy BIOS boot. IIRC (to be checked!) we don't support this officially anymore but well, we still make the ISO hybrid so I wanted to make sure we did not regress. I wonder if we should perhaps drop the isohybrid thing entirely at some point: it's a kludge and it can break the boot in supported use cases, so perhaps not worth the risk?
  • I've dd'ed the USB image to a USB stick and it boots fine on 1 legacy BIOS laptop and 1 UEFI laptop.

⇒ I'll merge if my local test suite run passes!

#21 Updated by johanbluecreek about 2 months ago

  • Status changed from Needs Validation to In Progress

#22 Updated by intrigeri about 2 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

#23 Updated by intrigeri 15 days ago

  • Related to Bug #17542: Check if IUK installation mixes syslinux data between the system partition and the running system's root filesystem added

Also available in: Atom PDF