Bug#871835: systemd-setup-machine-id leaving mount behind? (was "Bug#871835: Call for help: review patches for debootstrap")

[Michael Biebl]

Hi Antonio


sorry for the late reply...

Am 13.06.2018 um 01:20 schrieb Michael Biebl:
> Am 22.04.2018 um 15:09 schrieb Antonio Terceiro:
> 
>> # findmnt | grep machine-id
>> ├─/root/patched2/etc/machine-id
>> /dev/mapper/lemur--vg-root[/root/patched2/run/machine-id] ext4
>> ro,relatime,errors=remount-ro,data=ordered
>>
>> This explains the crash in mkosi and points the problem to something
>> that happens during the debootstrap run.
>>
>> I compared the output of debootstrap from unstable with the patched
>> debootstrap, and they are idential, i.e. packages are installed in the
>> same order, but for some reason, when running with the faster
>> debootstrap, the above mount is left over.
>>
>> Looking around, I suspect that this could be left behind by
>> systemd-machine-id-setup, however, I couldn't understand yet why this
>> would happen.
>>
>> systemd team: could you provide any insight? for reference, I am
>> attaching the current diff between debootstrap master branch, and a
>> local branch where I have Thomas Lange's patched applied.
> 
> I just stumbled over this today myself when using vmdebootstrap where I
> ran into the same issue as in [1]
> 
> As I couldn't reproduce the failure with cdebootstrap or debootstrap
> from stretch, I ran a git bisect against debootstrap.
> 
> The first faulty commit is
> https://salsa.debian.org/installer-team/debootstrap/commit/7e533a672ce413fc591b59e83f7c665c1a2e174e
> 
> And sure enough, reverting that commit fixed the dangling machine-id
> mount for me.
> 
> Henrich, do you want a bug report against debootstrap for that?

Fwiw, I didn't use systemd-nspawn or lxc.
All I did was run
# debootstrap sid /tmp/test-sid


-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Bug#871835: systemd-setup-machine-id leaving mount behind? (was "Bug#871835: Call for help: review patches for debootstrap")

[Michael Biebl]

Hi

Am 22.04.2018 um 15:09 schrieb Antonio Terceiro:

> # findmnt | grep machine-id
> ├─/root/patched2/etc/machine-id
> /dev/mapper/lemur--vg-root[/root/patched2/run/machine-id] ext4
> ro,relatime,errors=remount-ro,data=ordered
> 
> This explains the crash in mkosi and points the problem to something
> that happens during the debootstrap run.
> 
> I compared the output of debootstrap from unstable with the patched
> debootstrap, and they are idential, i.e. packages are installed in the
> same order, but for some reason, when running with the faster
> debootstrap, the above mount is left over.
> 
> Looking around, I suspect that this could be left behind by
> systemd-machine-id-setup, however, I couldn't understand yet why this
> would happen.
> 
> systemd team: could you provide any insight? for reference, I am
> attaching the current diff between debootstrap master branch, and a
> local branch where I have Thomas Lange's patched applied.

I just stumbled over this today myself when using vmdebootstrap where I
ran into the same issue as in [1]

As I couldn't reproduce the failure with cdebootstrap or debootstrap
from stretch, I ran a git bisect against debootstrap.

The first faulty commit is
https://salsa.debian.org/installer-team/debootstrap/commit/7e533a672ce413fc591b59e83f7c665c1a2e174e

And sure enough, reverting that commit fixed the dangling machine-id
mount for me.

Henrich, do you want a bug report against debootstrap for that?

Regards,
Micahel


[1] https://bugs.debian.org/899155
-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Bug#871835: systemd-setup-machine-id leaving mount behind? (was "Bug#871835: Call for help: review patches for debootstrap")

[Antonio Terceiro]

Dear systemd team: the context here is a patch set for debootstrap that,
while using a local apt cache, makes debootstrap complete in 2/3 of the
time, compared to the version currently in unstable.

see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=871835 for details

On Sat, 21 Apr 2018 20:30:47 -0300 Antonio Terceiro  wrote:
> I have also looked at these patches, and they look good to me, and also
> work great here. Using a local APT proxy, these patches make debootstrap
> complete in ~ 2/3 of the time that debootstrap from unstable takes.
> 
> The one thing I found is that trying to use debootstrap with these
> patches with mkosi makes mkosi crash like this:
> 
> ‣ Installing Debian complete.
> ‣ Assigning hostname...
> ‣ Assigning hostname complete.
> ‣ Unmounting Package Cache...
> ‣ Unmounting Package Cache complete.
> ‣ Resetting machine ID...
> Traceback (most recent call last):
>   File "/usr/bin/mkosi", line 2904, in 
> main()
>   File "/usr/bin/mkosi", line 2900, in main
> build_stuff(args)
>   File "/usr/bin/mkosi", line 2852, in build_stuff
> raw, tar, root_hash = build_image(args, workspace, run_build_script=False)
>   File "/usr/bin/mkosi", line 2711, in build_image
> reset_machine_id(args, workspace.name, run_build_script, for_cache)
>   File "/usr/bin/mkosi", line 1186, in reset_machine_id
> os.unlink(machine_id)
> OSError: [Errno 16] Device or resource busy: 
> '/home/terceiro/tmp/test-systemd-containers/.mkosi-agbwha1l/root/etc/machine-id'
> Traceback (most recent call last):
>   File "/usr/lib/python3.6/weakref.py", line 624, in _exitfunc
> f()
>   File "/usr/lib/python3.6/weakref.py", line 548, in __call__
> return info.func(*info.args, **(info.kwargs or {}))
>   File "/usr/lib/python3.6/tempfile.py", line 936, in _cleanup
> _rmtree(name)
>   File "/usr/lib/python3.6/shutil.py", line 480, in rmtree
> _rmtree_safe_fd(fd, path, onerror)
>   File "/usr/lib/python3.6/shutil.py", line 418, in _rmtree_safe_fd
> _rmtree_safe_fd(dirfd, fullname, onerror)
>   File "/usr/lib/python3.6/shutil.py", line 418, in _rmtree_safe_fd
> _rmtree_safe_fd(dirfd, fullname, onerror)
>   File "/usr/lib/python3.6/shutil.py", line 438, in _rmtree_safe_fd
> onerror(os.unlink, fullname, sys.exc_info())
>   File "/usr/lib/python3.6/shutil.py", line 436, in _rmtree_safe_fd
> os.unlink(name, dir_fd=topfd)
> OSError: [Errno 16] Device or resource busy: 'machine-id'
>
> I can't reproduce this with current debootstrap from unstable, but 1)
> none of these patches seem to do anything related, and 2) the error
> happens after debootstrap is already done, so I suspect this might be an
> issue with mkosi that is surfaced by debootstrap running faster, and not
> an issue with debootstrap itself.

Investigating further, I tested debootstrap alone, and realized that
with this patch set applied, after debootstrap finishes, a mount point
is left over:

# findmnt | grep machine-id
├─/root/patched2/etc/machine-id
/dev/mapper/lemur--vg-root[/root/patched2/run/machine-id] ext4
ro,relatime,errors=remount-ro,data=ordered

This explains the crash in mkosi and points the problem to something
that happens during the debootstrap run.

I compared the output of debootstrap from unstable with the patched
debootstrap, and they are idential, i.e. packages are installed in the
same order, but for some reason, when running with the faster
debootstrap, the above mount is left over.

Looking around, I suspect that this could be left behind by
systemd-machine-id-setup, however, I couldn't understand yet why this
would happen.

systemd team: could you provide any insight? for reference, I am
attaching the current diff between debootstrap master branch, and a
local branch where I have Thomas Lange's patched applied.
diff --git a/functions b/functions
index 1e41862..c0f749c 100644
--- a/functions
+++ b/functions
@@ -282,7 +282,7 @@ verify_checksum () {
 			error 1 SIGCHECK "Cannot check sha${SHA_SIZE}sum"
 		fi
 	fi
-	relsize="$(wc -c < "$1")"
+	relsize=`stat -c '%s' "$1"`
 	if [ "$expsize" -ne "$relsize" ] || [ "$expchecksum" != "$relchecksum" ]; then
 		return 1
 	fi
@@ -1326,9 +1326,8 @@ if in_path perl; then
 $unique = shift @ARGV; $field = lc(shift @ARGV); $mirror = shift @ARGV;
 %fields = map { $_, 0 } @ARGV;
 $prevpkg = "";
+$chksumfield = lc($ENV{DEBOOTSTRAP_CHECKSUM_FIELD}).":";
 while () {
-	chomp;
-	next if (/^ /);
 	if (/^([^:]*:)\s*(.*)$/) {
 		$f = lc($1); $v = $2;
 		if ($f eq "package:") {
@@ -1349,7 +1348,7 @@ while () {
 		$ver = $v if ($f eq "version:");
 		$arc = $v if ($f eq "architecture:");
 		$fil = $v if ($f eq "filename:");
-		$chk = $v if (lc $f eq lc($ENV{DEBOOTSTRAP_CHECKSUM_FIELD}).":");
+		$chk = $v if ($f eq $chksumfield);
 		$siz = $v if ($f eq "size:");
 		$val = $v if ($f eq $field);
 	} elsif (/^$/) {
@@ -1391,53 +1390,49 @@ while (read STDIN, $x, 1) {
 }' "$@"
 		elif [ "$1" = "GETDEPS" ]; then
 			local 

Bug#871835: Call for help: review patches for debootstrap

[Antonio Terceiro]

On Tue, 17 Apr 2018 10:42:43 +0900 Hideki Yamane  wrote:
> Hi,
> 
> On Wed, 11 Apr 2018 13:06:22 +0200
> Clément Hermann  wrote:
> > I had a look today. It looks good to me, at first glance I had concerns
> > (like using hash keys in boolean context without the exists function),
> > but everytime I checked further, it looked fine in context. That for the
> > perl part.
> > 
> > Other than that, if I may give my opinion, the commit messages make the
> > patches pretty self explanatory, and the portability concerns are
> > adressed (you still need grep -E but busybox can provide it, if your
> > grep doesn't implement it). The changes make perfect sense and the
> > performance boost is impresssive.
> > 
> > Barring any concern from someone more knowledgeable, I would definitely
> > apply this :)
> 
>  Thanks for your check! :)
>  And yes, performance improvement is so attractive for us.
> 
>  First review seems to be good status. And of course, more people
>  are also welcome.

I have also looked at these patches, and they look good to me, and also
work great here. Using a local APT proxy, these patches make debootstrap
complete in ~ 2/3 of the time that debootstrap from unstable takes.

The one thing I found is that trying to use debootstrap with these
patches with mkosi makes mkosi crash like this:

‣ Installing Debian complete.
‣ Assigning hostname...
‣ Assigning hostname complete.
‣ Unmounting Package Cache...
‣ Unmounting Package Cache complete.
‣ Resetting machine ID...
Traceback (most recent call last):
  File "/usr/bin/mkosi", line 2904, in 
main()
  File "/usr/bin/mkosi", line 2900, in main
build_stuff(args)
  File "/usr/bin/mkosi", line 2852, in build_stuff
raw, tar, root_hash = build_image(args, workspace, run_build_script=False)
  File "/usr/bin/mkosi", line 2711, in build_image
reset_machine_id(args, workspace.name, run_build_script, for_cache)
  File "/usr/bin/mkosi", line 1186, in reset_machine_id
os.unlink(machine_id)
OSError: [Errno 16] Device or resource busy: 
'/home/terceiro/tmp/test-systemd-containers/.mkosi-agbwha1l/root/etc/machine-id'
Traceback (most recent call last):
  File "/usr/lib/python3.6/weakref.py", line 624, in _exitfunc
f()
  File "/usr/lib/python3.6/weakref.py", line 548, in __call__
return info.func(*info.args, **(info.kwargs or {}))
  File "/usr/lib/python3.6/tempfile.py", line 936, in _cleanup
_rmtree(name)
  File "/usr/lib/python3.6/shutil.py", line 480, in rmtree
_rmtree_safe_fd(fd, path, onerror)
  File "/usr/lib/python3.6/shutil.py", line 418, in _rmtree_safe_fd
_rmtree_safe_fd(dirfd, fullname, onerror)
  File "/usr/lib/python3.6/shutil.py", line 418, in _rmtree_safe_fd
_rmtree_safe_fd(dirfd, fullname, onerror)
  File "/usr/lib/python3.6/shutil.py", line 438, in _rmtree_safe_fd
onerror(os.unlink, fullname, sys.exc_info())
  File "/usr/lib/python3.6/shutil.py", line 436, in _rmtree_safe_fd
os.unlink(name, dir_fd=topfd)
OSError: [Errno 16] Device or resource busy: 'machine-id'

I can't reproduce this with current debootstrap from unstable, but 1)
none of these patches seem to do anything related, and 2) the error
happens after debootstrap is already done, so I suspect this might be an
issue with mkosi that is surfaced by debootstrap running faster, and not
an issue with debootstrap itself.


signature.asc
Description: PGP signature

Bug#871835: Call for help: review patches for debootstrap

[Hideki Yamane]

Hi,

On Wed, 11 Apr 2018 13:06:22 +0200
Clément Hermann  wrote:
> I had a look today. It looks good to me, at first glance I had concerns
> (like using hash keys in boolean context without the exists function),
> but everytime I checked further, it looked fine in context. That for the
> perl part.
> 
> Other than that, if I may give my opinion, the commit messages make the
> patches pretty self explanatory, and the portability concerns are
> adressed (you still need grep -E but busybox can provide it, if your
> grep doesn't implement it). The changes make perfect sense and the
> performance boost is impresssive.
> 
> Barring any concern from someone more knowledgeable, I would definitely
> apply this :)

 Thanks for your check! :)
 And yes, performance improvement is so attractive for us.

 First review seems to be good status. And of course, more people
 are also welcome.

 
-- 
Regards,

 Hideki Yamane henrich @ debian.org/iijmio-mail.jp

Bug#871835: Call for help: review patches for debootstrap

[Clément Hermann]

Hi,

On 06/04/2018 04:01, Hideki Yamane wrote:
> Hi Perl people,
> 
>  Could you help us to check patches for embedded Perl code in debootstrap?
>  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=871835
> 
>  It would improve to make debootstrap faster a lot, but we also afraid to
>  break something.
> 

Disclaimer:
- I'm a perl team member, but not a DD (nor a DM).
- I'm definitely not the best perl expert available in the team, but
hopefully other members can have a quick look too.
- I use debootstrap a lot, but it's the first time I actually looks at
its code and I might miss some stuff regarding that.

So, now that it's out of the way...

I had a look today. It looks good to me, at first glance I had concerns
(like using hash keys in boolean context without the exists function),
but everytime I checked further, it looked fine in context. That for the
perl part.

Other than that, if I may give my opinion, the commit messages make the
patches pretty self explanatory, and the portability concerns are
adressed (you still need grep -E but busybox can provide it, if your
grep doesn't implement it). The changes make perfect sense and the
performance boost is impresssive.

Barring any concern from someone more knowledgeable, I would definitely
apply this :)


Cheers,

-- 
nodens