Good point.

Hmm, not on my (fc20) or kparal's (fc21) machines:

# yum -y install libtaskotron-config
Loaded plugins: langpacks, refresh-packagekit
Package libtaskotron-config-0.3.15-1.fc20.noarch already installed and latest version
Nothing to do
# echo $?
0

Does overwriting the file make more sense?

Thanks, I knew about that, just didn't fix it for some reason :D

We delete taskotron.log before execution anyway on buildmaster (and so we should delete remote_logdir as well), no? Or is it appropriate time to come up with better solution that deleting the logs?

Of course now that I've written it down, I can't reproduce it :)

I wonder what I was doing differently yesterday but it looks like this can be ignored

I think that making overwriting an option would be great. There are some cases where we want to be overwriting (taskotron.yaml) and there are other cases where we might not want to be overwriting the existing target.

On the other hand, this isn't a huge issue and we could just wait and see if it ends up causing problems. I'm fine with either

I think that we want to do something with the stdout/stderr here. I finally figured out why the task I was testing with was failing - it was missing something and blowing up before creating the log file. The error showed up in stdout on the vm but not on the task initiator.

If we end up going forward with what I had in mind for virthosts - we'll need something that's a bit more specific than what we're doing now. If we have multiple buildslaves running on the same machine, that'll create multiple logfiles and decent odds that one of them will be running when another is wrapping up.

It makes sense to me to either a) start putting logs in a per-buildslave location or b) having names that are unique enough so that each buildslave can figure out which logs belong to it.

Wasn't it saved in the stdio file on the task initiator? I am not sure I understand.

Ok, I didn't realize we were going to have multiple runners on the same machine. In that case, how about we store everything in the artifacts dir right away and not copy everything after the run is over?

I don't see any output in the stdio.log - the only file that got output was the remote-exec/taskotron.log file. I'll poke at this a bit more today to make sure, though

if the permissions work out, that sounds reasonable.

I think that we'll have more things to rework/tweak if we go forward with the multiple runners per machine plan, though. if storing everything in the artifacts dir right away doesn't work well, I think this would be better as an enhancement ticket that we deal with after figuring out some of the other details.

we don't require taskotron.yaml to be in /etc/ for normal operation, I'd like to see more flexibility here but that can be added as a later enhancement since it's a bit outside the scope of what this ticket was supposed to accomplish

I'd like to see an except block here, even if it just logs the exception that it catches - right now it's just squelching errors that cause problems

I suppose we need to make writing stdio into a file optional but to move things forward let's do it in a separate review. Thoughts?

We need to create uuid directory inside artifacts directory as soon as possible. If we do that outside (Remote)Runner class, we prevent others from using the class because it would fail on artifacts dir not existing. Any thoughts on this?

yeah, I'm fine with adding stuff later. I'm sure there's stuff that we haven't thought of yet, anyways :)

I can't think of any use cases where someone would want to use the (Remote)Runner class without the other stuff in main(). If we document the need for those directories to exist, I don't think it'd be much of a problem anyways - can you think of any use cases where that isn't the case?

Can't think of anything either. Just wanted to raise that concern in case someone would come up with something.

This throws a TB if there is a missing type or item. I'm not sure that's a valid use case but it'd be better not to blow up. I suspect that we're going to need some more arg validation or verify that what we have works for stuff like your concern about rm -rf being in item.

That being said, I also think that this is out of scope for this review.

Maybe task files (git checkout)?

This should be >= 1.15.1 to be in sync with requirements.txt.

I'm a bit confused about the remote part here. In the remote_stdio_name, I understand it, it's the stdio of the VM launched, so remote client. But in this case, it's the local taskotron log, isn't it? The remote taskotron log is saved as taskotron.log, right? So shouldn't this be local_log_name or initiator_log_name? (We really need to agree on some terminology in disposable clients mode, so that we're consistent and know what we mean).

Would it be possible to say which file? I know not every file handle has a file name attached, but maybe just for those that do? Maybe even throw an exception?

There's definitely missing something here :-)

Copy paste error from get_dir().

Copy paste error from get_dir().

You cannot call config.get_config() in the top level scope. If you do it like this, it is executed before running main() and before parsing cmdline args and running logger.init_prior_config(). If the config parsing fails for some reason, the default logging is not yet initialized, and you will not receive any helpful error messages.

You have to move config.get_config() to the class methods where you need to use it (or run it once in __init__() and save it).

On line 61 we call this artifactsdir (meaning the task-particular artifacts dir, i.e. <global artifacts dir>/<uuid>), here we call it uuid_dir. I find that mildly confusing. Can we use the same term in both places?

If you wanted to make it simpler, curl is always available ;)

Please put a line break here, and at least on line 217 as well. Thank you.

This is a great comment. But the item is provided by taskotron trigger, so the very fedmsg would have to be fake, right? So we need to make sure we check fedmsg certificates. And even if this happened, the only thing you control is a disposable client.

Would it make sense to swap these two operations? So that if somebody's task, by accident, creates a file called taskotron.log, it does not overwrite our own file?

You can also use

remote = task_data.get('environment', {}).get('machine')

if you like it more than handling the KeyError.

I'm again confused by the word remote here, a few comments would definitely improve it here. If remote is true, does it mean that this runtask script is running on a remote machine (i.e. the VM client)? Or does it mean we will schedule it remotely, i.e. we're running on the buildslave initiator?

By reading the code below, it seems that the latter is the case. Would it make sense to rename remote to run_remotely? It seems clearer to me.

A comment would help here.

A comment would help here.

A comment would help here.

A comment would help here.

OK. Left it there as it is since those lines will go away (hopefully) soon anyway.

Right. Or maybe someone could send a (force) build directly to buildbot with that item. But the attacker would need to know the password to the buildbot instance I think. Either way, the comment is just a result of me becoming more paranoid. :)