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.

Sign up for GitHub

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

Jump to bottom

Fix issue with salt-run jobs.list_jobs where Target: unknown-target #50328

Merged
merged 5 commits into from Nov 13, 2018
Merged

Fix issue with salt-run jobs.list_jobs where Target: unknown-target #50328

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6859e0e
Add regression test for issue #48734
Oct 31, 2018
c684c29
Fix issue #48734
Oct 31, 2018
b29fcfa
New test must be py3 compatible...oops. :)
Oct 31, 2018
caace3f
Rewrite job cache target test to be more robust
Nov 7, 2018
2c51cf9
Make target test job more specific to reduce test flakiness
Nov 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions salt/utils/job.py
View file
Open in desktop
Expand Up @@ -103,10 +103,12 @@ def store_job(opts, load, event=None, mminion=None):
log.error(emsg)
raise KeyError(emsg)

try:
mminion.returners[savefstr](load['jid'], load)
except KeyError as e:
log.error("Load does not contain 'jid': %s", e)
if job_cache != 'local_cache':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the local cache different? Trying to solve this at this level doesn't feel like the right approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would normally agree with you, but this was how this worked in the local cache before the logic change in #43454. This was the least invasive way to fix this without going down many local_cache rabbit holes which could potentially break many other things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Thanks for the explanation.

try:
mminion.returners[savefstr](load['jid'], load)
except KeyError as e:
log.error("Load does not contain 'jid': %s", e)

mminion.returners[fstr](load)

if (opts.get('job_cache_store_endtime')
Expand Down
33 changes: 33 additions & 0 deletions tests/integration/runners/test_jobs.py
View file
Open in desktop
Expand Up @@ -37,3 +37,36 @@ def test_list_jobs(self):
'''
ret = self.run_run_plus('jobs.list_jobs')
self.assertIsInstance(ret['return'], dict)


class LocalCacheTargetTest(ShellCase):
'''
Test that a job stored in the local_cache has target information
'''

def test_target_info(self):
'''
This is a test case for issue #48734

PR #43454 fixed an issue where "jobs.lookup_jid" was not working
correctly with external job caches. However, this fix for external
job caches broke some inner workings of job storage when using the
local_cache.

We need to preserve the previous behavior for the local_cache, but
keep the new behavior for other external job caches.

If "savefstr" is called in the local cache, the target data does not
get written to the local_cache, and the target-type gets listed as a
"list" type instead of "glob".

This is a regression test for fixing the local_cache behavior.
'''
ret = self.run_run_plus('jobs.list_jobs')
ret_key = list(ret['return'].keys())[0]
tgt = ret['return'][ret_key]['Target']
tgt_type = ret['return'][ret_key]['Target-type']

assert tgt != 'unknown-target'
assert tgt in ['minion', 'sub_minion']
assert tgt_type == 'glob'