-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Matcher in loader, take 4 #48809
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
Matcher in loader, take 4 #48809
Conversation
@@ -203,8 +203,8 @@ def test_multi_local_async_post(self): | |||
self.assertEqual(len(ret), 2) | |||
self.assertIn('jid', ret[0]) | |||
self.assertIn('jid', ret[1]) | |||
self.assertEqual(ret[0]['minions'], sorted(['minion', 'sub_minion', 'localhost'])) | |||
self.assertEqual(ret[1]['minions'], sorted(['minion', 'sub_minion', 'localhost'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these getting removed?
This was added for testing the enable_ssh_minions
feature added in 2018.3.
It does need the ssh daemon started in runtests.py with python -m tests.runtests --ssh
, but those minions should still be returning from the tornado tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is what I needed to know, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking to Nicole, those tests are not the flaky ones that she marked in 2017.7, so they should still be passing.
These should be returning localhost, because it should be matching the minions for the enable_ssh_minions
targeting. in salt.utils.minions
https://github.com/saltstack/salt/blob/develop/salt/utils/minions.py#L715
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cro 💥 very cool! 💥
@rallytime looks like these tests are failing on the head of develop too? can you confirm? @cro revert the test changes, and I will fix them in a different PR, and approve this one. |
Trying the tests inside of test-kitchen to see if I can verify them in our test setup. |
The tests look better, and do not appear to be failing on the tornado tests anymore, rerunning the tests that did not post unittest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My big concern here is that these functions all take self
and then get converted on-the-fly to class-based methods from the context of the minion.
I presume that this was to avoid changing the interface to the Matchers from the minion's perspective but I'm not sure this is the right decision. IMHO, we should modify the minion-side logic so that it just calls functions. This is the pattern in every other pluggable system in Salt and I am very hesitant about diverging from that.
doc/topics/matchers/index.rst
Outdated
|
||
Matchers are modules that provide Salt's targeting abilities. As of the | ||
Flourine release, matchers can be dynamically loaded. Currently new matchers | ||
cannot be created, but existing matchers may have their functionality altered or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just because the CLI would need to be modified as well? I feel like this needs more explanation.
@cro I have changed the base branch of this PR from develop to fluorine for inclusion in the Fluorine feature release. Also, if you could make the changes requested above ASAP, that would be great so we can get this merged in. Thanks! |
Bump @cro |
So if I understand correctly @cachedout, you would like to see this behave more like executors, which are loaded specifically with a call to
|
6561a34
to
d62b87d
Compare
K, |
Tests may not pass fully yet, I have a mock or two I have to figure out what to do with. |
@cro Correct. I know that's a bit of a pain but the consistency will be a win in the long run. Thanks for doing this! |
@cro Also a syntax error snuck in: https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48809/13/warnings52Result/new/ |
salt/modules/match.py
Outdated
try: | ||
return matcher.pcre_match(tgt) | ||
return matcher['pcre_match.match'](tgt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be plural, I think.
@cro Any update here? We need these tests fixed before we can get this in. |
This is faster than splitting into a list and then iterating over it.
By using a generator comprehension, we avoid pre-allocating an entire list of objects.
This reverses the order in which matches are attempted so that we start with more deeply-nested matches first. The rationale for this is that it is much more likely that when one uses `foo:bar:baz:hello:world`, they are looking for a deeply-nested dictionary containing a key `hello` which maps to `world`, than a key named `foo` matching the string `bar:baz:hello:world`. The one case in which this change would be less performant would be in the case where the value being checked is a UNIX-style PATH (which uses colons as separators).
*Just* in case something else that invokes this starts using tuples at some point, it'll save us a traceback.
670f087
to
631eab3
Compare
@cro Getting closer, but there are still some tests that need to be fixed here: https://jenkinsci.saltstack.com/job/pr-kitchen-centos7-py3/job/PR-48809/24/ All of those test failures are related to the changes in this PR. |
salt/matchers/list_match.py
Outdated
or tgt.startswith(__opts__['id'] + ',') \ | ||
or tgt.endswith(',' + __opts__['id']) | ||
if isinstance(tgt, collections.Sequence) and not isinstance(tgt, six.string_types): | ||
result = bool(__opts__['id'] in tgt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__opts__['id'] in tgt
already returns a bool, so wrapping it in bool()
is redundant.
Also, we can skip the type checking altogether by just relying on exception handling to narrow down what type the target is. I've opened cro#16 with one possible way we could do this.
Make sequence optimization more efficient
localhost should not appear in minion list unless there is an exchanged key for it, which there is not
AWS is having some issues, but tests did pass on the one platform where they finished without SSH connection issues. This should be good to merge once we can get the test suite to run to completion. |
What does this PR do?
Creates a new directory
salt/matchers
and migrates the matcher functions into separate .py files in this directory. These functions are loaded at minion startup, but can be reloaded with the newsaltutil.refresh_matchers
function.Tests written?
Matcher tests apply here.
Commits signed with GPG?
Yes