Skip to content

Log error when reactor tasks go to a full queue#48775

Merged
cachedout merged 7 commits into
saltstack:developfrom
geekinutah:mwilson/log_reactor_queue_overflow
Aug 14, 2018
Merged

Log error when reactor tasks go to a full queue#48775
cachedout merged 7 commits into
saltstack:developfrom
geekinutah:mwilson/log_reactor_queue_overflow

Conversation

@geekinutah
Copy link
Copy Markdown
Contributor

Fixes #46431

What does this PR do?

Check return values of fire_async, the only possible return of False is if queue is full.

What issues does this PR fix or reference?

46431

Previous Behavior

Silently ignore failure to inject a task into a full queue

New Behavior

Only adds a log message

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

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

Copy link
Copy Markdown
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

One minor change needed in the logging.

blind-review

Comment thread salt/utils/reactor.py

if ret is False:
log.error('Reactor \'%s\' failed to execute %s \'%s\': '
'TaskPool queue is full!'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no space between this sentence and the next. A space should be added before Consider.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure I totally follow the logic of this. This looks more to me like it's evaluating the return from the wrapped function call itself where False could be a totally legitimiate value. What am I missing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, from what I can see, False is only returned when the queue is full. As per current behavior, we will right now silently fail to insert something on the queue. The only behavior I am adding is a log message when the queue is full. Otherwise, continue as per today.

@rallytime rallytime requested a review from terminalmage July 27, 2018 17:58
Copy link
Copy Markdown
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

blind-review

@cachedout cachedout merged commit 8d5e394 into saltstack:develop Aug 14, 2018
@geekinutah geekinutah deleted the mwilson/log_reactor_queue_overflow branch August 14, 2018 19:33
@techhat techhat mentioned this pull request Apr 21, 2020
dwoz added a commit that referenced this pull request Apr 21, 2020
@sagetherage sagetherage added the has master-port port to master has been created label Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-jam has master-port port to master has been created Tests-Passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrency issues in reactor & orchestration with long running orchestrations simultaneously targeting different minions

6 participants