Tails

#1 Updated by bertagaz 2017-05-25 12:11:09

• Status changed from Confirmed to In Progress
• QA Check set to Info Needed

intrigeri wrote:
> How about we delete /var/lib/jenkins/workspace/build_Tails_ISO_*/vagrant/ (possibly excluding the currently running job’s directory) at some relevant time, e.g. before building? There might be a more clever solution though, I’ll let you think about it :)

That’s a rough workaround but that will be robust. We can do that at first as a hotfix, and try to use the Conditional Buildstep plugin that can be configured to run a build step (not a job) when it is marked as aborted or failed or on a lot of different conditions. Given how much we seem to rely on being able to run things like cleaning up after a build, I bet this will prove to be useful if it works. It seems to be compatible with the Jenkins version we’re running.

#3 Updated by bertagaz 2017-05-30 13:08:50

• Assignee changed from bertagaz to intrigeri
• % Done changed from 0 to 30
• QA Check changed from Info Needed to Ready for QA
• Feature Branch set to puppet-tails:bugfix/12577-cleanup-build-jobs-leftovers; jenkins-jobs:bugfix/12577-cleanup-build-jobs-leftovers

intrigeri wrote:
> Note: cleaning up after a build will only fix this problem in some cases (i.e. the easiest, cleanest failure modes). For a fix that works in the general case, the only option is to clean up when starting a build.

True. Pushed branches in puppet-tails and jenkins-job Git repo.

#4 Updated by intrigeri 2017-05-30 15:31:55

• Assignee changed from intrigeri to bertagaz
• % Done changed from 30 to 50
• QA Check changed from Ready for QA to Dev Needed

Looks good to me, please deploy (either after testing locally in a very similar environment, or at a time when you’re ready to quickly deal with a possible fallout :) Thanks!

#6 Updated by bertagaz 2017-06-01 10:52:40

• % Done changed from 50 to 60

intrigeri wrote:
> FWIW this just happened again: https://jenkins.tails.boum.org/job/build_Tails_ISO_feature-12599/18/console broke due to the previously failed build + clean_all not deleting libvirt volumes. I’ve manually stopped jenkins-slave.service, used virsh vol-delete to clean things up, and rebooted isobuilder2.

Ok. I’ve just deployed the branches, first build that used it is https://jenkins.tails.boum.org/job/reproducibly_build_Tails_ISO_feature-5630-deterministic-builds/11/console and got this step running fine. I’ll abort a job later today and will start a build on the same isobuilder to see how it goes.

#8 Updated by intrigeri 2017-06-08 17:49:32

• Target version changed from Tails_3.0 to Tails_3.1

Let’s consider our CI infra as frozen until 3.0 is out.

#10 Updated by bertagaz 2017-07-07 19:27:52

• % Done changed from 60 to 40

I played a bit today aborting stable build jobs (will mark them in Jenkins) at different build stages and watching the isobuilder system state after that, and how the next build behaves.

• At the basebox creation time: worst case. The vmdebootrap dmsetup/loop mounting of the basebox qcow2 image in vagrant/definitions/tails-builder/ directory stays in place, while the backend file is deleted. Subsequent builds on that host fail. We have to add some prebuild cleanup logic to remove any exiting leftovers from this stage.
• At the initial basebox provisioning step: works fine, next builds do not fail. That’s probably the one covered so far.
• At the ISO build step: works fine too. Unsurprisingly.

So there are some more cleanup to add for the first case. That’s a bit of dmsetup/mouting things that are in the root shell history of isobuilder3.lizard, and need to be scripted. More to come.

#11 Updated by bertagaz 2017-07-10 20:20:15

• Assignee changed from bertagaz to intrigeri
• Priority changed from Elevated to Normal
• % Done changed from 40 to 50
• QA Check changed from Dev Needed to Ready for QA

bertagaz wrote:
> * At the basebox creation time: worst case. The vmdebootrap dmsetup/loop mounting of the basebox qcow2 image in vagrant/definitions/tails-builder/ directory stays in place, while the backend file is deleted. Subsequent builds on that host fail. We have to add some prebuild cleanup logic to remove any exiting leftovers from this stage.
>
> So there are some more cleanup to add for the first case. That’s a bit of dmsetup/mouting things that are in the root shell history of isobuilder3.lizard, and need to be scripted. More to come.

I’ve extensively looped in extending and testing the actual cleanup_build_jobs_leftovers script, at different stages of the basebox creation. It seems solid enough to be RfQA.

It’s not that an emergency right now, aborting is robust enough to wait for the review and likely won’t happen much during the summer, so downgrading priority.

OTOH part of the review is to click on the aborting button in Jenkins at various stages and see how the next build goes, then unlikely paste a link if something goes wrong. :)

#12 Updated by intrigeri 2017-07-13 09:06:28

• Feature Branch changed from puppet-tails:bugfix/12577-cleanup-build-jobs-leftovers; jenkins-jobs:bugfix/12577-cleanup-build-jobs-leftovers to puppet-tails:master

(Fixing the “Feature Branch” field value as apparently the change was pushed straight to master.)

#13 Updated by intrigeri 2017-07-13 09:32:17

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

Wow, it seems you studied this class of problems thoroughly, congrats!

What does ${vmdebootstrap_mountpoint=} refer to? I suspect that’s a typo and then this fuser call can’t ever kill anything. Anyway, I think this part of the script can be greatly simplified by using the source of the mount instead of the target:

@@ -14,15 +14,13 @@ sudo killall debootstrap || true
 # Remove mounting left by vmdebootstrap when it's aborted.
 sudo umount /tmp/tmp*/proc || true
 sudo umount /tmp/tmp*/sys || true
-vmdebootstrap_mountpoint="$(mount | grep /dev/mapper/loop0p1 | awk '{ print $3 }')"
-if [ -n "${vmdebootstrap_mountpoint}" ]; then
-       tries=0
-       while ! sudo umount -f --verbose ${vmdebootstrap_mountpoint} && [ $tries -lt 12 ]; do
-               sudo fuser --ismountpoint --mount ${vmdebootstrap_mountpoint=} --kill || true
-               sleep 5
-               tries=$(expr $tries + 1)
-       done
-fi
+vmdebootstrap_device=/dev/mapper/loop0p1
+tries=0
+while ! sudo umount -f --verbose "$vmdebootstrap_device" && [ $tries -lt 12 ]; do
+        sudo fuser --ismountpoint --mount "$vmdebootstrap_device" --kill || true
+        sleep 5
+        tries=$(expr $tries + 1)
+done
 if sudo dmsetup ls | grep -qs loop0p1; then
        sudo dmsetup remove loop0p1
 fi

Note that signal handling is async’ by default on Unix. They way you use killall is thus racy: there’s no guarantee that any affected process has died when the next shell command is run. So the only wait to do this in a robust manner is to either loop until all such processes have died, or use killall’s --wait option (and hope the affected processes don’t ignore the SIGTERM process).

Please make patterns passed to grep more precise. I can do that myself if you prefer: in this case, please specify unambiguously (in English) what you’re trying to do and I’ll translate them into regexps.

Finally, I’d like clarifications about killall gpg-agent as I don’t get it: what’s the problem with remaining gpg-agent processes? If they were running inside some chroot, then killing them after unmounting everything seems useless (too late to be useful). And if they’re running on the host filesystem, then I don’t see why they need to be killed. Can you please explain what exact problem this command solves? I note that we don’t use sudo for that command, so I guess it’s about the jenkins user’s gpg-agent.

(In passing: at this point I’m starting to wonder if rebooting the isobuilder just like we do for isotesters would be simpler – and vastly more robust – than this pile of hacks we apparently have to do. But well, I trust your testing and let’s keep what’s working already for now. Let’s just keep that other option in mind :)

> OTOH part of the review is to click on the aborting button in Jenkins at various stages and see how the next build goes, then unlikely paste a link if something goes wrong. :)

I think I’ll just let some time pass and will close this ticket if I don’t see any such problem again :) Also, next time you play with such error conditions, pretty please do so on a topic branch of yours, and not on one of our base branches: the build history of the stable branch is meaningless now, and we got tons of false positive email that made me waste time understanding why the build of the stable branch was that broken :/

#14 Updated by bertagaz 2017-07-13 10:17:16

• Assignee changed from bertagaz to intrigeri

intrigeri wrote:
> Wow, it seems you studied this class of problems thoroughly, congrats!

I did.

> What does ${vmdebootstrap_mountpoint=} refer to? I suspect that’s a typo and then this fuser call can’t ever kill anything.

Yeah, typo. Funny thing is that it did seems to work anyway.

> Anyway, I think this part of the script can be greatly simplified by using the source of the mount instead of the target:

That’s how I implemented it at first, but was not sure it would work with fuser. Committed that.

> Note that signal handling is async’ by default on Unix. They way you use killall is thus racy: there’s no guarantee that any affected process has died when the next shell command is run. So the only wait to do this in a robust manner is to either loop until all such processes have died, or use killall’s --wait option (and hope the affected processes don’t ignore the SIGTERM process).

Yeah, I’ve wondered about --wait, but did not see this killall fail so far.

> Please make patterns passed to grep more precise. I can do that myself if you prefer: in this case, please specify unambiguously (in English) what you’re trying to do and I’ll translate them into regexps.

Please do. As it is now it’s working fine and there are very little chances we ever see another dm/loopback devices on this hosts.

> Finally, I’d like clarifications about killall gpg-agent as I don’t get it: what’s the problem with remaining gpg-agent processes? If they were running inside some chroot, then killing them after unmounting everything seems useless (too late to be useful). And if they’re running on the host filesystem, then I don’t see why they need to be killed. Can you please explain what exact problem this command solves? I note that we don’t use sudo for that command, so I guess it’s about the jenkins user’s gpg-agent.

That’s because gpg-agents are using a temporary directories, and blocking their removal at the next line of the script. It’s not precisely making the next build fail, but I thought it would be better to not have dozens of gpg-agents running, and their temporary directories.

> (In passing: at this point I’m starting to wonder if rebooting the isobuilder just like we do for isotesters would be simpler – and vastly more robust – than this pile of hacks we apparently have to do. But well, I trust your testing and let’s keep what’s working already for now. Let’s just keep that other option in mind :)

I would not say rebooting is simpler, given how it goes with the isotesters. :)

> > OTOH part of the review is to click on the aborting button in Jenkins at various stages and see how the next build goes, then unlikely paste a link if something goes wrong. :)
>
> I think I’ll just let some time pass and will close this ticket if I don’t see any such problem again :)

Ack, no probs. We’ll see if there will be other aborted builds.

> Also, next time you play with such error conditions, pretty please do so on a topic branch of yours, and not on one of our base branches: the build history of the stable branch is meaningless now, and we got tons of false positive email that made me waste time understanding why the build of the stable branch was that broken :/

Yeah, I did not think I’d need to abort that many builds when I started.

#17 Updated by intrigeri 2017-07-22 11:09:29

• Target version changed from Tails_3.1 to Tails_3.2

(I won’t have access to my dev infra much before September, and I don’t want to develop this in production.)

#18 Updated by intrigeri 2017-07-24 06:18:12

• Assignee changed from intrigeri to bertagaz
• Target version changed from Tails_3.2 to Tails_3.1

Well, actually, there are improvements that can be done in the meantime. They’re all about 1. making this script maintainable (as seen in this discussion it’s not exactly self-explanatory so some documentation is clearly needed IMO); 2. avoiding to broad side effects:

• regarding killall --wait gpg-agent:
  • please explain with comments what you explained above (whenever I see this line in 2 years and wonder what this huge hammer solution is about, I don’t want to have to guess or do Redmine archeology)
  • please use --user so the side-effects are a bit less wild: we’re not running single purpose Docker containers but full-blown Debian systems, and there may be other services/cronjobs that need a working gpg-agent; hopefully they won’t run as the same user as the one that left these gpg-agent instances behind. Actually, using loginctl there’s probably a much simpler, robust and without side-effects way to do all the killing we need in this script, but well; that’ll be for next time we need to do something like this :)
• Please add a comment explaining where /tmp/tmp* come from in sudo rm -rf /tmp/tmp*. I think you’ve explained that above.
• For the dmsetup/losetup removal, please replace the current comments (that don’t tell anything that the code doesn’t say already) with something that explains where these devices come from.
• s/Remove mounting left/Unmount filesystems left mounted/
• FTR the specifications for the regexps I’m supposed to improve don’t describe what the regexps are trying to do (they only state what the code block they’re used in is doing), but whatever, I’ll guess myself.
• Then reassign to me with target version = 3.2 and I’ll make the regexps more accurate.

#19 Updated by bertagaz 2017-07-25 18:53:48

• Target version changed from Tails_3.1 to Tails_3.2

intrigeri wrote:
> Well, actually, there are improvements that can be done in the meantime. They’re all about 1. making this script maintainable (as seen in this discussion it’s not exactly self-explanatory so some documentation is clearly needed IMO); 2. avoiding to broad side effects:

But I won’t do that before 3.1 if it ain’t going to be looked at before September. It’s still in better shape than before, and the polishing you’re asking is not so urgent.

#23 Updated by intrigeri 2017-07-29 08:34:29

• Target version changed from Tails_3.2 to Tails_3.1
• Feature Branch changed from puppet-tails:master to puppet-tails:bugfix/12577-cleanup-failed-build-leftovers

While looking at build logs on Jenkins I’ve noticed two bugs in this script:

• in the general case, /dev/mapper/loop0p1 is not mounted but we’ll try 12 times anyway, during one minute, to unmount it and kill whatever processes might be locking it;
• I don’t think that fuser --ismountpoint --mount ${vmdebootstrap_device} --kill will ever kill anything: --ismountpoint will prevent that.

I believe the untested branch I’ve pushed should fix these problems.

It also removes the two “interesting” regexps I was supposed to improve, so there’s no need to wait for September on that front either. I think these commits (and the process that has lead to me producing them) are a textbook example of one frustrating pattern we’re seeing over and over again, so I’ll use them as examples in a “meta” email I’m gonna write right now.

I’m also resetting the target version that was changed based on a misunderstanding.

#24 Updated by bertagaz 2017-08-01 12:14:40

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

intrigeri wrote:
> * in the general case, /dev/mapper/loop0p1 is not mounted but we’ll try 12 times anyway, during one minute, to unmount it and kill whatever processes might be locking it;

Seen that too, did not have the time to look in depth why, thanks for the fix.

> * I don’t think that fuser --ismountpoint --mount ${vmdebootstrap_device} --kill will ever kill anything: --ismountpoint will prevent that.

That’s a remaining of a patch of yours (Bug #12577#note-13) I’ve applied in puppet-tails:2356e54 (sorry for the bad review).

> I believe the untested branch I’ve pushed should fix these problems.

I do too.

> It also removes the two “interesting” regexps I was supposed to improve, so there’s no need to wait for September on that front either.

I did not get you were available for something else than the regexp rewriting before September.

> I think these commits (and the process that has lead to me producing them) are a textbook example of one frustrating pattern we’re seeing over and over again, so I’ll use them as examples in a “meta” email I’m gonna write right now.

Seen that. Need more time to process it peacefully enough.

> I’m also resetting the target version that was changed based on a misunderstanding.

Yes. At the time I delayed it, another reason was that I had to focus on 3.1, without knowing how available I would be. So that was a way to lower the priority of that task. Meanwhile it appeared I had spare time for this. Unclear self-management bla…

I think I’ve fixed all your remaining remarks in my latest commits on that branch pointing to this ticket, so assigning back to you.

#25 Updated by intrigeri 2017-08-01 15:51:24

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

>> I think these commits (and the process that has lead to me producing them) are a textbook example of one frustrating pattern we’re seeing over and over again, so I’ll use them as examples in a “meta” email I’m gonna write right now.

> Seen that. Need more time to process it peacefully enough.

There’s no rush :)

> I think I’ve fixed all your remaining remarks in my latest commits on that branch pointing to this ticket, so assigning back to you.

Looks OK to me, thanks. Please deploy ideally after testing extensively locally, or at a time when you’re ready to cope with the (potential) fallout in the following week or so. And if you want to deploy and then test this on our production setup, please do so on a branch of yours and not on stable/devel this time ;)

I’m really glad we’ll soon be able to close this ticket (and that the issue it’s about is already essentially fixed :)

#26 Updated by bertagaz 2017-08-03 15:29:09

• Status changed from In Progress to Resolved
• Assignee deleted (bertagaz)
• % Done changed from 50 to 100
• QA Check changed from Dev Needed to Pass

intrigeri wrote:
> > I think I’ve fixed all your remaining remarks in my latest commits on that branch pointing to this ticket, so assigning back to you.
>
> Looks OK to me, thanks. Please deploy ideally after testing extensively locally, or at a time when you’re ready to cope with the (potential) fallout in the following week or so. And if you want to deploy and then test this on our production setup, please do so on a branch of yours and not on stable/devel this time ;)

Deployed, tested, works fine. Case closed.