Bug #15984

Review & polish disk image generation

Added by Anonymous 2018-09-28 09:51:31 . Updated 2018-12-02 15:01:40 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Build system
Target version:
Tails_3.11
Start date:
2018-09-28
Due date:
% Done:

100%

Feature Branch:
feature/15292-generate-usb-image
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:
316

Description

The initial code lives in https://gitlab.com/segfault3/tails-usb-image-from-iso

Subtasks

History

#1 Updated by Anonymous 2018-09-28 09:52:36

  • Deliverable for set to 316

#2 Updated by Anonymous 2018-09-28 09:56:09

  • Target version set to Tails_3.11

#3 Updated by Anonymous 2018-09-28 10:20:07

  • Estimated time set to 8 h

#4 Updated by Anonymous 2018-09-28 10:36:16

  • blocked by Bug #15991: Code review & rubber-duck for USB Image added

#5 Updated by intrigeri 2018-10-09 09:21:46

  • blocks deleted (Bug #15991: Code review & rubber-duck for USB Image)

#6 Updated by intrigeri 2018-10-09 09:26:47

  • Description updated
  • Category set to Build system

#7 Updated by intrigeri 2018-10-09 10:51:53

For Feature #15316 and Feature #15317 I had to run this code so I started by reading it (I don’t run arbitrary code on my system :) hence a first few comments:

  • Very nice code overall, I’m glad you’re using type hints etc. :)
  • I’m no big fan of from logging import […] basicConfig: when reading basicConfig(level=INFO) it was not obvious at first that we were configuring the logging subsystem an not something else.
  • --use-existing lacks a help string
  • I suggest s/“Invalid ISO”/“Input file is not an ISO (no .iso extension)”/
  • I think you can drop “Set up OLPC”, CDLABEL, rootfstype= and extlinux stuff.
  • Things might go faster if using something bigger than bs=1M but of course premature optimization etc. :)
  • Not sure how we can have 'lost+found' on a FAT filesystem.
  • I think we need to make up our mind wrt. using syslinux from the ISO or from the running system and then drop the unused code.
  • orig_dir in copy_syslinux_module can probably be simplified since this code is not meant to be run on arbitrary OS’es.

#8 Updated by intrigeri 2018-10-09 11:01:03

FWIW I’ve run the script as non-root on tails-amd64-3.9.1.iso on my sid system, from a directory that is not the Git checkout of tails-usb-image-from-iso, and it failed:

INFO:__main__:Installing bootloader
INFO:__main__:Executing 'pkexec syslinux --offset 1048576 --directory /syslinux/ --install ./tails-amd64-3.9.1.img'
/bin/syslinux: ./tails-amd64-3.9.1.img: No such file or directory
INFO:__main__:Tearing down loop device
Traceback (most recent call last):
  File "/home/intrigeri/tails/tails-usb-image-from-iso/create_usb_image_from_iso.py", line 411, in <module>
    main()
  File "/home/intrigeri/tails/tails-usb-image-from-iso/create_usb_image_from_iso.py", line 406, in main
    image_creator.create_image()
  File "/home/intrigeri/tails/tails-usb-image-from-iso/create_usb_image_from_iso.py", line 113, in create_image
    self.install_syslinux_using_system_syslinux()
  File "/home/intrigeri/tails/tails-usb-image-from-iso/create_usb_image_from_iso.py", line 365, in install_syslinux_using_system_syslinux
    '--install', self.image
  File "/home/intrigeri/tails/tails-usb-image-from-iso/create_usb_image_from_iso.py", line 388, in execute
    subprocess.check_call(cmd)
  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['pkexec', 'syslinux', '--offset', '1048576', '--directory', '/syslinux/', '--install', './tails-amd64-3.9.1.img']' returned non-zero exit status 1.

There is a ./tails-amd64-3.9.1.img file in the directory where I’m running the command from.

But it worked fine if I run the command from a checkout of the repo. Probably not a big deal but we’ll need to keep this in mind when integrating this code into the build system.

#9 Updated by intrigeri 2018-10-09 12:21:23

Before assigning to me for review, please make it clear that you’ve tested booting such an image both in EFI and non-EFI modes (libvirt can do both :)

#10 Updated by segfault 2018-10-16 20:21:40

intrigeri wrote:
> * I’m no big fan of from logging import […] basicConfig: when reading basicConfig(level=INFO) it was not obvious at first that we were configuring the logging subsystem an not something else.

I completely agree. I only did this because I didn’t want to import logging (I had to train myself to not use logging.info(), but logger.info() instead, where logger = logging.getLogger(), because that prints better log messages when using multiple modules). I fixed this now.

> * --use-existing lacks a help string
> * I suggest s/“Invalid ISO”/“Input file is not an ISO (no .iso extension)”/
> * I think you can drop “Set up OLPC”, CDLABEL, rootfstype= and extlinux stuff.
> * Not sure how we can have 'lost+found' on a FAT filesystem.
> * I think we need to make up our mind wrt. using syslinux from the ISO or from the running system and then drop the unused code.
> * orig_dir in copy_syslinux_module can probably be simplified since this code is not meant to be run on arbitrary OS’es.

Fixed all of these

> * Things might go faster if using something bigger than bs=1M but of course premature optimization etc. :)

At least on my system it doesn’t go any faster with a bigger block size than 1M.

#11 Updated by segfault 2018-10-16 20:30:25

  • Assignee changed from segfault to intrigeri
  • QA Check set to Info Needed

intrigeri wrote:
> FWIW I’ve run the script as non-root on tails-amd64-3.9.1.iso on my sid system, from a directory that is not the Git checkout of tails-usb-image-from-iso, and it failed:
>
> […]
>
> There is a ./tails-amd64-3.9.1.img file in the directory where I’m running the command from.

That’s strange. I can’t reproduce it. At what commit was your repo when you saw that? I fixed an issue in ee04b7092f74ec701563e66cd3299ed834da17a0 which might have caused this. Else I suspect you hit another race condition (or the same one I already tried to work around).

#12 Updated by intrigeri 2018-10-17 07:50:19

  • Assignee changed from intrigeri to segfault

> That’s strange. I can’t reproduce it. At what commit was your repo when you saw that? I fixed an issue in ee04b7092f74ec701563e66cd3299ed834da17a0 which might have caused this. Else I suspect you hit another race condition (or the same one I already tried to work around).

I was using ee04b7092f74ec701563e66cd3299ed834da17a0.

#13 Updated by segfault 2018-10-17 16:02:14

I was able to reproduce the issue and pushed a fix (it seems to be fixed by using the absolute path to the image).

#14 Updated by segfault 2018-10-17 16:49:01

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

> Before assigning to me for review, please make it clear that you’ve tested booting such an image both in EFI and non-EFI modes (libvirt can do both :)

Done

#15 Updated by intrigeri 2018-11-02 17:25:47

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

LGTM!

I have only one serious concern: it seems that extract_iso will extract to the hard drive and not in the tmpfs we setup when building in RAM. I’m worried about the build time impact for people who don’t build in RAM and have rotating hard drives. But perhaps I’m being overly paranoid. OTOH if extract_iso extracted to the BUILD_DIR (tmpfs if building in RAM), then we would have to increase the size of that tmpfs substantially, which would probably prevent most of us from building in RAM… unless we go back to extracting the ISO contents directly into the system partition, and not in a temporary directory. It seems that you’re doing it this way only so that calculate_system_partition_size can do its job with du, right? I wonder why we can’t skip that and assume that the contents of the ISO has roughly the same size as the ISO itself (it’s not compressed, after all) or use isosize. We’re already in crude estimation land with the extra 10 MiB we add, so why not? I think this would allow fixing the concern (trade-off between build time and RAM requirements) I’m raising here.

Small things that should be trivial to fix:

  • There are still a few “XXX”. I say just drop all those that are about workarounds Tails Installer implements for udisks race conditions: I believe there’s very little chance that we hit these issues in this context (no slow bare metal flash medium, no CPU eaten by USB emulation).
  • Re. “XXX: Should we support overlay and kernel_args?”: no, I think that was a Fedora-ism.
  • Re. “XXX: Support —force and —stupid switches?”: please do the same as Tails Installer, whatever that is, to limit the risk of regressions. We’re going to switch to GRUB for Secure Boot so it’s not worth investing time now into improving our syslinux setup.
  • Re. the reference to the ChromeOS bug, two things:
    • I suspect that’s exactly why we sleep before running syslinux. Adding a comment about it.
    • I only skimmed of that bug report and it seems that it’s supposed to be fixed in mtools 4.0.18-r1. Stretch has 4.0.18 so I’m wondering if that’s the same bug. Anyway, I was just curious :)
  • We run live-build as root so I don’t think you need to pkexec syslinux. And then presumably you can drop the build-dep that was added for pkexec.

#16 Updated by intrigeri 2018-11-02 17:38:22

  • Status changed from Confirmed to In Progress

Applied in changeset commit:tails|688fca38043fe77249c54ca92fb5c91ff3ccca55.

#17 Updated by segfault 2018-11-11 14:22:17

intrigeri wrote:
> I have only one serious concern: it seems that extract_iso will extract to the hard drive and not in the tmpfs we setup when building in RAM. I’m worried about the build time impact for people who don’t build in RAM and have rotating hard drives. But perhaps I’m being overly paranoid. OTOH if extract_iso extracted to the BUILD_DIR (tmpfs if building in RAM), then we would have to increase the size of that tmpfs substantially, which would probably prevent most of us from building in RAM… unless we go back to extracting the ISO contents directly into the system partition, and not in a temporary directory. It seems that you’re doing it this way only so that calculate_system_partition_size can do its job with du, right? I wonder why we can’t skip that and assume that the contents of the ISO has roughly the same size as the ISO itself (it’s not compressed, after all) or use isosize. We’re already in crude estimation land with the extra 10 MiB we add, so why not? I think this would allow fixing the concern (trade-off between build time and RAM requirements) I’m raising here.

Done

> Small things that should be trivial to fix:
>
> * There are still a few “XXX”. I say just drop all those that are about workarounds Tails Installer implements for udisks race conditions: I believe there’s very little chance that we hit these issues in this context (no slow bare metal flash medium, no CPU eaten by USB emulation).

Done

> * Re. “XXX: Should we support overlay and kernel_args?”: no, I think that was a Fedora-ism.

Done

> * Re. “XXX: Support —force and —stupid switches?”: please do the same as Tails Installer, whatever that is, to limit the risk of regressions. We’re going to switch to GRUB for Secure Boot so it’s not worth investing time now into improving our syslinux setup.

Tails Installer supports the --force and --safe options, which are passed to syslinux as --force and --stupid, if set. I guess we can drop these.

> * Re. the reference to the ChromeOS bug, two things:
> I suspect that’s exactly why we sleep before running syslinux. Adding a comment about it.
> I only skimmed of that bug report and it seems that it’s supposed to be fixed in mtools 4.0.18-r1. Stretch has 4.0.18 so I’m wondering if that’s the same bug. Anyway, I was just curious :)

Ack

> * We run live-build as root so I don’t think you need to pkexec syslinux. And then presumably you can drop the build-dep that was added for pkexec.

I would like to keep using the script (as non-root) outside of the build process, to create USB images from existing ISOs. So I’m now only using pkexec if EUID != 0, and dropped the pkexec build-dep.

#18 Updated by segfault 2018-11-11 14:25:05

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

#19 Updated by intrigeri 2018-11-12 11:26:07

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)
  • % Done changed from 0 to 100
  • QA Check changed from Ready for QA to Pass

All right here!

#20 Updated by segfault 2018-11-14 20:28:05

  • Status changed from Resolved to In Progress
  • Assignee set to intrigeri
  • % Done changed from 100 to 80
  • QA Check changed from Pass to Ready for QA

I pushed three more commits to 1. remove some unused variables (I don’t remember why those were there in the first place), 2. simplify some code, and 3. add a missing newline.

#21 Updated by intrigeri 2018-11-15 18:22:20

  • Feature Branch set to feature/15292-usb-image

#22 Updated by intrigeri 2018-11-15 18:24:27

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)
  • % Done changed from 80 to 100
  • QA Check changed from Ready for QA to Pass

#23 Updated by segfault 2018-11-28 11:30:15

  • Status changed from Resolved to In Progress
  • Assignee set to intrigeri
  • % Done changed from 100 to 90
  • QA Check changed from Pass to Ready for QA
  • Feature Branch changed from feature/15292-usb-image to feature/15292-generate-usb-image

I pushed a small commit to remove the unnecessary cancellable=None argument from D-Bus calls. I thought this was required but your last commit shows it can be omitted.

#24 Updated by intrigeri 2018-11-28 16:44:06

  • Status changed from In Progress to Resolved
  • Assignee deleted (intrigeri)
  • % Done changed from 90 to 100
  • QA Check changed from Ready for QA to Pass

segfault wrote:
> I pushed a small commit to remove the unnecessary cancellable=None argument from D-Bus calls. I thought this was required but your last commit shows it can be omitted.

I’ve not easily found out what’s the difference between passing None explicitly vs. passing nothing, and the updated code does not seem to explode, so yeah, merged!

#25 Updated by intrigeri 2018-12-02 15:01:40

  • Due date deleted (2018-12-01)