Project

General

Profile

Bug #12577

Feature #5630: Reproducible builds

Bug #12531: ISO builds on Jenkins are fragile since the migration to vagrant-libvirt

Aborted Jenkins ISO builds can cause next ones on the same isobuilder to fail

Added by intrigeri over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Continuous Integration
Target version:
Start date:
05/22/2017
Due date:
% Done:

100%

Feature Branch:
puppet-tails:bugfix/12577-cleanup-failed-build-leftovers
Type of work:
Sysadmin
Blueprint:
Starter:
Affected tool:

Description

E.g. https://jenkins.tails.boum.org/job/build_Tails_ISO_bugfix-12529-simplify-kernel-install-in-vagrant/5/ left 14G of data behind itself in /var/lib/jenkins/workspace/build_Tails_ISO_bugfix-12529-simplify-kernel-install-in-vagrant/vagrant/, which broke the next ISO build on that isobuilder. I had to manually clean this up to repair that isobuilder.

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 :)

History

#1 Updated by bertagaz over 2 years ago

  • 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.

#2 Updated by intrigeri over 2 years ago

That's a rough workaround but that will be robust. We can do that at first as a hotfix

OK, good.

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.

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.

#3 Updated by bertagaz over 2 years ago

  • 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 over 2 years ago

  • 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!

#5 Updated by intrigeri over 2 years ago

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.

#6 Updated by bertagaz over 2 years ago

  • % 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.

#7 Updated by bertagaz over 2 years ago

bertagaz wrote:

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.

I've just pushed another commit in the puppet-tails repo to fix the directory listing syntax, that was incomplete and was ignoring the reproducible build jobs workspace. I'll try to abort a job later.

#8 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

  • % 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 over 2 years ago

  • 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 over 2 years ago

  • 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 over 2 years ago

  • 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 over 2 years ago

  • 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.

#15 Updated by intrigeri over 2 years ago

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.

Thanks. I'll review when the other bits are solved :)

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.

Nitpicking: the killall cannot fail, it's just sending a signal.

If your plan is to keep things as-is, i.e. "send an async signal and immediately do stuff that depends on the async operation to have already completed", then please add a comment explaining that you're aware that all this stuff is racy, happens to work by chance so far, and why you don't think implementing a proper solution is worth our time. Which might take as much time as actually fixing the problem, but that's your call.

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.

OK. I will, once you've done what I'm asking ("please speficy […]").

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.

This is still too unclear for me to understand, sorry if I'm being thick here. I don't see how running processes can block rm -rf, so I suspect there's some confusion going on here (rm vs. umount, possibly).

Reading the rest of the script, it seems that the /tmp/tmp* "temporary directories" are the chroot built by vmdebootstrap, i.e. the mountpoint of /dev/mapper/loop0p1. Correct?

My understanding is that these gpg-agent's are run within that chroot, and then if they're still around when we try to umount $vmdebootstrap_device, the umount operation will fail; that's why we have the fuser --ismountpoint --mount "$vmdebootstrap_device" thing is place, and thus the killall gpg-agent done later is useless. But your explanation is different: you're telling me that these gpg-agent processes block rm -rf /tmp/tmp*. This makes me very doubtful. Actually, I wonder if these lines were useful (when you added them) because of some bug in the script, that you've fixed since then, e.g. the spurious "=" char that prevented the fuser thing to work.

So I see two options here: either you bother reverse-engineering your thought process and add a comment to explain what exactly these lines are useful for, or you try removing them and then we'll have actual data to understand what exactly they were useful for (if it's the case). I think the latter option is simpler, cheaper, and nicer to everyone involved. Let's just do that, no?

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.

Sure, agreed :)

#16 Updated by bertagaz over 2 years ago

intrigeri wrote:

Nitpicking: the killall cannot fail, it's just sending a signal.

Well, it can exit with exit code > 0. But as it "cannot fail" in the way you meant, I was referring to "fail to kill the process and prevent the rest of the script to run correctly".

If your plan is to keep things as-is, i.e. "send an async signal and immediately do stuff that depends on the async operation to have already completed", then please add a comment explaining that you're aware that all this stuff is racy, happens to work by chance so far, and why you don't think implementing a proper solution is worth our time. Which might take as much time as actually fixing the problem, but that's your call.

I won't fight. Added the --wait option to the killalls.

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.

OK. I will, once you've done what I'm asking ("please speficy […]").

Forgot to say I did add comments in the script regarding this.

This is still too unclear for me to understand, sorry if I'm being thick here. I don't see how running processes can block rm -rf, so I suspect there's some confusion going on here (rm vs. umount, possibly).

Reading the rest of the script, it seems that the /tmp/tmp* "temporary directories" are the chroot built by vmdebootstrap, i.e. the mountpoint of /dev/mapper/loop0p1. Correct?

My understanding is that these gpg-agent's are run within that chroot, and then if they're still around when we try to umount $vmdebootstrap_device, the umount operation will fail; that's why we have the fuser --ismountpoint --mount "$vmdebootstrap_device" thing is place, and thus the killall gpg-agent done later is useless. But your explanation is different: you're telling me that these gpg-agent processes block rm -rf /tmp/tmp*. This makes me very doubtful. Actually, I wonder if these lines were useful (when you added them) because of some bug in the script, that you've fixed since then, e.g. the spurious "=" char that prevented the fuser thing to work.

Nop, you're wrong, in part because I've been sloppy when I described why this lines: gpg-agent is running with homedir set to /tmp/tmpXXX, while the chroot is set in /tmp/tmp.XXX. That's not the same directory. I think it's started outside the chroot by debootstrap early stage.

If gpg-agent was killed in the chroot umounting loop as you assume, then the line killing it would have failed in this run. It's not because it's not tight to the chroot and thus not part of the processes being killed by the chroot umouting loop.

So I see two options here: either you bother reverse-engineering your thought process and add a comment to explain what exactly these lines are useful for, or you try removing them and then we'll have actual data to understand what exactly they were useful for (if it's the case). I think the latter option is simpler, cheaper, and nicer to everyone involved. Let's just do that, no?

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.

Sure, agreed :)

So that's why this line is still here even if it does not prevent the following rm -rf line to run correctly.

#17 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

  • 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 over 2 years ago

  • 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.

#20 Updated by intrigeri over 2 years ago

But I won't do that before 3.1 if it ain't going to be looked at before September.

I would look at it before September. What I can't do before September is my own development (that I want to test locally before pushing to production).

#21 Updated by intrigeri over 2 years ago

the polishing you're asking is not so urgent.

Sigh. Documenting our code so that it makes sense to the reviewer is not "polishing", it's merely the basic code quality standards we've been applying everywhere else in Tails for ages; I see no reason why sysadmin code would be subject to lower standards, nor why I need to demand such basic things so often on sysadmin work. This has been going on for too long and I've grown increasingly tired of this situation, so I'll start a conversation soonish about this topic (among others meta bits because we can't keep going through review processes like this forever).

#22 Updated by intrigeri over 2 years ago

bertagaz wrote:

It's still in better shape than before,

Sorry, I should at least have acknowledged that: yes, it is vastly better than before. Thanks!

#23 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

  • 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 (#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 over 2 years ago

  • 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 over 2 years ago

  • 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.

Also available in: Atom PDF