*WIP* More hacky support, but for containers
Needs RevisionPublic

Authored by roshi on Mon, May 15, 6:54 PM.

Details

Reviewers
tflink
mkrizek
jskladan
Group Reviewers
libtaskotron
Summary

This patch builds off of Martin's ansible tasks commit (rLTRN2b720ca61cb8).
I've added a Dockerfile in the 'libtaskotron/ext/' directory as
well as a module to 'libtaskotron/ext/disposable' for dealing with
Docker containers.

This expects a docker daemon to be running on the host (so is akin
to --libvirt), and will build the 'taskotron-worker' image if it's
not already found on the system.

Testing this is very similar to testing the patch Martin wrote,
except you use the '--docker' flag during invocation.

To test:
$ git clone https://github.com/stefwalter/gzip-dist-git
$ sudo runtask -d -i gzip -t koji_build -a x86_64 --docker gzip-dist-git/tests/test_rpm.yml

Test Plan
  • Run the above playbook

Diff Detail

Repository
rLTRN libtaskotron
Branch
docker-tasks (branched from develop)
Lint
Lint WarningsExcuse: This is a false positive, as that is inside a string for the inventory file.
SeverityLocationCodeMessage
Warninglibtaskotron/executor.py:104E225flake8 E225
Unit
Unit Test Errors
Build Status
Buildable 1146
Build 1146: arc lint + arc unit

Unit TestsFailed

Excuse: WIP
TimeTest
1 mstesting.functest_logger.TestLogger.test_logfile_no_write
self = <testing.functest_logger.TestLogger instance at 0x7f6ff98265a8> tmpdir = local('/tmp/pytest-of-root/pytest-3/test_logfile_no_write0')
0 mstesting.test_directive_modules.TestDirectives.test_directive_class_inherits_from_base
self = <testing.test_directive_modules.TestDirectives instance at 0x7f6ff93e7b00> def test_directive_class_inherits_from_base(self):
0 mstesting.test_directive_modules.TestDirectives.test_directive_class_process_method_exists
self = <testing.test_directive_modules.TestDirectives instance at 0x7f6ff94386c8> def test_directive_class_process_method_exists(self):
1 mstesting.test_directive_modules.TestDirectives.test_directive_class_var_exists
self = <testing.test_directive_modules.TestDirectives instance at 0x7f6ff93b4bd8> def test_directive_class_var_exists(self):
0 ms.testing.functest_executor
testing/functest_executor.py:8: in <module> from libtaskotron import executor libtaskotron/executor.py:17: in <module>
View Full Test Results (4 Failed · 20 Broken · 190 Passed)
roshi created this revision.Mon, May 15, 6:54 PM

Looks good for a WIP. My concern here is, that with disposable minions, we do a thing where we use "the right fedora version", so e.g. fc24 packages are tested on F24 machine, and so on. I'd like to see the same for Docker - it can even be done quite easily. Not that it needs to happen for PoC, but I'd like at least a big fat "TODO/FIXME" somewhere in the code to remind you of that :)

libtaskotron/executor.py
18

Just a nitpick, but I'd rather see from libtaskotron.ext.disposable.docker import DockerClient and docker.DockerClient() respectively. Just nit-pick though.

62

Might be worth adding config.RuntaskModeName.DOCKER, so this can be used not only from the command line.

99

Y U No return a (ipaddr, port) tuple from the _spawn_container() directly?

libtaskotron/ext/disposable/docker.py
16

You seem to be using a lot of direct command line. I'd rather use docker-py [https://docker-py.readthedocs.io/en/stable/] - it does whatever the docker command can, and it is even packaged. Not a big deal, and it might even be overly complex for what you are doing here. But I though it worth mentioning.

libtaskotron/ext/docker/yumrepoinfo.conf
2

Is duplicating yumrepoinfo.conf really necessary - I know this is WIP (and thus the "easy" method), but I'd much rather see the conf/yumrepoinfo.conf.example being used. This only adds another place where stuff can get out of sync.

roshi added a comment.Tue, May 16, 1:02 PM

We can probably add some logic to manipulate the version of Fedora that goes into the container we run. I'll look into it.

Thanks for the feedback!

libtaskotron/executor.py
62

Yeah, that makes sense.

99

No real reason.

libtaskotron/ext/disposable/docker.py
16

I would rather use docker-python as well. However, there are a couple reasons I didn't use it.

First, the docker API is notorious for breaking things when they change it. Oddly enough, the CLI is about the most stable interface into docker. Second, the packaged versions we have in F24 and F25 are all on a much older version of the API. The API needs to be matched to the version the daemon is running on whatever host these containers will be running on. The 1.x series (that's packaged for 24 and 25) is no longer supported as they've moved onto 2.x - and 2.x is backwards incompatible with any code written for the 1.x.

I tried to write it in a fashion that if docker api get stabilized we can easily move to using the API without having to change much.

libtaskotron/ext/docker/yumrepoinfo.conf
2

we'd at the very least have to have a symlink into the docker directory for docker build to work. However, my hope is that we can get these containers built and put into the fedora registry so we don't have to maintain them here.

kparal added a subscriber: kparal.Tue, May 16, 2:14 PM
kparal added inline comments.
libtaskotron/executor.py
46

I don't really understand docker, but wouldn't it be better (and safer) to run on 127.0.0.1 rather than 0.0.0.0?

libtaskotron/ext/disposable/docker.py
16

Could we ping the package maintainer and ask her to update the package? I'd also feel better if we didn't have to parse the command line output.

19

Same question regarding default interface.

libtaskotron/ext/docker/yumrepoinfo.conf
2

+1 jskladan. Duplicating these config files (not just this one, all of them) is going to be a maintenance hell. I don't know how exactly docker build process works, but it seems we could either copy in the example config files or the system installed config files dynamically?

roshi added inline comments.Wed, May 17, 2:32 PM
libtaskotron/ext/disposable/docker.py
16

The API is tied to the version of the daemon being run. AIUI, all docker things would have to be updated. The CLI has and does remain stable, which is why I landed on using that. Also, the output we're parsing is from docker's go templates, not raw stdout - so we're much less likely to run into changes that affect our own output templates.

roshi updated this revision to Diff 3041.Thu, May 18, 2:12 PM
  • Updated to reflect review comments.
roshi updated this revision to Diff 3042.Thu, May 18, 2:27 PM

Updated diff to reflect comments in review.

roshi added a comment.Thu, May 18, 2:28 PM

Sorry for the spam. Last arc diff went against master I think, since I forgot to base it off the right commit - so it was showing a ton of stuff not included in this review.

kparal added inline comments.Fri, May 19, 2:03 PM
libtaskotron/ext/disposable/docker.py
16

When I look at python-docker-py package, the same version (1.1.0.6) is in F25, F26 and Rawhide. So IIUIC, this is API 1.x, and all the rest of the docker packages in the system are also based on API 1.x, because they need to be kept in sync, correct? API 2.x doesn't seem to have hit even Rawhide, yet.

So what exactly is the problem? We can use API 1.x, and when docker+API 2.x is introduced in Rawhide/Branched in the future, we can rewrite libtaskotron to support both (calling the relevant methods with the right arguments depending whether it's 1.x or 2.x). Do you think this is too much work, are the API changes vast? Or am I missing something else?

I'm quite nervous from command line parsing here. I understand it's supposed to be very stable, but... very nervous.

roshi added inline comments.Fri, May 19, 7:15 PM
libtaskotron/ext/disposable/docker.py
16

I guess I don't really understand what makes you nervous about parsing stdout. I would have no problem doing this in a bash script, and CLI is an API...

There are 2 different APIs at play in my thinking here. First is the actual docker API, and the second is the docker-python API. They both have a history of changing and it feels like more of a chance for a headache. If you look at the specfile for python-docker-py ( http://pkgs.fedoraproject.org/cgit/rpms/python-docker-py.git/tree/python-docker-py.spec?h=f26#n138 ) you'll see a 4 day period where it got bumped to 2.0.2 and then dropped back to 1.10.6, as an example. I'd have to do more digging to find other times docker has broken it's API, but the sentiment I've gotten across the board is that it happens fairly regularly. Digging into this seemed like it'd take more time than it's worth, and this works now.

We don't need all the functionality the API, so it feels a bit heavy to pull in a whole new dep for such a small need. The way I see it, with the current code we have to look for changes in docker cli --format flag to break things. If we use docker-py, now we have 2 places errors can crop up instead of one.

If it breaks severely, I can port it to use docker-py; though I don't think we'll need to.

kparal added inline comments.Mon, May 22, 2:21 PM
libtaskotron/ext/disposable/docker.py
16

I would have no problem doing this in a bash script

Because there are no better ways in bash...

and CLI is an API...

That's where we fundamentally differ, I believe. I don't consider CLI output to be an API, until it actually prints json/xml or something. APIs are far superior to text parsing - you have much better error handling (exceptions, typed values), you have structured data, APIs are versioned, you can get logging, etc.

First is the actual docker API, and the second is the docker-python API. They both have a history of changing and it feels like more of a chance for a headache. If you look at the specfile for python-docker-py ( http://pkgs.fedoraproject.org/cgit/rpms/python-docker-py.git/tree/python-docker-py.spec?h=f26#n138 ) you'll see a 4 day period where it got bumped to 2.0.2 and then dropped back to 1.10.6, as an example.

I talked to the maintainer (ttomecek). The 4 day period was a packaging accident. python-docker-py is API 1.x, they will keep this in all fedora releases. python-docker is API 2.x, it got renamed because upstream renamed it. They keep it in parallel in F26 and above (not sure about F24/F25 plans). According to ttomecek, the APIs are stable as long as you keep using the same major version. Between versions the APIs change a lot, that is correct. (However, since we're using only the basics, hopefully the changes would not affect us much? And it certainly should not be an unexpected change, in contrast to CLI changes, which has no stability guarantees).

If we use docker-py, now we have 2 places errors can crop up instead of one.

I don't follow here, I assume we would do everything through the python api. What's the second place?

Overall, I'm not completely opposed to cli parsing. Just trying to make sure it's really the best approach here. Of course, since the code is already written, it makes sense to use it and replace it once it gets problematic.

tflink added a subscriber: tflink.Mon, May 22, 7:14 PM
roshi added inline comments.Mon, May 22, 7:15 PM
libtaskotron/ext/disposable/docker.py
16

Because there are no better ways in bash...

The point was that if this was bash we'd run it in prod and never give it a second thought.

That's where we fundamentally differ, I believe. I don't consider CLI output to be an API...

By nature of running in a shell, it's an API. Return codes, awk (and --format flags) and application logging cover all the other bits you mentioned. But I digress, as this is probably better suited to a conversation over beers at Flock :)

What's the second place?

The second place is the docker daemon itself. If the daemon gets updated, but docker-py doesn't then it'll stop working - since they have to be in sync. It's the whole "now you have two problems" thing (at least in my head).

Of course, since the code is already written, it makes sense to use it and replace it once it gets problematic.

That's good enough for me :) As for the duplicated conf files, for now I'll just maintain it so we can push it out the door. I have a couple ideas for that in the future, but right now I don't feel that it should stop us from moving forward. That work?

kparal added inline comments.Tue, May 23, 8:51 AM
libtaskotron/ext/disposable/docker.py
16

The second place is the docker daemon itself. If the daemon gets updated, but docker-py doesn't then it'll stop working - since they have to be in sync.

OK, but we have package maintainers to keep an eye on that. That's not our problem. And they'll surely do a good job to keep things in sync, because docker is going to be an important part of our release process and artifacts we create. I don't think this is an issue.

As for the duplicated conf files, for now I'll just maintain it so we can push it out the door. I have a couple ideas for that in the future, but right now I don't feel that it should stop us from moving forward. That work?

Sure, thanks.

jskladan requested changes to this revision.Tue, May 23, 10:10 AM

Well, I'm not a huge fan of this - since not all the commands support the --format flag. This will most definitely have the same issues as testcloud does, but if the goal is to really have it now...

The only thing I'm not going to waive on is the config duplication. Fortunately, this can easily be made sane by
ADD https://pagure.io/taskotron/libtaskotron/blob/master/f/conf/yumrepoinfo.conf.example /etc/taskotron/yumrepoinfo.conf

This revision now requires changes to proceed.Tue, May 23, 10:10 AM

Have you considered using ansible's docker module [1]? Seems like they have it all implemented.

http://docs.ansible.com/ansible/docker_container_module.html