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

Better handle errors in entry points #50632

Merged
merged 6 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@astronouth7303
Copy link
Contributor

commented Nov 26, 2018

What does this PR do?

Fixes entry point loading, so that if it fails, the exception is logged instead of propagated to the larger system.

What issues does this PR fix or reference?

None.

Previous Behavior

The minion would fail to start if there was any kind of problem with entry point loading.

New Behavior

The exception is logged instead.

Tests written?

No, and I'm not sure how or where.

Commits signed with GPG?

Yes.

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@astronouth7303 astronouth7303 requested a review from saltstack/team-core as a code owner Nov 26, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Nov 26, 2018

@astronouth7303 astronouth7303 force-pushed the astronouth7303:loader-exception-handler branch from 0a4367f to fb05bd5 Nov 26, 2018

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 26, 2018

Is there a bug filed against this issue? I have never heard of this failing before and I am curious as to why it would do so. I'd love to see the exact stacktrace here.

@astronouth7303

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

I found problems where if you have a bad entry point declared (in my case, because of copy/paste errors and then the code wasn't being packaged), then salt won't start. There's also weird things that happen if there's a problem in the called function.

You probably hadn't heard of anyone encountering this problem because this feature is completely undocumented (see #50633) and nobody has used it as far as I know. (Except now, because this is super helpful for my own projects.)

for path in loaded_entry_point():
ext_type_types.append(path)
except Exception:
log.exception("Error running entry point %r", entry_point)

This comment has been minimized.

Copy link
@isbm

isbm Nov 27, 2018

Contributor

This error message needs to be rephrased for a regular DevOp, who has no idea what means "entry point" here and why it should run.

Also I believe it should be at error level. We want traceback only in debug mode.

This comment has been minimized.

Copy link
@astronouth7303

astronouth7303 Nov 28, 2018

Author Contributor

I'll see what I can do. I'm not sure the names that the salt administrator would be familiar are readily available. I'll have to dig into the data types.

You know that exception is error level?

This comment has been minimized.

Copy link
@isbm

isbm Nov 28, 2018

Contributor

You know that exception is error level?

At error. And it gives you a traceback. Do we actually need it? Your error is only informative that failed to run an entry point 😉 (please clarify what means those running entry points!). So I'd rather give a traceback here only when one is really looking at it:

try:
 ...
except MySpecificException as exc:
    log.error('An entry point just ran away from you!')
except Exception:
    log.error('Oh, this neatly crashed.')
    log.debug('This is how it crashed. Enjoy your day.', exc_info=True)

This comment has been minimized.

Copy link
@astronouth7303

astronouth7303 Nov 28, 2018

Author Contributor

The developer would certainly strongly appreciate the traceback, and I would consider any time this code is triggered to be a bug in some third-party code.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

@astronouth7303 Thanks for the explanation. This is a reasonably safe change for us so we're happy to get this in but I do think you should make the changes that @isbm recommends before we merge it.

astronouth7303 added some commits Nov 28, 2018

@astronouth7303

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Ok, logging is improved.

@@ -138,6 +138,18 @@ def static_loader(
return ret


def _format_entrypoint_target(ep):
"""

This comment has been minimized.

Copy link
@cachedout

cachedout Nov 29, 2018

Collaborator

The Salt style guide indicates that these should be three single-quotes.

@isbm

isbm approved these changes Nov 29, 2018

astronouth7303 added some commits Nov 30, 2018

@cachedout cachedout merged commit 22acaaa into saltstack:2017.7 Nov 30, 2018

10 checks passed

WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details

@astronouth7303 astronouth7303 deleted the astronouth7303:loader-exception-handler branch Nov 30, 2018

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.