git clean with sudo where possible

[j-baker]

Depending on whether the building user has sudo privileges, it's possible to write files in a build that Jenkins can't clean up, breaking builds for everyone else on that node. Stashbot sudo cleaning would avoid this.

[terabyte]

@j-baker - thanks for the suggestion! I'm not sure running sudo "willy nilly" is a good idea. FIrst off, this will cause a ton of spam in audit logs if passwordless sudo isn't enabled for the user. Also, people might like to know if any root files are being written, this just clobbers them all with no feedback if passwordless sudo is enabled.

A much better solution I'd recommend is having your build run a build script like this:
Cmd: ./bin/verify.sh

verify.sh:

#!/bin/bash
set -e
function cleanup {
  # Your cleanup code here
}
trap cleanup EXIT

<run your build here>

And of course, your cleanup code can be sudo chown ... or whatever. I would not be suprised if you are using docker? docker is a very common usecase that sorta "craps root-owned files everywhere". It is also possible to run as a user in your docker container whose ID is the same, and chown the files in there during your build. Long story short there are lots of fixes but the one I've suggested here is most flexible because devs can edit the script and it doesn't force other users of stashbot to run sudo, or add yet another configuration option to the config screen.

Does that make sense? Is there a usecase my suggestion doesn't cover?

Thanks!

[terabyte]

(also, feel free to pop into #stashbot on irc.freenode.org if you want to chat about this further, I am cmyers there)

[j-baker]

Yes, we were using docker. Our failure mode was that we left a few docker-created files there, then all of the build-nodes became corrupted because the clean command had insufficient privileges to remove the files, and all of our builds failed. We then had to have someone with access clear off the boxes we'd soiled, which took considerable time as we are UK based.

We have a script like that now, but created this PR to preemptively help others from having this problem - because it's not at all clear that you should need such a cleanup script.

[terabyte]

Yeah, if your build writes root-owned files, and fails to clean them up, I don't feel that should be stashbot's concern (and is in fact important to be a separated concern). Even if we added this to stashbot, it wouldn't have helped you unless the user running your builds had sudo access, and if it did, you could have checked in a script that ran "sudo " and got yourself unblocked without delay that way as well...right?

[newren]

I generally agree with Carl that it seems a bit odd to put 'sudo' invocations into stashbot. However, users can't actually get themselves unblocked because stashbot always runs 'git reset --hard && git clean -fdx' before any user-defined script, so it'll fail and quit before users have any chance to do anything.

We could switch this around slightly and have the command be
git reset --hard && { git clean -fdx || sudo git clean -fdx; }
For cases where people don't have root-owned files spewed over their tree, that would basically behave as before (because the reset --hard and clean -fdx would succeed and the '||' would short-circuit). Only when the normal git clean -fdx fails would the attempt to sudo be invoked.

But that might still be slightly odd. In the docker case where the user has sudo rights, it cleans up for us nicely. But when the problem is that someone did a 'chmod -R a-w somedir' (and we have seen stuff like that) but doesn't have sudo rights, then it's just weird and doesn't help solve the problem.

And on that note...

Do we also need to do a 'chmod -R a+w .' here? And 'find . -type f | xargs chattr -i'? And maybe a git rebase --abort? (and merge --abort and cherry-pick --abort and am abort and bisect reset)? And ...

The number of ways people can screw up their builds when we reuse them seems pretty large...

[j-baker]

Yeah, you're definitely right, at the core this issue is caused by reusing build slaves. However, the whole reason for the git clean in the first place is presumably because we use such build slaves, and because it's an easy way to handle 95% of the cases we care about.
Your modification (no sudo unless required) would increase the currently failing cases with minimal additional complexity. It seems that these Docker cases are generally problematic - the fact there are instructions on how to set uid bits and recommendations to specifically git clean yourself before stashbot does is evidence of that.

[terabyte]

Hmm, that is an exceedingly good point, @newren - I'm going to think about this a bit over the next day or two. At a minimum I would want this to be a not-enabled-by-default setting at the server level maybe, to accept it into my fork, I think. But I'll think on it more before I make a "final ruling" =)

[newren]

@terabyte: An alternative way that might be cleaner is adding a check for the existence of some global system-defined pre-job or post-job script, and if it exists, run it. That way palantir could define a global pre-job script of '/usr/local/itools/bin/palantir-cleanup.sh' and we could do the ugly, gross, site-specific stuff that tends to hit many of our internal projects (e.g. cleaning out all tags and refetching them, or sudo chown -R jenkins:jenkins ., or maybe running a setuid program that calls docker-cleanup or some chattr nonsense or whatever). It'd make it really easy for the site-admins to add new cleanup stuff this way without resaving all existing jobs (just edit the palantir-cleanup.sh script), and also has the benefit that company-specific stuff doesn't clutter the main stashbot. The only trick would be allowing the script to be defined in the global stashbot configuration page, and having that script be sucked into all the jenkins templates. (With a big warning in the UI that if you change the script name, you have to re-save all jobs.)

Or something along those lines...

[terabyte]

@newren I would greatly prefer that approach. That seems like a much cleaner and more flexible way of solving this and other problems. I'd make a text field in the jenkins config settings that is a path to a global cleanup script which is run, if it exists, in each job. Shouldn't be too difficult a change to make.