[MUST BE CAREFULLY REVIEWED] saltcloud / lxc improvments #10869

Merged
merged 18 commits into from Mar 6, 2014

Conversation

Projects
None yet
8 participants
Contributor

kiorky commented Feb 28, 2014

If you want to begin review, feel free but it is not finished.

Idea is to complete lxc modules , fix some salt cloud code (multiprocessing) and add a saltcloud lxc provider

Contributor

kiorky commented Feb 28, 2014

Contributor

salt-jenkins commented Feb 28, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1915/

Contributor

salt-jenkins commented Feb 28, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1916/

Contributor

salt-jenkins commented Mar 1, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1923/

Owner

techhat commented Mar 1, 2014

@s0undt3ch, would you please take a close look at this?

Owner

s0undt3ch commented Mar 1, 2014

@techhat I sure will, though, once I have the necessary time available... @kiorky don't despair!

Contributor

mgwilliams commented Mar 1, 2014

Note that lxc userspace tools v1.0 was released last week. It is a complete overhaul of the tools, and I think packages are going to be aggressively pushed out for current OS releases. While it is mostly backwards compatible, it did break the salt lxc module. I mention it here, as some of the updates for 1.0 (particularly the lxc.info function) would not be incompatible with this pull request. I should have my updates pushed out by end of day on Monday.

Contributor

kiorky commented Mar 1, 2014

what im doing is 1.0 only anyway. :)

Contributor

kiorky commented Mar 1, 2014

And, im thinking im now at 75%/80% done.

Contributor

kiorky commented Mar 1, 2014

(i think i wont have time to add much tests for the moment)

Contributor

kiorky commented Mar 1, 2014

rebased my current working copy, there must be debug and pdb in there ^^

Contributor

kiorky commented Mar 1, 2014

Strangely github doesnt show commits in the order of my git log does

Contributor

salt-jenkins commented Mar 1, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1929/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1932/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1933/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1934/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1935/

Contributor

kiorky commented Mar 2, 2014

#10884 fixes one of the broken tests

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1937/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1939/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1942/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1943/

Contributor

salt-jenkins commented Mar 2, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1944/

Contributor

kiorky commented Mar 2, 2014

I think that 100% of the stuff is there, im now doing some tests, but review can be done.

Ping @thatch45 @s0undt3ch

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1952/

Owner

s0undt3ch commented Mar 3, 2014

Ok. My first observation is, can you submit the LXC stuff on another PR(even if one depends on the other, just mention it)? Does it really neend to be the same?

My other comments will come a little later since inline commenting from the phone is not that great.

Contributor

kiorky commented Mar 3, 2014

Well, it will be tedious for me to locally have them separated as they are very dependant... Or if i inter-merge branches, then the history would be cluterred

This was referenced Mar 3, 2014

Contributor

kiorky commented Mar 3, 2014

So i created #10897 & #10898 which on this PR is dependant, @s0undt3ch

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1954/

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1958/

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1960/

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1961/

Owner

thatch45 commented Mar 3, 2014

@kiorky, it looks like these are still churning, I will come back tomorrow to re-review as I don't have a lot of PR time today

Contributor

kiorky commented Mar 3, 2014

rebased to commits done on lxc by @mgwilliams

Contributor

salt-jenkins commented Mar 3, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1977/

Owner

s0undt3ch commented Mar 3, 2014

I still have some comments to do but I can only come back to this on the 5th Mar. @thatch45 if you beat me to it and are comfortable with the code, merge ahead and I'll comment and/or change afterwards...

Sorry for being slow here @kiorky...

Pedro Algarvio @ Phone

----- Reply message -----
From: "SaltStack Jenkins" notifications@github.com
To: "saltstack/salt" salt@noreply.github.com
Cc: "Pedro Algarvio" pedro@algarvio.me
Subject: [salt] [MUST BE CAREFULLY REVIEWED] saltcloud / lxc improvments (#10869)
Date: Mon, Mar 3, 2014 23:37
Test FAILed.

Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1977/


Reply to this email directly or view it on GitHub.

Contributor

kiorky commented Mar 3, 2014

no problems, i think i triggered another bunches of problems with virtual terminal, but it s a pain to debug them.
However, via normal salt bootstrap, provision is ok.
Indeed, our custrom bootstrap script is a bit long to execute (15minutes the first time) and the vt exists telling 'sucess' whereas the underlying script is not finished at all, so minion is unresponsive and sald cloud fails.
I had to switch on another topic late on this afternoon so i didnt finnished to debug that.
/cc @regilero @thatch45

Contributor

salt-jenkins commented Mar 4, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1990/

Contributor

kiorky commented Mar 4, 2014

also depend now on #10927

Contributor

salt-jenkins commented Mar 4, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1992/

Contributor

salt-jenkins commented Mar 4, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2012/

Contributor

salt-jenkins commented Mar 4, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2013/

Contributor

salt-jenkins commented Mar 4, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2016/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2028/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2031/

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+def __virtual__():
+ '''
+ Needs no special configuration
+ '''
+ return True
+
+
+def _get_grain_id(id_):
+ if not get_configured_provider():
+ return
+ infos = get_configured_provider()
+ return 'salt.cloud.lxc.{0}.{1}'.format(infos['target'], id_)
+
+
+def _minion_opts():
+ cfg = os.environ.get('SALT_MINION_CONFIG', '/etc/salt/minion')
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Don't hard code the configuration paths. Please replace /etc/salt/minion with os.path.join(salt.syspaths.CONFIG_DIR, 'minion').

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+
+def _get_grain_id(id_):
+ if not get_configured_provider():
+ return
+ infos = get_configured_provider()
+ return 'salt.cloud.lxc.{0}.{1}'.format(infos['target'], id_)
+
+
+def _minion_opts():
+ cfg = os.environ.get('SALT_MINION_CONFIG', '/etc/salt/minion')
+ opts = config.minion_conf(cfg)
+ return opts
+
+
+def _master_opts():
+ cfg = os.environ.get('SALT_MASTER_CONFIG', '/etc/salt/master')
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Don't hard code the configuration paths. Please replace /etc/salt/master with os.path.join(salt.syspaths.CONFIG_DIR, 'master').

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ kwargs = kw.pop('kwargs')
+ except KeyError:
+ kwargs = {}
+ if not target:
+ infos = get_configured_provider()
+ if not infos:
+ return
+ target = infos['target']
+ laps = time.time()
+ cache = False
+ if fun in __CACHED_FUNS:
+ cache = True
+ laps = laps // __CACHED_FUNS[fun]
+ try:
+ sargs = json.dumps(args)
+ except Exception:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

I'm sure json has a specific exception in case of problematic dumps, please use that exception instead of Exception

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ infos = get_configured_provider()
+ if not infos:
+ return
+ target = infos['target']
+ laps = time.time()
+ cache = False
+ if fun in __CACHED_FUNS:
+ cache = True
+ laps = laps // __CACHED_FUNS[fun]
+ try:
+ sargs = json.dumps(args)
+ except Exception:
+ sargs = ''
+ try:
+ skw = json.dumps(kw)
+ except Exception:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Same as above, avoid using the most general exception unless really necessary.

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ laps = time.time()
+ cache = False
+ if fun in __CACHED_FUNS:
+ cache = True
+ laps = laps // __CACHED_FUNS[fun]
+ try:
+ sargs = json.dumps(args)
+ except Exception:
+ sargs = ''
+ try:
+ skw = json.dumps(kw)
+ except Exception:
+ skw = ''
+ try:
+ skwargs = json.dumps(kwargs)
+ except Exception:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Same as above, no need to repeat

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ wait_for_res -= 0.5
+ try:
+ cret = runner.cmd(
+ 'jobs.lookup_jid',
+ [jid, {'__kwarg__': True, 'output': False}])
+ if target in cret:
+ ret = cret[target]
+ break
+ # spetial case, some answers may be crafted
+ # to handle the unresponsivness of a specific command
+ # which is also meaningfull, eg a minion not yet provisionned
+ if fun in ['test.ping'] and not wait_for_res:
+ ret = {
+ 'test.ping': False,
+ }.get(fun, False)
+ except Exception:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Same as above. Salt should have a proper exception which should be caught here, if you find that it hasn't, we need to add it.

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ 'test.ping': False,
+ }.get(fun, False)
+ except Exception:
+ if wait_for_res:
+ pass
+ else:
+ raise
+ time.sleep(0.5)
+ try:
+ if 'is not available.' in ret:
+ raise SaltCloudSystemExit(
+ 'module/function {0} is not available'.format(fun))
+ except SaltCloudSystemExit:
+ raise
+ except Exception:
+ pass
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Really? ignore any uncaught exception?

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+def list_nodes(conn=None, call=None):
+ hide = False
+ names = __opts__.get('names', [])
+ profile = __opts__.get('profile', [])
+ destroy = __opts__.get('destroy', False)
+ action = __opts__.get('action', '')
+ for opt in ['full_query', 'select_query', 'query']:
+ if __opts__.get(opt, False):
+ call = 'full'
+ if action and not call:
+ call = 'action'
+ if profile and names and not destroy:
+ hide = True
+ if not get_configured_provider():
+ return
+ lxclist = _salt('lxc.list', **dict(extra=True))
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner
>>> def foo(*args, **kwargs):
...     print 'A:', args
...     print 'KW:', kwargs
...                                                                                                                                                                                                                                            
>>> foo('bar', **dict(extra=True))                                                                                                                                                                               
A: ('bar',)                                                                                                                                                                                                                                    
KW: {'extra': True}                                                                                                                                                                                                                            
>>> 

So, in fact, the call can simply be _salt('lxc.list', extra=True)... No?

>>> foo('bar', extra=True)
A: ('bar',)                                                                                                                                                                                                                                    
KW: {'extra': True}                                                                                                                                                                                                                            
>>> 

Or am I missing something?

@kiorky

kiorky Mar 5, 2014

Contributor

ditch left over, ...
done.

@s0undt3ch s0undt3ch commented on the diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ keys = ret['changes'].items()
+ keys.sort()
+ for ch, comment in keys:
+ sret += (
+ '\n'
+ ' {0}:\n'
+ ' {1}'
+ ).format(ch, comment.replace(
+ '\n',
+ '\n'
+ ' '))
+ if not ret['result']:
+ if 'changes' in ret:
+ del ret['changes']
+ raise SaltCloudSystemExit(sret)
+ log.info(sret)
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

While developing salt I've found that this kind of logging usage is only helpful when developing the feature that made us add the log call. After a while, when you see this kind of log messages, you're somewhat lost to why was that logged.

All this to say that, even though the log call will be a bit verbose, which might tell us why we're seeing it, it never hurts to use something like, log.info('LXC Checkpoint returned: \n{0}'.format(sret), or similar...

Please not that this is not a blocking problem, just something for us to keep in mind...

@kiorky

kiorky Mar 5, 2014

Contributor

checkpoint incrementally show the "big" steps in vm provisionning, that 's not really verbose ;)

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+def create(vm_, call=None):
+ '''Create an lxc Container.
+ This function is indopotent and will try to either provision
+ or finish the provision of an lxc container.
+ '''
+ mopts = _master_opts()
+ if not get_configured_provider(vm_):
+ return
+ __grains__ = _salt('grains.items')
+ name = vm_['name']
+ if not 'minion' in vm_:
+ vm_['minion'] = {}
+ minion = vm_['minion']
+
+ from_container = vm_.get('from_container', None)
+ image = vm_.get('image', 'ubuntu')
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

I would also default to None but I'm being picky here...

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch commented on the diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ vm_['minion'] = {}
+ minion = vm_['minion']
+
+ from_container = vm_.get('from_container', None)
+ image = vm_.get('image', 'ubuntu')
+ vgname = vm_.get('vgname', None)
+ backing = vm_.get('backing', None)
+ snapshot = vm_.get('snapshot', True)
+ profile = vm_.get('profile', None)
+ fstype = vm_.get('fstype', None)
+ dnsservers = vm_.get('dnsservers', [])
+ lvname = vm_.get('lvname', None)
+ ip = vm_.get('ip', None)
+ mac = vm_.get('mac', None)
+ netmask = vm_.get('netmask', '24')
+ bridge = vm_.get('bridge', 'lxcbr0')
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

I assume that this is the default bridge name in most of the HOWTO's out there?

@kiorky

kiorky Mar 5, 2014

Contributor

This is the default bridge in any lxc install yes

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2036/

@s0undt3ch s0undt3ch commented on the diff Mar 5, 2014

salt/cloud/clouds/lxc.py
+ gateway = vm_.get('gateway', 'auto')
+ size = vm_.get('size', '10G')
+ ssh_username = vm_.get('ssh_username', 'ubuntu')
+ sudo = vm_.get('sudo', True)
+ password = vm_.get('password', 'ubuntu')
+ lxc_conf_unset = vm_.get('lxc_conf_unset', [])
+ lxc_conf = vm_.get('lxc_conf', [])
+ stopped = vm_.get('stopped', False)
+ master = vm_.get('master', None)
+ script = vm_.get('script', None)
+ script_args = vm_.get('script_args', None)
+ users = vm_.get('users', None)
+ for k in ['password',
+ 'ssh_username']:
+ vm_[k] = locals()[k]
+
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Instead of commenting on all of your chosen defaults, I'm commenting on the end of them.

We should keep defaults somewhat generic(usable in any distro) or unset(to for the user to know what they are for and set them). By unset I mean None of course.

You seem to default to Ubuntu which I'm guessing is your development environment. This is not a blocking problem, ie, if your code is merged as it it, at some point someone will make it more generic. Still, it's something to keep in mind.

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch commented on an outdated diff Mar 5, 2014

salt/modules/cloud.py
@@ -28,7 +28,7 @@ def __virtual__():
Only work on POSIX-like systems
'''
if HAS_SALTCLOUD:
- return True
+ return 'cloud'
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Not renaming the module, so, True should be returned instead. Less time spent on loader logic.

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
if salt.utils.which('lxc-autostart'):
return 'lxc'
elif salt.utils.which('lxc-version'):
- log.warning('Support for lxc < 1.0 may be incomplete.')
+ passed = False
+ try:
+ passed = subprocess.check_output(
+ 'lxc-version').split(':')[1].strip() >= '1.0'
+ except Exception:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Use a subprocess exception here. I believe, CalledProcessError in this case.

@kiorky

kiorky Mar 5, 2014

Contributor

this code is in a docstring, i wont fix it :)

@s0undt3ch s0undt3ch commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
return 'lxc'
return False
+ '''
+ return 'lxc'
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

True should be returned here(and above but the previous coder also returned a string, so, not your fault 😄, but, since you're changing this code...)

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
@@ -476,6 +666,25 @@ def set_parameter(name, parameter, value):
return True
+def templates():
+ '''
+ Returns a list of existing templates
+
+ .. code-block:: bash
+
+ salt '*' lxc.templates
+ '''
+ templates = []
+ san = re.compile('^lxc-')
+ tdir = '/usr/share/lxc/templates'
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Is this the path where all templates are stored in all distributions?
This could either be configurable or allowed to be passed as a kwarg?

@kiorky

kiorky Mar 5, 2014

Contributor

No need, this is an lxc hardcoded thing.
Nevertheless I add a keyword

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
+ try:
+ cmd = (
+ "lxc-attach -n \"{0}\" -- "
+ " /bin/sh -c \""
+ "").format(name)
+ for i in users:
+ cmd += "echo {0}:{1}|chpasswd && ".format(
+ pipes.quote(i),
+ pipes.quote(password),
+ )
+ cmd += " /bin/true\""
+ cret = __salt__['cmd.run_all'](cmd)
+ if cret['retcode'] != 0:
+ raise Exception('Can\'t change passwords')
+ ret['comment'] = 'Password updated for {0}'.format(users)
+ except Exception, ex:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Again, don't catch Exception here, find out which are actually raised. In this case, I believe CommandExecutionError...

@kiorky

kiorky Mar 5, 2014

Contributor

Yes here i want a general catcher...

@kiorky

kiorky Mar 5, 2014

Contributor

done ...

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
+ if not val:
+ conf += '{0}\n'.format(k)
+ else:
+ conf += '{0} = {1}\n'.format(k.strip(), val.strip())
+ conf_changed = conf != orig_config
+ chrono = datetime.datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
+ if conf_changed:
+ wfic = open('{0}.{1}'.format(lxc_conf_p, chrono), 'w')
+ wfic.write(conf)
+ wfic.close()
+ wfic = open(lxc_conf_p, 'w')
+ wfic.write(conf)
+ wfic.close()
+ ret['comment'] = 'Updated'
+ ret['result'] = True
+ except Exception, ex:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

😄 Exception .....

@kiorky

kiorky Mar 5, 2014

Contributor

Yes here i want a general catcher...

@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Why is a general one needed here?

@kiorky

kiorky Mar 5, 2014

Contributor

Ok less safebelts ......
Done.

@s0undt3ch s0undt3ch commented on the diff Mar 5, 2014

salt/states/cloud.py
if info and not 'Error' in info:
ret['changes'] = info
ret['result'] = True
ret['comment'] = ('Created instance {0} using provider {1}'
- 'and the following options: {2}').format(
+ ' and the following options: {2}').format(

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/states/lxc.py
@@ -0,0 +1,321 @@
+#!/usr/bin/env python
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

There's no if __name__ == '__main__' in this module so, the above shebang can be removed...

@kiorky

kiorky Mar 5, 2014

Contributor

done

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/states/lxc.py
+ size=size,
+ vgname=vgname,
+ profile=profile,
+ )
+ if cret.get('error', ''):
+ ret['result'] = False
+ ret['comment'] = '{0}\n{1}'.format(
+ cret['error'], 'Container cloning error')
+ else:
+ ret['result'] = (
+ cret['cloned'] or 'already exists' in cret.get('comment', ''))
+ ret['comment'] += 'Container cloned\n'
+ changes['status'] = 'cloned'
+ return ret
+
+# vim:set et sts=4 ts=4 tw=80:
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

Every vim user likes his own vim settings don't you think?

@kiorky

kiorky Mar 5, 2014

Contributor

its when a create a new py files, removing...

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 5, 2014

salt/utils/cloud.py
@@ -1267,35 +1302,47 @@ def root_cmd(command, tty, sudo, **kwargs):
' '.join(ssh_args), kwargs, pipes.quote(command)
)
log.debug('SSH command: {0!r}'.format(cmd))
+ class PasswordError(Exception):
@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

I believe we should move this exception definition to salt/cloud/exceptions.py. What do you think @techhat?

@kiorky

kiorky Mar 5, 2014

Contributor

done

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2038/

Owner

s0undt3ch commented Mar 5, 2014

Ok, so, I've mentioned quite some stuff here, and @kiorky, yes, salt has more usage like this all over the place but we should reduce it whenever we can 😄

@techhat I can have another look at this PR when @kiorky says he's done(again 😄)

Contributor

kiorky commented Mar 5, 2014

im done

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2043/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2042/

Contributor

kiorky commented Mar 5, 2014

you can have another look @s0undt3ch ;)

Owner

techhat commented Mar 5, 2014

@kiorky, I'll be doing the merging on this one, so make sure you ping me too if we have to revisit more.

@techhat techhat and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/__init__.py
@@ -130,15 +251,19 @@ def select_query(self, query_type='list_nodes_select'):
mapper = salt.cloud.Map(self._opts_defaults())
return mapper.map_providers_parallel(query_type)
- def profile(self, profile, names, **kwargs):
+ def profile(self, profile, names, vm_opts=None, **kwargs):
@techhat

techhat Mar 5, 2014

Owner

I'm going to be nitpicky on this one, since we already use a variable called vm_ and one called opts. Is this meant to be a combination of the two? If so, why? And if not, then it needs to be renamed, to avoid confusion.

@kiorky

kiorky Mar 5, 2014

Contributor

No, that is not yet another dict of options ...
Kwargs are dedicated to mapper...
And even more it is options to be merged inside the local vm_ variable in the later function.

@techhat techhat and 1 other commented on an outdated diff Mar 5, 2014

salt/cloud/__init__.py
'''
Pass in a profile to create, names is a list of vm names to allocate
+
+ vm_opts is a spetial dict that will be per node options overrides
@techhat

techhat Mar 5, 2014

Owner

"special"

@kiorky

kiorky Mar 5, 2014

Contributor

done

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2045/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2044/

@techhat techhat commented on the diff Mar 5, 2014

salt/states/cloud.py
@@ -39,33 +49,56 @@ def present(name, provider, **kwargs):
name
The name of the instance to create
- provider
+ cloud_provider
@techhat

techhat Mar 5, 2014

Owner

This has already been established as provider. Changing it will break backwards compatibility, even if only with one released version, and does not make it more specific, seeing as we're already in the cloud state. Please change it back.

@kiorky

kiorky Mar 5, 2014

Contributor

provider in salt system deserves salt provider, so it is a non feature and true lie ;)

Did you saw the commit comments ?
See http://docs.saltstack.com/ref/states/providers.html
Having provider as state kwargs is just a bad thing

@techhat

techhat Mar 5, 2014

Owner

I concede and appreciate your point.

Contributor

kiorky commented Mar 5, 2014

@techhat @s0undt3ch review can be done

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2047/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2048/

Owner

techhat commented Mar 5, 2014

Thanks @kiorky. Looks like the current code failures are unrelated, and it looks like you've addressed the linting issues. We have a lot of changes here, and some of them are quite invasive, so I'm pretty scared... but I think I may be ready to merge this. But I will wait until after lunch, just in case something unexpected happens.

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2050/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2053/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2054/

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2056/

Contributor

kiorky commented Mar 5, 2014

hangon, i have a little feature to add to lxc driver before merge (in the late evening)

@mgwilliams mgwilliams and 1 other commented on an outdated diff Mar 5, 2014

salt/states/lxc.py
+ lvname=lvname,
+ backing=backing,
+ )
+ if cret.get('error', ''):
+ ret['result'] = False
+ ret['comment'] = cret['error']
+ else:
+ exists = (
+ cret['created']
+ or 'already exists' in cret.get('comment', ''))
+ ret['comment'] += 'Container created\n'
+ ret['changes']['status'] = 'Created'
+ return ret
+
+
+def cloned(name,
@mgwilliams

mgwilliams Mar 5, 2014

Contributor

Just a thought... would it make more sense to have a single present state that delegates to created and cloned?

@kiorky

kiorky Mar 5, 2014

Contributor

the provision which does that is on salt-cloud, @mgwilliams.

@kiorky

kiorky Mar 5, 2014

Contributor

it was does in state at a moment, but was removed and rebased for a long time :)

@mgwilliams mgwilliams and 2 others commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
def __virtual__():
+ '''
+ To speed up the whole thing, we decided to not use the
+ subshell way and assume things are in place for lxc
+ Discussion made by @kiorky and @thatch45
@mgwilliams

mgwilliams Mar 5, 2014

Contributor

Really? Is a call to salt.utils.which that expensive? I can see avoiding the call to lxc-version itself (which doesn't exist for lxc 1.0 release anyway). Without something, every minion will always report lxc as being available.

@kiorky

kiorky Mar 5, 2014

Contributor

yes shelling out to a subprocess is expensive, moreover with multiple reloads and a consequent number of minions.

@mgwilliams

mgwilliams Mar 5, 2014

Contributor

All the same, using salt.utils.which to determine whether an execution module should load is pretty standard in Salt (at, dig, djangomod, eix, etc). Again, is it desirable to have the lxc module loading on every minion, regardless of whether the lxc tools are actually available? If this shelling out is a real issue, maybe something could be devised whereby repeated loads do not shell out (e.g., set a grain and test that).

@kiorky

kiorky Mar 5, 2014

Contributor

Maybe @thatch45 will be more the person to convince about ;).

@s0undt3ch

s0undt3ch Mar 5, 2014

Owner

salt.utils.which does not shell out.

@kiorky

kiorky Mar 6, 2014

Contributor

pushing smthing

@kiorky

kiorky Mar 6, 2014

Contributor

pushed ;)

@mgwilliams mgwilliams and 1 other commented on an outdated diff Mar 5, 2014

salt/modules/lxc.py
+ ret['ipv4_ips'].append(ip_address)
+ if ip_address == '127.0.0.1':
+ ret['private_ips'].append(ip_address)
+ ret['private_ipv4_ips'].append(ip_address)
+ elif salt.utils.cloud.is_public_ip(ip_address):
+ ret['public_ips'].append(ip_address)
+ ret['public_ipv4_ips'].append(ip_address)
+ else:
+ ret['private_ips'].append(ip_address)
+ ret['private_ipv4_ips'].append(ip_address)
+ for k in [l for l in ret if l.endswith('_ips')]:
+ ret[k].sort(key=_ip_sort)
+ return ret
+
+
+def set_pass(name, users, password):
@mgwilliams

mgwilliams Mar 5, 2014

Contributor

I would suggest updating to allow chpasswd options, especially the option to provide pre-encrypted passwords:

def set_pass(name, users, password, encrypted=False, crypt_method=None):

From man chpasswd:

       -c, --crypt-method METHOD
           Use the specified method to encrypt the passwords.

           The available methods are DES, MD5, NONE, and SHA256 or SHA512 if your libc support these methods.

           By default, PAM is used to encrypt the passwords.

       -e, --encrypted
           Supplied passwords are in encrypted form.
@kiorky

kiorky Mar 5, 2014

Contributor

provide a PR once merged ;)

Contributor

salt-jenkins commented Mar 5, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2066/

@s0undt3ch s0undt3ch and 1 other commented on an outdated diff Mar 6, 2014

salt/cloud/__init__.py
import glob
import time
import signal
import logging
+from pprint import pprint
@s0undt3ch

s0undt3ch Mar 6, 2014

Owner

Are you using this pprint import?

@kiorky

kiorky Mar 6, 2014

Contributor

uhm, not sure anymore

@mgwilliams mgwilliams and 1 other commented on an outdated diff Mar 6, 2014

salt/modules/lxc.py
def __virtual__():
- if salt.utils.which('lxc-autostart'):
- return 'lxc'
+ if salt.utils.which('lxc-start'):
@mgwilliams

mgwilliams Mar 6, 2014

Contributor

Is this an improvement on what's already in the saltstack/develop branch? By checking both lxc-autostart (only in new versions) and lxc-version (not in 1.0), you get some version checking without shelling out. But I'm not 100% certain when lxc-autostart was added, so maybe the tests should be in the other order (lxc-version, display deprecation warning, otherwise check for lxc-somethingelse and return True)

@kiorky

kiorky Mar 6, 2014

Contributor

You cant test without shelling out to know the lxc version as 1.0alpha & betas have a mixed of those tools and will pollute logs with noisy falsepositives.

Contributor

salt-jenkins commented Mar 6, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2079/

Contributor

salt-jenkins commented Mar 6, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2092/

techhat merged commit 93d7ff2 into saltstack:develop Mar 6, 2014

1 check failed

default Merged build finished.
Details
Contributor

kiorky commented Mar 6, 2014

kiorky deleted the unknown repository branch Mar 6, 2014

Contributor

grischa commented on salt/utils/cloud.py in 7ca9fe5 Jul 28, 2014

kwargs contains no username or hostname or keyfile or password, because they are matched explicitly, so the command will fail.
I managed to make it work by changing line 696 to tty, sudo, username=username, hostname=host, key_filename=key_filename, password=password, **kwargs)
However, I am not sure whether that will work for all cases.

Contributor

grischa replied Jul 28, 2014

Issue here: #14544

Contributor

kiorky replied Jul 28, 2014

attention with this part of salt cloud, there may be a bug, but it is still a very fragile part of the stack.

Contributor

kiorky replied Jul 28, 2014

(@techhat and i are the ones to review such changes)

For your fix, it can collide with kwargs content, so you need to make another dict merging the args at least !

Contributor

cachedout replied Jul 28, 2014

Contributor

grischa replied Jul 29, 2014

I see now that my fix isn't good in general, better to change *_kwargs to *_ssh_kwargs. However that is undefined at function definition time, so the function needs to be moved.
However, even better might be to receive *_kwargs as an argument and call the function with *_ssh_kwargs, so that parameter passing is done explicitly not via context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment