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

allow using proxy minion from cli #48344

Merged
merged 14 commits into from Aug 1, 2018
Merged

Conversation

@gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Jun 27, 2018

What does this PR do?

This allows using proxy minions from the minion box, for troubleshooting, and also not needing a master.

[root@salt salt]# tail -c +0 /etc/salt/proxy /srv/pillar/*
==> /etc/salt/proxy <==
master: localhost

==> /srv/pillar/proxy.sls <==
proxy:
  proxytype: docker
  name: {{grains.id}}

==> /srv/pillar/top.sls <==
base:
  '*':
    - proxy
[root@salt salt]# salt-call --local grains.get virtual_subtype
local:
[root@salt salt]# docker ps
CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS               NAMES
f0b4c86881cb        centos              "/usr/lib/systemd/sy…"   3 hours ago         Up 3 hours                              quirky_edison
[root@salt salt]# salt-call --local grains.get virtual_subtype --proxyid quirky_edison
local:
    Docker

Tests written?

Not Yet

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested a review from as a code owner Jun 27, 2018
@ghost ghost self-requested a review Jun 27, 2018
@gtmanfred
Copy link
Contributor Author

@gtmanfred gtmanfred commented Jun 27, 2018

@mirceaulinic you will be interested in this.

@gtmanfred gtmanfred requested a review from cro Jun 27, 2018
Copy link
Contributor

@isbm isbm left a comment

@gtmanfred great work! And tests?.. 😉

allow_missing_funcs = any([
self.minion.executors['{0}.allow_missing_func'.format(executor)](function_name)
for executor in executors
if '{0}.allow_missing_func' in self.minion.executors
Copy link
Contributor

@isbm isbm Jun 28, 2018

Nitpick: I would still put this into one line with for loop, as it looks a bit odd, sort of wrong syntax. 😉

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 28, 2018

Yeah, most of this still needs to be cleaned up, a lot of it is copied from elsewhere, and just needs to be made into functions in salt.utils, so they can be used in both places.

salt/minion.py Outdated

if 'proxy' not in self.opts['pillar'] and 'proxy' not in self.opts:
errmsg = 'No proxy key found in pillar or opts for id ' + self.opts['id'] + '. ' + \
'Check your pillar/opts configuration and contents. Salt-proxy aborted.'
Copy link
Contributor

@isbm isbm Jun 28, 2018

Ermm... why not:

errmsg = 'No proxy key found in pillar or opts for id {id}. ' \
         'Check your pillar/options configuration and contents. ' \
         'Salt-proxy aborted.'.format(id=self.opts['id'])

But this is about formatting. The message itself is cryptic and misleading. Users, who has no idea of the Salt internals and programming will start thinking about AES keys. Please rephrase it for them.

salt/minion.py Outdated
'Check your pillar/opts configuration and contents. Salt-proxy aborted.'
log.error(errmsg)
self._running = False
raise SaltSystemExit(code=-1, msg=errmsg)
Copy link
Contributor

@isbm isbm Jun 28, 2018

What is -1? We have our own exit codes and we should stick to that, reusing constants.

import salt.defaults.exitcodes
raise SaltSystemExit(salt.defaults.exitcodes.EX_GENERIC, msg=errmsg)

Or add to exit codes new constant and reuse it above, e.g.:

EX_PROXY_RELOAD = 200  # All proxy-related errors can be 200-255, for example

salt/minion.py Outdated
raise SaltSystemExit(code=-1, msg=errmsg)

if 'proxy' not in self.opts:
self.opts['proxy'] = self.opts['pillar']['proxy']
Copy link
Contributor

@isbm isbm Jun 28, 2018

Hmm, this check if... will make __opts__ more important than pillars, which isn't right thing. Meaning, you cannot push proxy data to the running minion without it being restarted.

@terminalmage we wanted to change that also in config getter, right?

Copy link
Member

@mirceaulinic mirceaulinic Jun 28, 2018

The proxy block into the Pillar is mandatory for the Proxy Minion to start (i.e., it won't be able to start without). With this in mind, the Proxy Minion startup is different than the regular Minion, as the data is compiled twice: once to be able to get it up and running, then to make it available and re-compiled after the Grains are collected. It's a very complex process, as the Grains require the Proxy to be up (which needs the Pillar data in order to know where to connect to), and some other Pillars may require Grains. So please bear in mind that this is a different story.

salt/minion.py Outdated
self.opts = salt.utils.dictupdate.merge(self.opts,
self.opts['pillar'],
strategy=self.opts.get('proxy_merge_pillar_in_opts_strategy'),
merge_lists=self.opts.get('proxy_deep_merge_pillar_in_opts', False))
Copy link
Contributor

@isbm isbm Jun 28, 2018

I actually never understand why static, firm, rigid, rarely changed opts are over dynamic, volatile, flexible, often changed pillars. It just makes no sense. Imagine you could just push temporarily to your minion's config, do something and return back to the default state. But here now you have it totally stuck, unless you reconfigure it and restart it.

I would propose here to do the other way around: always merge pillars over opts. And add a special opt (again, you can deliver it over pillars and even control that remotely!!!) that would disable that.

But this is probably a subject to discuss...

salt/minion.py Outdated
if not HAS_PSUTIL:
log.error('Unable to enforce modules_max_memory because psutil is missing')
if not HAS_RESOURCE:
log.error('Unable to enforce modules_max_memory because resource is missing')
Copy link
Contributor

@isbm isbm Jun 28, 2018

Can we rephrase these messages to be user-compatible? ".....psutil Python package dependency is not installed". Same to "resource".

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 28, 2018

Yeah, this isn't going to survive the cleanup, because it is a one off process, and this stuff is not needed.

}
data.update(kwargs)
executors = getattr(self.sminion, 'module_executors', []) or \
opts.get('module_executors', ['direct_call'])
Copy link
Contributor

@isbm isbm Jun 28, 2018

It also can be cleaner here as well:

executors = getattr(self.sminion, 'module_executors',
                    opts.get('module_executors', ['direct_call']))

UPDATE:
Ah, I wasn't correct. This won't work (as the other place too) if there is module_executors attribute to the sminion, but is empty. However, your version will also omit module_executors from opts, if there are any in both cases.

So just pointing this out, the rest is up to you. 😉

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 28, 2018

So, there are certain instances when module_executors is specified on the proxy minion, and that is the module_executor that needs to be used. If that is not specified, then the user is free to specify their own module_executors.

So this is in the correct order.

Copy link
Contributor

@isbm isbm Jun 29, 2018

That's clear, but if module_executor is specified in both places, then the opts are discarded. Shouldn't we have to merge that instead (simply + instead of or)?

if mopts:
self.opts = mopts
else:
self.opts = salt.config.proxy_config(c_path)
Copy link
Contributor

@isbm isbm Jun 28, 2018

You use this idiom already, so why not here too:

self.opts = mopts or salt.config.proxy_config(c_path)

}
data.update(kwargs)
executors = getattr(self.minion, 'module_executors', []) or \
self.opts.get('module_executors', ['direct_call'])
Copy link
Contributor

@isbm isbm Jun 28, 2018

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 28, 2018

allow_missing_funcs = any([
self.sminion.executors['{0}.allow_missing_func'.format(executor)](function_name)
for executor in executors
if '{0}.allow_missing_func' in self.sminion.executors
Copy link
Contributor

@isbm isbm Jun 28, 2018

Maybe one line.

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 28, 2018

This actually shouldn't be needed at all, just a result of copying everything over.

@gtmanfred
Copy link
Contributor Author

@gtmanfred gtmanfred commented Jun 28, 2018

       ================================================================================
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       Starting Proxy Tests
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       test_can_it_ping (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:73.5%|MEM:73.7%]  ...  ok
       test_grains_items (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:46.3%|MEM:73.5%]  ...  ok
       test_list_pkgs (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:27.1%|MEM:74.7%]  ...  ok
       test_service_get_all (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:33.2%|MEM:76.5%]  ...  ok
       test_service_list (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:31.5%|MEM:76.7%]  ...  ok
       test_service_start (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:51.9%|MEM:78.9%]  ...  ok
       test_upgrade (integration.proxy.test_shell.ProxyCallerSimpleTestCase)
       [CPU:38.4%|MEM:80.2%]  ...  ok
       test_can_it_ping (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:27.3%|MEM:80.4%]  ... ok
       test_grains_items (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:19.8%|MEM:80.4%]  ... ok
       test_install_pkgs (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:16.2%|MEM:80.4%]  ... ok
       test_list_pkgs (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:26.6%|MEM:80.5%]  ... ok
       test_remove_pkgs (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:25.6%|MEM:80.5%]  ... ok
       test_service_get_all (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:25.9%|MEM:80.5%]  ... ok
       test_service_list (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:16.7%|MEM:80.5%]  ... ok
       test_service_start (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:16.7%|MEM:80.5%]  ... ok
       test_service_stop (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:20.8%|MEM:80.5%]  ... ok
       test_state_apply (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:17.1%|MEM:80.5%]  ... ok
       test_state_highstate (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:20.1%|MEM:80.5%]  ... ok
       test_upgrade (integration.proxy.test_simple.ProxyMinionSimpleTestCase)
       [CPU:25.9%|MEM:80.5%]  ... ok

       ----------------------------------------------------------------------
       Ran 19 tests in 120.612s

       OK

       ============================  Overall Tests Report  ============================
       ***  No Problems Found While Running Tests  ************************************
       ================================================================================
       OK (total=19, skipped=0, passed=19, failures=0, errors=0)
       ============================  Overall Tests Report  ============================

This is the output of the added tests when running proxy tests.

Copy link
Contributor

@isbm isbm left a comment

Great cleanup! I still have few little nitpicks, if you don't mind.

@@ -99,7 +99,10 @@ def __init__(self, opts):
# be imported as part of the salt api doesn't do a
# nasty sys.exit() and tick off our developer users
try:
self.minion = salt.minion.SMinion(opts)
if self.opts.get('proxyid', None) is not None:
Copy link
Contributor

@isbm isbm Jun 29, 2018

Let's hope nothing can set proxyid as 0 or "".

executors = getattr(self.sminion, 'module_executors', []) or \
opts.get('module_executors', ['direct_call'])
if isinstance(executors, six.string_types):
executors = [executors]
Copy link
Contributor

@isbm isbm Jun 29, 2018

Well, that's an interesting question. Shouldn't we just disallow wrong syntax in config and also make sure that executors are always a list? The variable name suggests it though. Personally I am not very much into favour of auto-fixing shoddy configs, because it will also grow problems like "oh, I was writing config also in that way and it worked before, but now not, fixitpls!".

I'd propose to actually crash here and politely ask to write a proper config. 😉

Copy link
Contributor Author

@gtmanfred gtmanfred Jun 29, 2018

This is here because it is easier to pass in --module-executors direct_call, then to have to pass it as a list on the commandline.

Also, this is the same format it is for the salt command.

Copy link
Contributor

@isbm isbm Jul 2, 2018

Hmm, but you actually can pass a list in command line (comma-separated or so). That's why I would propose to always convert it to the list at the very beginning, even if it is just one element there. Otherwise we will have this kinda spaghetti everywhere.

@@ -37,7 +37,10 @@ def is_proxy():
try:
# Changed this from 'salt-proxy in main...' to 'proxy in main...'
# to support the testsuite's temp script that is called 'cli_salt_proxy'
if 'proxy' in main.__file__:
#
# (gtmanfred) Add '--proxyid' in sys.argv so that salt-call --proxyid
Copy link
Contributor

@isbm isbm Jun 29, 2018

But Git blame says who wrote this comment, no need to extra-add it. 😉

@@ -0,0 +1,65 @@
# -*- coding: utf-8 -*-
'''
:codeauthor: Thayne Harbaugh (tharbaug@adobe.com)
Copy link
Contributor

@isbm isbm Jun 29, 2018

The test_proxy.py copypaste leftovers. 😉

Copy link
Contributor

@rallytime rallytime left a comment

This is neat!

I have a couple of small requests.

@@ -88,6 +88,12 @@ Salt Caller
.. autoclass:: salt.client.Caller
:members: cmd

Salt Proxy Caller
-----------
Copy link
Contributor

@rallytime rallytime Jul 3, 2018

The dashed line needs to match the length of the line above, or we'll get an RST warning.

salt/minion.py Outdated
class SProxyMinion(SMinion):
'''
Create an object that has loaded all of the minion module functions,
grains, modules, returners etc. The SMinion allows developers to
Copy link
Contributor

@rallytime rallytime Jul 3, 2018

Copy/Paste error? SMinion --> SProxyMinion?

.. code-block:: bash
salt '*' sys.reload_modules
Copy link
Contributor

@rallytime rallytime Jul 3, 2018

gen_modules here instead of reload_modules?

Copy link
Contributor Author

@gtmanfred gtmanfred Jul 3, 2018

Nah, reload_modules is correct, let me fix the code so that gen_modules overwrites reload_modules like it does in the regular salt-call

@rallytime rallytime requested a review from mirceaulinic Jul 3, 2018
isbm
isbm approved these changes Jul 3, 2018
cro
cro approved these changes Jul 3, 2018
@gtmanfred gtmanfred changed the title [WIP] allow using proxy minion from cli allow using proxy minion from cli Jul 3, 2018
@rallytime rallytime requested a review from cachedout Jul 3, 2018
Copy link
Member

@mirceaulinic mirceaulinic left a comment

That's awesome! Thanks @gtmanfred!

Copy link
Contributor

@cachedout cachedout left a comment

Request for additional documentation before we merge this.


def _mixin_setup(self):
self.add_option(
'--module-executors',
Copy link
Contributor

@cachedout cachedout Jul 18, 2018

IMHO we need more documentation than this about what's happening here. I'm certain that I couldn't figure out what this does from the perspective of the average Salt user.

Copy link
Contributor Author

@gtmanfred gtmanfred Jul 18, 2018

This is already documented for the salt command, it would be the same thing.

https://docs.saltstack.com/en/latest/ref/executors/index.html

I can add an example with salt-call --module-executors in here?

Copy link
Contributor Author

@gtmanfred gtmanfred Jul 27, 2018

Looking for feedback on my response.

Copy link
Contributor

@cachedout cachedout Jul 27, 2018

That's fine. We can rethink this down the road because it's a larger problem than just this particular flag.

@rallytime rallytime requested a review from cachedout Jul 27, 2018
@rallytime rallytime merged commit 60bbdee into saltstack:develop Aug 1, 2018
4 of 9 checks passed
@gtmanfred gtmanfred deleted the proxycaller branch Aug 1, 2018
terminalmage added a commit to terminalmage/salt that referenced this issue Sep 6, 2018
Pull saltstack#48344 added logic to salt-call which runs the desired function using all
of the executors, or just the direct_call one if none are configured.

However, the previous code which ran the function was not removed, which
results in the function being run twice.

This removes the (now) redundant function call.
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

6 participants