Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Creating Docker images with pure Salt and running Salt commands inside containers #34484

Merged
merged 27 commits into from Jul 15, 2016
Merged

[WIP] Creating Docker images with pure Salt and running Salt commands inside containers #34484

merged 27 commits into from Jul 15, 2016

Conversation

@dmacvicar
Copy link
Contributor

@dmacvicar dmacvicar commented Jul 6, 2016

What does this PR do?

  • It implements creating Docker images from sls states (Salt on the image not required, base image only needs python)

screenshot from 2016-07-05 23-16-16

cmds_bvwyaairc-

  • Allows to run arbitrary Salt modules inside a container without needing Salt in the container (uses salt-thin).

cmrl4paviaemgly

What issues does this PR fix or reference?

Tests written?

Yes. (dockerng.sls pending)

cc @meaksh @isbm @dincamihai

@cachedout
Copy link
Contributor

@cachedout cachedout commented Jul 6, 2016

This is really awesome!

@cachedout cachedout added the Awesome label Jul 6, 2016
@thatch45
Copy link
Member

@thatch45 thatch45 commented Jul 6, 2016

Awesome! You got it in @dmacvicar !

@rallytime
Copy link
Contributor

@rallytime rallytime commented Jul 6, 2016

Neat!

@dmacvicar There are a couple of pylint errors to clean up. It would also be good to get some .. versionadded:: Carbon tags in the docstrings of each of the new public-facing functions.

return st_.state.compile_high_data(high_data)


def _gather_pillar(pillarenv, pillar_override, grains={}):

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Just change the signature like this:

def _gather_pillar(pillarenv, pillar_override, **grains):
@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 6, 2016

Thanks for the feedback. I corrected everything mentioned until now.

  • What do you think of the name of the call function? Is there any convention to these meta-calls? snapper module will introduce a similar one. It would be cool if they follow some convention in naming and style.

Originally, the function signature was: call(container_id, function=None, args=[], kwargs={}) but now is just call(container_id, function, *args, **kwargs), which means I just forward all parameters. It makes it easier to use but also means that I have to filter __key like args injected by salt so that `call(id, 'test.ping') works.

  • The current thin handling is a bit brute force and does not has the amount of logic salt-ssh does to the unpacking.
@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 6, 2016

  • Delete the tarballs in /tmp before commit
@isbm
Copy link
Contributor

@isbm isbm commented Jul 6, 2016

@dmacvicar The call signature is now correct. Nothing wrong with the filtering, since everything starting with __ is usually technical "carryover" that you can throw away, in case you need.

@isbm
Copy link
Contributor

@isbm isbm commented Jul 6, 2016

@dmacvicar The name call itself isn't really bad, although keep in mind that there is __call__ function to every "callable" class:

class Foo(object):
    def __call__(self):
        return "Hello world"

print(Foo()())  # Prints "Hello world"

@cachedout @thatch45 objections?

'''
if 'cp.fileclient' not in __context__:
__context__['cp.fileclient'] = \
salt.fileclient.get_file_client(__opts__)

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

BTW, this can be one line. No need to enforce 80 characters in Salt.

This comment has been minimized.

@thatch45

thatch45 Jul 6, 2016
Member

yes, this can be one line

This comment has been minimized.

@dmacvicar

dmacvicar Jul 13, 2016
Author Contributor

Fixed.
As I showed @isbm, main problem is that running pylint --rcfile=.testing.pylintrc --disable=W1307 salt/modules/dockerng does not have the same behavior as what I see in Jenkins.

errors += ext_errors
errors += st_.state.verify_high(high_data)
if errors:
return errors

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Up to you though, but usually I try always avoid += on strings, replacing by:

errors = ' '.join([errors, ext_errors, st_.state.verify_high(high_data)]).strip()
if errors:
    return errors

And so on. But exactly this part isn't very much performant, so you can leave it as is. At least it saves codelines. 😉

This comment has been minimized.

@thedrow

thedrow Jul 8, 2016
Contributor

I think that joining will be slower than +=. Also whenever you do something like this, use tuples.

This comment has been minimized.

@isbm

isbm Jul 8, 2016
Contributor

@thedrow Actually, I just wrote a little script to test both, getting meaningful numbers. Joining is about 3x times faster. So it didn't changed since Python 1.5 version 😉

'''
# put_archive reqires the path to exist
ret = __salt__['dockerng.run_all'](name, 'mkdir -p /tmp/salt_thin')
if ret['retcode'] != 0:

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Nitpick: in this case != 0 is only an explicit self-documentation "Not Zero" for a code reader. But technically you can say just:

if ret['retcode']:
    ....

To a Python reader it means "something non-False is in the "retcode" section".

'-l', 'quiet',
'--',
function
] + list(args) + ['{0}={1}'.format(key, value) for (key, value) in kwargs.items() if not key.startswith('__')]

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Few things:

  1. Here args already a list.
  2. Use six.iteritems instead of dict.items(): https://pythonhosted.org/six/#six.iteritems

This comment has been minimized.

@dmacvicar

dmacvicar Jul 6, 2016
Author Contributor

args is a tuple, I converted it to list because I got an error from the interpreter.

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Ah, right. Immutable.

This comment has been minimized.

@s0undt3ch

s0undt3ch Jul 7, 2016
Member

And use six.iteritems() since obj.iteritems() is not python 3 compatible.
Also, .items() is not wrong, it just uses more memory under python 2(On python 3 its equivalent to obj.iteritems().

.. code-block:: bash
salt myminion dockerng.sls compassionate_mirzakhani mods=rails,web

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

😉

if mods is not None:
mods = [x.strip() for x in mods.split(',')]
else:
mods = []

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

This can be one line:

mods = [item.strip() for item in mods.split(',')] if mods else []
try:
trans_tar_sha256 = salt.utils.get_hash(trans_tar, 'sha256')
__salt__['dockerng.copy_to'](name, trans_tar,
'/tmp/salt_state.tgz',

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Here I would suggest to use a random prefix in the archive name.

This comment has been minimized.

@thatch45

thatch45 Jul 6, 2016
Member

subdir again, this is almost as bad as downloading code from the internet without validation and then executing it ;)

grains = __salt__['dockerng.call'](name, function='grains.items')

# compile pillar with container grains
pillar = _gather_pillar(saltenv, {}, grains)

This comment has been minimized.

@dmacvicar

dmacvicar Jul 6, 2016
Author Contributor

I think this call is wrong now that I changed the signature. I think I have to pass **grainshere to convert the dict to keyword arguments.

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

Well, yes. If you have something like:

def foo(*args, **kwargs): pass

Then you supposed to call it this way:

foo(*['some', 'stuff'], **{'here': 'after'})

The only limitation here that keywords in kwargs must be always strings.

ret = __salt__['dockerng.create'](image=base,
cmd='/usr/bin/sleep infinity',
interactive=True, tty=True)
id = ret['Id']

This comment has been minimized.

@isbm

isbm Jul 6, 2016
Contributor

JFYI: id is a built-in function and you are over-writing it. On the other hand you are unlikely will need it.

This comment has been minimized.

@thatch45

thatch45 Jul 6, 2016
Member

our lint checks should get mad about this one, we often just use the var id_

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 8, 2016

@thedrow What do you mean with base.sls? There are 3 functions:

  • dockerng.call : just executes a salt module into a running container
  • dockerng.sls : from a list of sls modules, calculates the highstate, and executes it into a running container (taking care of pillar data and grains). It does not commit, you can do that with dockerng.commit.
  • dockerng.sls_build : from a list of sls modules, a base image and a new image name, creates a new image that results of applying the state to the base image. This commits the new image and tags it with the given name.
@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 8, 2016

I think all feedback until now is incorporated. Feel free to point to more issues. Some stuff that can be reviewed:

  • handling of environments, pillar overrides.
_mk_fileclient()
trans_tar = salt.client.ssh.state.prep_trans_tar(
__context__['cp.fileclient'],
chunks, refs, pillar=pillar)

This comment has been minimized.

@cachedout

cachedout Jul 8, 2016
Contributor

We still need an id passed in here. Otherwise we're getting this:

AttributeError: 'NoneType' object has no attribute 'startswith'
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/home/mp/devel/salt/salt/scripts.py", line 352, in salt_call
    client.run()
  File "/home/mp/devel/salt/salt/cli/call.py", line 58, in run
    caller.run()
  File "/home/mp/devel/salt/salt/cli/caller.py", line 134, in run
    ret = self.call()
  File "/home/mp/devel/salt/salt/cli/caller.py", line 197, in call
    ret['return'] = func(*args, **kwargs)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5780, in sls_build
    __salt__['dockerng.sls'](id_, mods, saltenv, **kwargs)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5711, in sls
    trans_tar = _prepare_trans_tar(mods=mods, saltenv=saltenv, pillar=pillar)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5573, in _prepare_trans_tar
    chunks, refs, pillar=pillar)
  File "/home/mp/devel/salt/salt/client/ssh/state.py", line 148, in prep_trans_tar
    cachedir = os.path.join('salt-ssh', id_)
  File "/usr/lib/python2.7/posixpath.py", line 68, in join
    if b.startswith('/'):
AttributeError: 'NoneType' object has no attribute 'startswith'
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/home/mp/devel/salt/salt/scripts.py", line 352, in salt_call
    client.run()
  File "/home/mp/devel/salt/salt/cli/call.py", line 58, in run
    caller.run()
  File "/home/mp/devel/salt/salt/cli/caller.py", line 134, in run
    ret = self.call()
  File "/home/mp/devel/salt/salt/cli/caller.py", line 197, in call
    ret['return'] = func(*args, **kwargs)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5780, in sls_build
    __salt__['dockerng.sls'](id_, mods, saltenv, **kwargs)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5711, in sls
    trans_tar = _prepare_trans_tar(mods=mods, saltenv=saltenv, pillar=pillar)
  File "/home/mp/devel/salt/salt/modules/dockerng.py", line 5573, in _prepare_trans_tar
    chunks, refs, pillar=pillar)
  File "/home/mp/devel/salt/salt/client/ssh/state.py", line 148, in prep_trans_tar
    cachedir = os.path.join('salt-ssh', id_)
  File "/usr/lib/python2.7/posixpath.py", line 68, in join
    if b.startswith('/'):
AttributeError: 'NoneType' object has no attribute 'startswith'

This comment has been minimized.

@dmacvicar

dmacvicar Jul 8, 2016
Author Contributor

@cachedout what do you mean by "here" and how do I reproduce this?

This comment has been minimized.

@cachedout

cachedout Jul 8, 2016
Contributor

This was my command: sudo salt-call --local -ldebug dockerng.sls_build base=centos vim mods=demo. It seem to blow up if prep_trans_tar isn't given an id kwarg.

This comment has been minimized.

@dmacvicar

dmacvicar Jul 8, 2016
Author Contributor

I do the same. Can you share your demo module?

This comment has been minimized.

@cachedout

cachedout Jul 10, 2016
Contributor

  test.show_notification:
    - text: 'Notify me'
@thedrow
Copy link
Contributor

@thedrow thedrow commented Jul 9, 2016

I meant an sls file that acts like a highstate but for containers.

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 9, 2016

@thedrow a highstate is at the end resolved as a list of sls modules, so I guess this could be added as a layer on top, just like the functions dockerng.call dockerng.sls and dockerng.sls_buildare layered one on top of another.

I played with some ideas around it, but none worked very well, but a dockerng.highstateshould be entirely possible.

dmacvicar added 4 commits Jul 12, 2016
@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 12, 2016

@cachedout I am still unable to reproduce the id_ problem.

My setup is Salt master+minion with Vagrant, Salt 2015.08 and the dockerng plugin symlinked into the _modules of the Vagrant shared folder.

How is your environment setup?

I would like to reproduce it to make sure there are no other issues of missing parameters as well.

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 13, 2016

@cachedout reproduced and fixed. It only happens on 2016.x.

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 14, 2016

This should be more or less ready for merging.

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 14, 2016

As next steps (future work) I suggest:

  • creating an equivalent state functions like dockerng.image_present

May be reuse the same function and instead of

myuser/myimage:mytag:
  dockerng.image_present:
    - build: /home/myuser/docker/myimage

also allow

myuser/myimage:mytag:
  dockerng.image_present:
    - mods:
      - foo
   - base: leap

(I thought about having sls_build in the same function but I was not convinced that the amount of possible combinations of parameters was worth it, feedback welcome)

@cachedout cachedout merged commit b11fd8e into saltstack:develop Jul 15, 2016
1 of 5 checks passed
1 of 5 checks passed
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt Linode Ubuntu14.04 #2992 — FAILURE
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #3284 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #3431 — FAILURE
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #3349 — SUCCESS
Details
@cachedout
Copy link
Contributor

@cachedout cachedout commented Jul 15, 2016

Merged! Thanks @dmacvicar. This is really great.

.. code-block:: bash
salt myminion dockerng.call test.ping

This comment has been minimized.

@vutny

vutny Jul 28, 2016
Contributor

@dmacvicar It should be like this or maybe I figured it out wrong way:

salt myminion dockerng.call containter_name test.ping

I think container name or hash argument (name) has missed.

@vutny
Copy link
Contributor

@vutny vutny commented Jul 28, 2016

@dmacvicar This is super cool awesome stuff! Thanks!
But it would be really helpful to have some tutorial with examples on docs.saltstack.com about how to use these new dockerng functions.

@dmacvicar
Copy link
Contributor Author

@dmacvicar dmacvicar commented Jul 28, 2016

@thedrow
Copy link
Contributor

@thedrow thedrow commented Jul 29, 2016

I think we need to add a new section to the to the tutorials page.

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jul 29, 2016

I agree, having a tutorial helps usage immensely. I will get that ball rolling

@dmacvicar dmacvicar deleted the dmacvicar:docker_images branch Jul 30, 2016
@vutny
Copy link
Contributor

@vutny vutny commented Aug 1, 2016

At least some short intro at the top docstring of dockerng module would be great.

@dmacvicar I left an in-line comment above: #34484 (comment)

Could you please take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants