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

Fix save_load which is never called for returner jobs #43454

Merged
merged 6 commits into from Oct 18, 2017

Conversation

Projects
None yet
7 participants
@kunal-bajpai
Copy link
Contributor

commented Sep 12, 2017

What does this PR do?

Fixes the the runner jobs.lookup_jid when a job cache other than local_cache is used. Requires a fix to the condition that is checked before save_load is run.

What issues does this PR fix or reference?

#43453

Previous Behavior

Runner job load not saved to job cache.

New Behavior

Runner job load is saved in job cache.

Tests written?

No. Tested locally.

Kunal Ajay Bajpai
@salt-jenkins

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2017

@kunal-bajpai, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ckochenower, @kiorky and @terminalmage to be potential reviewers.

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

On second thought, the check for getfstr is redundant as it is already done a couple lines before here

salt/salt/utils/job.py

Lines 93 to 100 in b4ca024

try:
savefstr_func = mminion.returners[savefstr]
getfstr_func = mminion.returners[getfstr]
fstr_func = mminion.returners[fstr]
except KeyError as error:
emsg = "Returner '{0}' does not support function {1}".format(job_cache, error)
log.error(emsg)
raise KeyError(emsg)

Replace the check for 'jid' key with a try/except block as it is more pythonic (follows EAFP)

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2017

@garethgreenaway @rallytime can you review this please? There is no workaround to this bug.

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

@garethgreenaway can you please help me get this reviewed? The bug makes external job cache unusable in its current state.

@@ -99,10 +99,10 @@ def store_job(opts, load, event=None, mminion=None):
log.error(emsg)
raise KeyError(emsg)

if 'jid' in load \
and 'get_load' in mminion.returners \

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Sep 28, 2017

Contributor

@kunal-bajpai if I'm right you're trying to fix the error in this check where we have to use and getfstr in minion.returners to ensure it's there before calling, but we're checking for 'get_load' which is never there.
If so please just fix this line, don't replace these checks with try. This will make your goal more clear.

This comment has been minimized.

Copy link
@kunal-bajpai

kunal-bajpai Sep 29, 2017

Author Contributor

salt/salt/utils/job.py

Lines 102 to 105 in b4ca024

if 'jid' in load \
and 'get_load' in mminion.returners \
and not mminion.returners[getfstr](load.get('jid', '')):
mminion.returners[savefstr](load['jid'], load)

There is an issue with the above construct and i'll try and explain each one separately:-

  1. and 'get_load' in mminion.returners \

    Not only does this check incorrectly look for 'get_load' instead of '{0}.get_load'.format(job_cache), this check is redundant as this is already checked for in the following construct, a few lines before this check appears

    salt/salt/utils/job.py

    Lines 93 to 100 in b4ca024

    try:
    savefstr_func = mminion.returners[savefstr]
    getfstr_func = mminion.returners[getfstr]
    fstr_func = mminion.returners[fstr]
    except KeyError as error:
    emsg = "Returner '{0}' does not support function {1}".format(job_cache, error)
    log.error(emsg)
    raise KeyError(emsg)
  2. and not mminion.returners[getfstr](load.get('jid', '')):

    This check is not required. It is intended to ensure that a document with key JID is not present in the job cache. However, the job of ensuring JIDs do not collide is of prep_jid which has been called before

    salt/salt/utils/job.py

    Lines 55 to 57 in b4ca024

    jidstore_fstr = '{0}.prep_jid'.format(job_cache)
    try:
    mminion.returners[jidstore_fstr](False, passed_jid=load['jid'])

    Both the above checks can thus safely be removed
    We are thus left with the following check only
if 'jid' in load:
    mminion.returners[savefstr](load['jid'], load)
  1. Finally, the above construct tries to LBYL which is the opposite of the EAFP principle of Python.
    An equivalent but more Pythonic construct is
try:
        mminion.returners[savefstr](load['jid'], load)
 except KeyError as e:
        log.error("Load does not contain 'jid': %s", e)

Only load['jid'] can generate a KeyError here as savefstr is also guaranteed to be present due to the earlier check

salt/salt/utils/job.py

Lines 93 to 100 in b4ca024

try:
savefstr_func = mminion.returners[savefstr]
getfstr_func = mminion.returners[getfstr]
fstr_func = mminion.returners[fstr]
except KeyError as error:
emsg = "Returner '{0}' does not support function {1}".format(job_cache, error)
log.error(emsg)
raise KeyError(emsg)

@DmitryKuzmenko the goal of this PR is to fix the bug by cleaning up the problematic construct.

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Sep 29, 2017

Contributor

@kunal-bajpai this makes sense. Thank you for explanation.

@saltstack saltstack deleted a comment from KumarRajnish Sep 28, 2017

@DmitryKuzmenko

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

re-run py3

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

@rallytime can you help review and merge this?

@rallytime
Copy link
Contributor

left a comment

This looks good to me. Thank you for the detailed explanation!

However, I would also like @cachedout to review this as well.

@rallytime rallytime requested a review from cachedout Oct 4, 2017

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

@cachedout can you review this please?

@kunal-bajpai

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

@thatch45 @cachedout @rallytime this PR has been in review for nearly a month now. This is a critical fix for making masters stateless and is a blocker for us at Linkedin. I cannot have it miss the bus for Oxygen release. Please let me know if there is anything I can do to expedite the review process.

@thatch45

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

Lets get this thing merged then!

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

re-run py3

@rallytime rallytime merged commit 3d5e3e5 into saltstack:develop Oct 18, 2017

5 of 8 checks passed

GPG All commits must have a verified GPG signature
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #3024 — FAILURE
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #18517 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #11043 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #15732 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #15601 — SUCCESS
Details
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

Thanks @kunal-bajpai for submitting this fix

rallytime added a commit to rallytime/salt that referenced this pull request Oct 31, 2018

Fix issue saltstack#48734
A regression was caused in the local_cache returner with PR saltstack#43454.

PR saltstack#43454 fixes an issue where `jobs.lookup_jid` was not working correctly
with external job caches.

However, when this issue was fixed, there was a slight logic error that
was overlooked for the local_cache. This fixes the issue with the local_cache
so that the "Target" and "Target Type" data is restored to their previous
values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.