WIP More hacky support, but for containers
Needs RevisionPublic
Actions

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

Details

Reviewers

tflink
jskladan
kparal
mkrizek
jsedlak

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 Warnings

Excuse: Error with linter.

Severity	Location	Code	Message
Warning	libtaskotron/executor.py:104	E225	flake8 E225

Unit

Unit Test Errors

Build Status

Buildable 1163
Build 1163: arc lint + arc unit

Unit TestsFailed
View All

Excuse: WIP

	Time	Test
	1 ms	testing.functest_logger.TestLogger.test_logfile_no_write self = <testing.functest_logger.TestLogger instance at 0x7fcb325a9128> tmpdir = local('/tmp/pytest-of-root/pytest-6/test_logfile_no_write0')
	0 ms	testing.test_directive_modules.TestDirectives.test_directive_class_inherits_from_base self = <testing.test_directive_modules.TestDirectives instance at 0x7fcb31ec6c68> def test_directive_class_inherits_from_base(self):
	0 ms	testing.test_directive_modules.TestDirectives.test_directive_class_process_method_exists self = <testing.test_directive_modules.TestDirectives instance at 0x7fcb327c4170> def test_directive_class_process_method_exists(self):
	1 ms	testing.test_directive_modules.TestDirectives.test_directive_class_var_exists self = <testing.test_directive_modules.TestDirectives instance at 0x7fcb3277fdd0> 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.May 15 2017, 6:54 PM

jskladan added a reviewer: libtaskotron.May 16 2017, 6:57 AM

jskladan added a project: libtaskotron.

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
1 ↗	(On Diff #3033)	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.

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
1 ↗	(On Diff #3033)	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.

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
1 ↗	(On Diff #3033)	+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?

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.

Updated to reflect review comments.

Updated diff to reflect comments in review.

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.

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.

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.

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.May 22 2017, 7:14 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?

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.

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.May 23 2017, 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

In D1196#22322, @mkrizek wrote:

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

We discussed this in IRC earlier this morning:

13:38 < tflink> roshi: you've talked to folks with more docker experience about this, no?
13:39 * kparal doesn't have experience with docker-compose
13:45 < roshi> I did
13:45 < roshi> I talked to adam (maxamillion) about it and he said CLI is the least headache
13:46 < roshi> as well as other, non-fedora folks I know who deal with quite a bit of docker
13:46 < roshi> the general consensus was that for what I was looking to do (which isn't very much), --format would be the easiest to deal with in the long term
13:47 < roshi> I don't really see us doing much more with docker in this context, but I could be wrong
13:47 < maxamillion> roshi: we talked about docker-compose vs ansible-docker?
13:47 < maxamillion> (also, do you mean ansible-container ?)
13:47 < roshi> we talked about python libs vs cli parsing with the --format flag
13:47 < maxamillion> oh right
13:47 < maxamillion> +1
13:47 < maxamillion> sorry, I thought you were talking about multi-container stuff

<snip>

13:49 < roshi> I can see us moving to ansible-container at some point, but for what we're trying to do right now, I don't see the value add
13:49 < roshi> seems like a pre-optimization to me, and I like to try to follow YAGNI
13:51 < maxamillion> probably a fair assessment

(edit: it formatted weird(

Update yumrepo.conf location per diff comments and remove yumrepo.conf

kparal mentioned this in D1195: Support for Ansible Tasks.Jun 13 2017, 11:10 AM

Martin just landed his commit in a feature branch, can you please rebase against that?

Rebased per @kparal 's comments.

That's weird, it's still showing more than just your changes in the diff. I merged develop into feature/ansiblize. Can you try to rebase again? I'd like to read the clean diff once more, and in the current state it's not clear which of the changes are yours.

Also please make sure you're running arc diff feature/ansiblize when updating this.

Retrying the diff... (man I hate arcanist)

On top of the failing tests, some of which are really weird, I suggest you investigate/fix ASAP, I have some mostly semantical issues.

libtaskotron/executor.py
40–42	This does not make much sense to me, honestly. Why do you have (created out of the blue, BTW, which is a bit naughty) `self.client`, which is the actual container, and then `self.task_container` which I'd intuitively think contains the container, but is in fact just a random int? Please remove `self.client`, and use `self.task_container` instead. Also, insted of returning the (evidently quite important) random int (`host_port` from `create_container`), please store the `host_port` directly in the `DockerClient` object. Whether you return the tuple (I also kind do not understand the need to return hard-coded localhost IP, especially since it is the default value of `ipaddr` later on in the `_prepare` methond, and default value of `host` in the `DockerClient`code..) to make it "more in line with `spawn_vm`, or not, I don't really care, but please get rid of this particular nonsense.
libtaskotron/ext/disposable/docker.py
19–23	please unify `host_ip` param and `self.host` variable names. I suggest going with `host_ip` in both cases, as it is descriptive of what is going on.
152	instead of returning `host_port` (which seems to be quite important in the other parts of the code), please set `self.host_port` instead.
libtaskotron/ext/docker/namespaces.yaml
1–5	I'm not really sure this is right. I would not like for the Docker hosts to be able to just do whatever, whilst virtual machines are limited to the standard set by `conf/namespaces.yaml.example`

This revision now requires changes to proceed.Jun 20 2017, 3:08 PM

Some of the test suite failures seem to be caused by missing dependencies (import hawkey or import libvirt fails). Follow the readme to install dependencies. The test suite needs to pass so that we can continue refactoring while being sure we haven't broken anything.

But a bigger problem seems to be that your code is not based on current feature/ansiblize, but on rLTRN2b720ca61cb8. That is some old version of Martin's patch and it is not part of any existing branch. You'll need to rebase it on top of feature/ansiblize, if you want to commit it there.

Also, I'd like to see some decent test suite coverage for your new code before we merge it to develop. It doesn't need to be now, but it should happen soon-ish. 'Now' would of course be better ;-) Might help to uncover some bugs already.

Thanks!

libtaskotron/executor.py
101–104	Could you change this into triple-quotes string syntax so that it's more readable? Or simple strings, but easier to read?
119–121	If possible, it's better to use the longer variant of cmdline arguments, because it's immediately understandable when reading the source code.
libtaskotron/ext/disposable/docker.py
184–186	Please move this to `exceptions.py`, possibly as `TaskotronDockerError` or something similar.

mkrizek resigned from this revision.Thu, Jul 13, 9:04 AM

jsedlak resigned from this revision.Wed, Aug 2, 10:32 AM

Revision Contents

		Path	Coverage (All)	Coverage (Touched)
M		libtaskotron/executor.py (42 lines)	0%	0%
A	M	libtaskotron/ext/disposable/docker.py (186 lines)	0%	0%
A	M	libtaskotron/ext/docker/Dockerfile (21 lines)	-	-
A	M	libtaskotron/ext/docker/namespaces.yaml (5 lines)	-	-
A	M	libtaskotron/ext/docker/taskotron.yaml (7 lines)	-	-
M		libtaskotron/main.py (2 lines)	0%	0%

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3033	2b720ca		May 15 2017, 6:54 PM	★	★
Diff 2	3041	384483a	- Updated to reflect review comments.	May 18 2017, 2:12 PM	★	★
Diff 3	3042	2b720ca	Updated diff to reflect comments in review.	May 18 2017, 2:27 PM	★	★
Diff 4	3067	384483a	- Update yumrepo.conf location per diff comments and remove yumrepo.conf	Jun 12 2017, 3:03 PM	★	★
Diff 5	3072	384483a	Rebased per @kparal 's comments.	Jun 19 2017, 7:56 PM	★	★
Diff 6	3073	2b720ca	Retrying the diff... (man I hate arcanist)	Jun 20 2017, 2:37 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
ecd00b872d5f	fa8be924ea53	2b720ca61cb8	Mike Ruckman	WIP More hacky support, but for containers (Show More…)	May 10 2017, 5:45 PM

Diff 3073

View Options

libtaskotron/executor.py

1	# -- coding: utf-8 --	1		# -- coding: utf-8 --
2	# Copyright 2009-2015, Red Hat, Inc.	2		# Copyright 2009-2015, Red Hat, Inc.
3	# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>	3		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4	# See the LICENSE file for more details on Licensing	4		# See the LICENSE file for more details on Licensing
5		5
6	from __future__ import absolute_import	6		from __future__ import absolute_import
7		7
8	import os	8		import os
9	import os.path	9		import os.path
10	import subprocess	10		import subprocess
11		11
12	from libtaskotron import config	12		from libtaskotron import config
13	from libtaskotron.logger import log	13		from libtaskotron.logger import log
14	from libtaskotron import file_utils	14		from libtaskotron import file_utils
15	from libtaskotron import os_utils	15		from libtaskotron import os_utils
16	from libtaskotron import exceptions as exc
17		16
18	from libtaskotron.ext.disposable import vm	17		from libtaskotron.ext.disposable import vm
		18		from libtaskotron.ext.disposable import docker
			jskladanUnsubmitted Not Done Just a nitpick, but I'd rather see `from libtaskotron.ext.disposable.docker import DockerClient` and `docker.DockerClient()` respectively. Just nit-pick though.
19	# TODO exc	19		# TODO exc
20		20
21		21
22	class Executor(object):	22		class Executor(object):
23	def __init__(self, arg_data):	23		def __init__(self, arg_data):
24	self.arg_data = arg_data	24		self.arg_data = arg_data
25	self.task_vm = None	25		self.task_vm = None
26	self.run_remotely = False	26		self.run_remotely = False
		27		self.task_container = None
27		28
28	def _spawn_vm(self):	29		def _spawn_vm(self):
29	self.task_vm = vm.TestCloudMachine(self.arg_data['uuid'])	30		self.task_vm = vm.TestCloudMachine(self.arg_data['uuid'])
30	self.task_vm.prepare()	31		self.task_vm.prepare()
31		32
32	log.info('Running task over SSH on machine %s', self.task_vm.instancename)	33		log.info('Running task over SSH on machine %s', self.task_vm.instancename)
33		34
34	self.task_vm.wait_for_port(22)	35		self.task_vm.wait_for_port(22)
35		36
36	return self.task_vm.ipaddr	37		return self.task_vm.ipaddr
37		38
		39		def _spawn_container(self):
		40		self.client = docker.DockerClient()
		41		self.task_container = self.client.create_container(uuid=self.arg_data['uuid'],
		42		artifacts=self.arg_data['artifactsdir'])
			jskladanUnsubmitted Not Done This does not make much sense to me, honestly. Why do you have (created out of the blue, BTW, which is a bit naughty) `self.client`, which is the actual container, and then `self.task_container` which I'd intuitively think contains the container, but is in fact just a random int? Please remove `self.client`, and use `self.task_container` instead. Also, insted of returning the (evidently quite important) random int (`host_port` from `create_container`), please store the `host_port` directly in the `DockerClient` object. Whether you return the tuple (I also kind do not understand the need to return hard-coded localhost IP, especially since it is the default value of `ipaddr` later on in the `_prepare` methond, and default value of `host` in the `DockerClient`code..) to make it "more in line with `spawn_vm`, or not, I don't really care, but please get rid of this particular nonsense.
		43
		44		log.info('Running task on a container on port {}.'.format(self.task_container))
		45
		46		return ("127.0.0.1", self.task_container)
			kparalUnsubmitted Not Done 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?
		47
38	def _prepare(self):	48		def _prepare(self):
39	# when running remotely, run directly over ssh, instead of using libvirt	49		# when running remotely, run directly over ssh, instead of using libvirt
40	persistent = False	50		persistent = False
		51		# when running on docker, run directly over ssh, into the container
		52		docker = False
41		53
42	runtask_mode = config.get_config().runtask_mode	54		runtask_mode = config.get_config().runtask_mode
43	if runtask_mode == config.RuntaskModeName.LOCAL:	55		if runtask_mode == config.RuntaskModeName.LOCAL:
44	self.run_remotely = False	56		self.run_remotely = False
45	elif runtask_mode == config.RuntaskModeName.LIBVIRT:	57		elif runtask_mode == config.RuntaskModeName.LIBVIRT:
46	self.run_remotely = True	58		self.run_remotely = True
		59		elif runtask_mode == config.RuntaskModeName.DOCKER:
		60		self.run_remotely = True
		61		docker = True
47	else:	62		else:
			jskladanUnsubmitted Not Done Might be worth adding `config.RuntaskModeName.DOCKER`, so this can be used not only from the command line.
			roshiAuthorUnsubmitted Not Done Yeah, that makes sense.
48	assert False, 'This should never occur'	63		assert False, 'This should never occur'
49		64
50	if self.arg_data['local']:	65		if self.arg_data['local']:
51	log.debug("Forcing local execution (option --local)")	66		log.debug("Forcing local execution (option --local)")
52	self.run_remotely = False	67		self.run_remotely = False
53		68
54	elif self.arg_data['libvirt']:	69		elif self.arg_data['libvirt']:
55	log.debug("Forcing remote execution (option --libvirt)")	70		log.debug("Forcing remote execution (option --libvirt)")
56	self.run_remotely = True	71		self.run_remotely = True
57	persistent = False	72		persistent = False
58		73
59	elif self.arg_data['ssh']:	74		elif self.arg_data['ssh']:
60	log.debug('Forcing remote execution (option --ssh)')	75		log.debug('Forcing remote execution (option --ssh)')
61	self.run_remotely = True	76		self.run_remotely = True
62	persistent = True	77		persistent = True
63		78
		79		elif self.arg_data['docker']:
		80		log.debug('Forcing execution on docker (option --docker)')
		81		self.run_remotely = True
		82		persistent = False
		83		docker = True
		84
64	log.debug('Execution mode: %s', 'remote' if self.run_remotely else 'local')	85		log.debug('Execution mode: %s', 'remote' if self.run_remotely else 'local')
65		86
66	ipaddr = '127.0.0.1'	87		ipaddr = '127.0.0.1'
67	if self.run_remotely:	88		if self.run_remotely and not docker:
68	if persistent:	89		if persistent:
69	ipaddr = self.arg_data['machine']	90		ipaddr = self.arg_data['machine']
70	else:	91		else:
71	ipaddr = self._spawn_vm()	92		ipaddr = self._spawn_vm()
72		93
73	with open(os.path.join('inventory'), 'w') as f:	94		with open(os.path.join('inventory'), 'w') as f:
74	f.write('[localhost]\n%s' % ipaddr)	95		f.write('[localhost]\n%s' % ipaddr)
75		96
		97		if docker:
		98		ipaddr, port = self._spawn_container()
		99
			jskladanUnsubmitted Not Done Y U No return a `(ipaddr, port)` tuple from the `_spawn_container()` directly?
			roshiAuthorUnsubmitted Not Done No real reason.
		100		with open(os.path.join('inventory'), 'w') as f:
		101		f.write('[localhost]\n%s\t\tansible_user=root'
		102		' ansible_port=%s ansible_password=passw0rd '
		103		'ansible_ssh_common_args="-o UserKnownHostsFile=/dev/null '
		104		'-o StrictHostKeyChecking=no"'% (ipaddr, port))
			Lint: flake8 E225 missing whitespace around operator
			kparalUnsubmitted Not Done Could you change this into triple-quotes string syntax so that it's more readable? Or simple strings, but easier to read?
		105
76	def _execute(self):	106		def _execute(self):
77	cmd = [	107		cmd = [
78	'ansible-playbook', self.arg_data['task'],	108		'ansible-playbook', self.arg_data['task'],
79	'-b',	109		'-b',
80	'-u root',	110		'-u root',
81	'-i', 'inventory',	111		'-i', 'inventory',
82	'-e', 'artifacts=%s' % self.arg_data['artifactsdir'],	112		'-e', 'artifacts=%s' % self.arg_data['artifactsdir'],
83	'-e', 'subjects=%s' % self.arg_data['item'],	113		'-e', 'subjects=%s' % self.arg_data['item'],
84	]	114		]
85		115
86	if self.run_remotely:	116		if self.run_remotely and not self.arg_data['docker']:
87	cmd.extend(['--private-key', self.arg_data['ssh_privkey']])	117		cmd.extend(['--private-key', self.arg_data['ssh_privkey']])
		118		elif self.arg_data['docker']:
		119		cmd.extend(['-K'])
88	else:	120		else:
89	cmd.extend(['-K', '-c', 'local'])	121		cmd.extend(['-K', '-c', 'local'])
			kparalUnsubmitted Not Done If possible, it's better to use the longer variant of cmdline arguments, because it's immediately understandable when reading the source code.
90		122
91	log.debug('Running ansible playbook %s', ' '.join(cmd))	123		log.debug('Running ansible playbook %s', ' '.join(cmd))
92	try:	124		try:
93	output, _ = os_utils.popen_rt(cmd, stderr=subprocess.STDOUT)	125		output, _ = os_utils.popen_rt(cmd, stderr=subprocess.STDOUT)
94	with open(os.path.join(self.arg_data['artifactsdir'], 'ansible.log'), 'w') as f:	126		with open(os.path.join(self.arg_data['artifactsdir'], 'ansible.log'), 'w') as f:
95	f.write(output)	127		f.write(output)
96	except subprocess.CalledProcessError, e:	128		except subprocess.CalledProcessError, e:
97	log.error('ansible-playbook ended with %d return code', e.returncode)	129		log.error('ansible-playbook ended with %d return code', e.returncode)
Show All 9 Lines		132	def start(self):
107		139
108	self._prepare()	140		self._prepare()
109	self._execute()	141		self._execute()
110		142
111	# clean up	143		# clean up
112	if self.task_vm is not None:	144		if self.task_vm is not None:
113	self.task_vm.teardown()	145		self.task_vm.teardown()
114	os.remove('inventory')	146		os.remove('inventory')
		147
		148		if self.task_container is not None:
		149		self.client.stop_container('taskotron-worker-{}'.format(self.arg_data['uuid']))
		150		self.client.rm_container('taskotron-worker-{}'.format(self.arg_data['uuid']))

View Options

libtaskotron/ext/disposable/docker.py

This file was added.

1		# -- coding: utf-8 --
2		# Copyright 2009-2015, Red Hat, Inc.
3		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4		# See the LICENSE file for more details on Licensing
5
6		"""Interface for locally spawned docker containers used for task execution."""
7
8		import os
9		import random
10		import subprocess as sp
11
12		from libtaskotron.logger import log
13
14
15		class DockerClient(object):
16		'''Helper class for working with the docker daemon.'''
	jskladanUnsubmitted Not Done 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.
	roshiAuthorUnsubmitted Not Done 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.
	kparalUnsubmitted Not Done 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.
	roshiAuthorUnsubmitted Not Done 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.
	kparalUnsubmitted Not Done 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.
	roshiAuthorUnsubmitted Not Done 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.
	kparalUnsubmitted Not Done 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.
	roshiAuthorUnsubmitted Not Done 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?
	kparalUnsubmitted Not Done 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.
17
18		def __init__(self, host_ip="127.0.0.1"):
19		''':param host_ip: The host IP where the docker daemon runs. We
	kparalUnsubmitted Not Done Same question regarding default interface.
20		assume for now that it's running on localhost.'''
21
22		# FIXME: Support running commands against a remote host.
23		self.host = host_ip
	jskladanUnsubmitted Not Done please unify `host_ip` param and `self.host` variable names. I suggest going with `host_ip` in both cases, as it is descriptive of what is going on.
24		self.images = self._get_images()
25		self.containers = self._get_containers()
26		# We need to keep a list of the containers we create, so
27		# we can co-exist with other users of whatever docker
28		# daemon we might be interacting with.
29		self.owned_containers = []
30		self.owned_images = []
31
32		def _get_images(self):
33		'''Return a list of image dicts.'''
34
35		template = "{{.ID}}\t{{.Repository}}"
36		raw_output = sp.check_output(['docker',
37		'images',
38		'--format',
39		template])
40
41		images = raw_output.split('\n')
42		attrs = ['id', 'name']
43		images = [image.split('\t') for image in images[:-1]]
44		images = [dict(zip(attrs, image)) for image in images]
45
46		return images
47
48		def _get_containers(self):
49		'''Return a list of all containers on the host.'''
50
51		# This is a go template to clean up docker ps output
52		template = "{{.ID}}\t{{.Command}}\t{{.Status}}\t{{.Ports}}\t{{.Names}}"
53
54		raw_output = sp.check_output(['docker',
55		'ps',
56		'-a',
57		'--format',
58		template])
59
60		containers = raw_output.split('\n')
61
62		# Clean up the output a bit, also remove the last, empty, entry
63		containers = [x.split('\t') for x in containers][:-1]
64
65		attrs = ['id', 'command', 'status', 'ports', 'names']
66		containers = [dict(zip(attrs, container)) for container in containers]
67
68		return containers
69
70		def _find_container(self, search_term):
71		'''Find a container based on either name or id.'''
72
73		for container in self.containers:
74		if search_term in container.values():
75		return container
76		else:
77		pass
78
79		return "No container found by {}.".format(search_term)
80
81		def _manipulate_container(self, identifier, action=None):
82		'''Start or stop a container based on either it's name or id.
83
84		:param action: command for docker, either 'start', 'stop', or 'rm'
85		:param identifier: a container ID or Name'''
86
87		if action not in ['start', 'stop', 'rm']:
88		raise ClientError("Available actions are: start, stop, and rm. "
89		"Could not perform: {}".format(action))
90
91		container = self._find_container(identifier)
92		try:
93		sp.check_call(['docker',
94		action,
95		container['names']])
96
97		except:
98		log.debug("Failed to {} the container '{}'.".format(action, container['names']))
99		raise ClientError("Could not {} the container.".format(action))
100
101		def build_image(self, path_to_dockerfile_dir, imgtag='taskotron-worker'):
102		'''Run `docker build Dockerfile` against the specified path.'''
103		sp.check_call(['docker',
104		'build',
105		'-t',
106		imgtag,
107		path_to_dockerfile_dir])
108		# Update our lists
109		self.images = self._get_images()
110		self.owned_images.append(imgtag)
111
112		def create_container(self,
113		image_name='taskotron-worker',
114		container_name='taskotron-worker',
115		uuid=None,
116		artifacts=None):
117		'''Method to create a container from a specified image.
118		:param image_name: the name of the built image you want to launch.
119		'''
120
121		# Build the image if it's not already there
122		image_found = False
123		for image in self.images:
124		if image_name == image['name']:
125		log.debug("Image already exists, skipping image build...")
126		image_found = True
127
128		if not image_found:
129		log.debug("Image not found, building...")
130		self.build_image(os.path.abspath(os.curdir) + '/libtaskotron/ext/docker/')
131
132		if uuid:
133		container_name += "-{}".format(uuid)
134
135		# Generate random port for host to use (between 2200-2300)
136		host_port = "{}".format(random.randrange(2200, 2300))
137		log.info("Container will be available on port: {}".format(host_port))
138		sp.check_call(['docker',
139		'run',
140		'-d',
141		'--name', container_name,
142		'-p', '{}:22'.format(host_port),
143		'-v', '{}:/artifacts'.format(artifacts),
144		image_name])
145
146		# Update our list of all containers
147		self.containers = self._get_containers()
148
149		# Update the list of owned_containers
150		self.owned_containers.append(self._find_container(container_name))
151
152		return host_port
	jskladanUnsubmitted Not Done instead of returning `host_port` (which seems to be quite important in the other parts of the code), please set `self.host_port` instead.
153
154		def start_container(self, identifier):
155		self._manipulate_container(identifier, action='start')
156
157		def stop_container(self, identifier):
158		self._manipulate_container(identifier, action='stop')
159
160		def rm_container(self, identifier):
161		# Update our list of owned containers, since we're deleting it
162		for i in xrange(len(self.owned_containers)):
163		if self.owned_containers[i]['names'] or self.owned_containers[i]['id'] == identifier:
164		self.owned_containers.pop(i)
165		break
166
167		self._manipulate_container(identifier, action='rm')
168
169		def clear_all(self):
170		'''Clean out all owned containers and images.'''
171
172		log.info("Clearing out all our containers...")
173		for container in self.owned_containers:
174		self.stop_container(container['names'])
175		self.rm_container(container['names'])
176
177		log.info("Clearing out all our images...")
178		for image in self.owned_images:
179		sp.check_call(['docker',
180		'rmi',
181		image])
182
183
184		class ClientError(Exception):
185		'''General exceptions we might run into using this client.'''
186		pass
	kparalUnsubmitted Not Done Please move this to `exceptions.py`, possibly as `TaskotronDockerError` or something similar.

View Options

libtaskotron/ext/docker/Dockerfile

This file was added.

		1		#FROM registry.fedoraproject.org/fedora:25
		2		FROM fedora:25
		3
		4		RUN dnf install openssh-server PyYAML libtaskotron-core libtaskotron-fedora \
		5		libtaskotron-config python-solv python-librepo passwd -y && \
		6		mkdir -p /var/log/taskotron /srv/taskotron/artifacts && \
		7		dnf clean all
		8
		9		ENV LANG C.utf8
		10		ENV LC_ALL C.utf8
		11
		12		ADD https://pagure.io/taskotron/libtaskotron/blob/master/f/conf/yumrepoinfo.conf.example /etc/taskotron/yumrepoinfo.conf
		13		ADD taskotron.yaml /etc/taskotron/taskotron.yaml
		14		ADD namespaces.yaml /etc/taskotron/namespaces.yaml
		15
		16		RUN echo "passw0rd" \| passwd --stdin root
		17		RUN ssh-keygen -t rsa -f /etc/ssh/ssh_host_rsa_key -N ""
		18
		19		EXPOSE 22
		20
		21		CMD ["/sbin/sshd", "-D"]

View Options

libtaskotron/ext/docker/namespaces.yaml

This file was added.

1		namespaces_safe:
2		- scratch
3		- qa
4		- pkg
5		- dist
	jskladanUnsubmitted Not Done I'm not really sure this is right. I would not like for the Docker hosts to be able to just do whatever, whilst virtual machines are limited to the standard set by `conf/namespaces.yaml.example`

View Options

libtaskotron/ext/docker/taskotron.yaml

This file was added.

		1		profile: production
		2		runtask_mode: local
		3		resultsdb_server: http://resultsdb:5001/api/v2.0
		4		resultsdb_frontend: http://localhost:5002
		5		execdb_server: http://localhost:5003
		6		taskotron_master: http://localhost:8010
		7		artifacts_baseurl: http://localhost:8081

View Options

libtaskotron/main.py

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s)		52	parser.add_argument('--libvirt', action='store_true',
53	help="make the task run remotely in a disposable client spawned using "	53		help="make the task run remotely in a disposable client spawned using "
54	"libvirt (the default behavior for production profile).")	54		"libvirt (the default behavior for production profile).")
55	parser.add_argument("--ssh", metavar='user@machine[:port]',	55		parser.add_argument("--ssh", metavar='user@machine[:port]',
56	help="make the task run on a remote machine over ssh")	56		help="make the task run on a remote machine over ssh")
57	parser.add_argument("--ssh-privkey", metavar="</path/to/private.key>",	57		parser.add_argument("--ssh-privkey", metavar="</path/to/private.key>",
58	help="path to private key for remote connections over ssh")	58		help="path to private key for remote connections over ssh")
59	parser.add_argument("--no-destroy", action="store_true",	59		parser.add_argument("--no-destroy", action="store_true",
60	help="do not destroy disposable client at the end of task execution")	60		help="do not destroy disposable client at the end of task execution")
		61		parser.add_argument("--docker", action="store_true",
		62		help="Run the task inside a docker container.")
61		63
62	return parser	64		return parser
63		65
64		66
65	def check_args(parser, args):	67		def check_args(parser, args):
66	""" Check if passed args doesn't have conflicts and have proper format. In case of error, this	68		""" Check if passed args doesn't have conflicts and have proper format. In case of error, this
67	function prints error message and exits the program.	69		function prints error message and exits the program.
68		70
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines

*WIP* More hacky support, but for containersNeeds RevisionPublicActions

Details

Diff Detail

Unit TestsFailedView All

Revision Contents

Diff 3073

libtaskotron/executor.py

libtaskotron/ext/disposable/docker.py

libtaskotron/ext/docker/Dockerfile

libtaskotron/ext/docker/namespaces.yaml

libtaskotron/ext/docker/taskotron.yaml

libtaskotron/main.py

WIP More hacky support, but for containers
Needs RevisionPublic
Actions

Unit TestsFailed
View All