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

Stale events 53411 #53412

Merged
merged 6 commits into from Jun 28, 2019
Merged

Stale events 53411 #53412

merged 6 commits into from Jun 28, 2019

Conversation

@cro
Copy link
Member

@cro cro commented Jun 7, 2019

What does this PR do?

Adds event_listen_queue_max_seconds to fix #53411

What issues does this PR fix or reference?

#53411

Previous Behavior

If event_listen_queue is set to large values or if it is set but the event bus is not busy enough, events can languish in the queue and cause pathological behavior (CLI reporting Minion did not return, job results taking too long to return.

New Behavior

The event_listen_queue will be flushed if the oldest event in the queue is older than event_listen_queue_max_seconds regardless of the number of events in the queue.

Commits signed with GPG?

Yes

@cro cro requested a review from as a code owner Jun 7, 2019
@cro cro changed the base branch from develop to 2018.3 Jun 7, 2019
@dwoz
Copy link
Contributor

@dwoz dwoz commented Jun 7, 2019

Looks good but we are waiting on some tests for this change.

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

The extraneous conditional
else:
too_long_in_queue = False

might be unneeded, since line 1272 resets to False, unless I missed the need for it, in which case let me know what I missed.
otherwise looks good,

@cro
Copy link
Member Author

@cro cro commented Jun 18, 2019

After banging my head against this for some time I think we will need to resort to a manual test.

  1. The test suite currently starts a master but never restarts one so unless we make this config the default there's no way to alter the configuration
  2. There are no tests (that I can find) in the suite that verify functionality of non-default master config options.

If there is an approach I am unaware of I'm happy to explore it.

@waynew
Copy link
Contributor

@waynew waynew commented Jun 18, 2019

Both of those points sound accurate to me.

I would love it if we had an automated suite that roughly does what we do with the current integration suite, but instead was more like just running a salt orchestration, and we verified that each of the state result was what we expected. Basically an automated manual test suite 😂

@cro
Copy link
Member Author

@cro cro commented Jun 18, 2019

K, fixed the doc test failure I think.

@cro cro force-pushed the stale-events-53411 branch from 4fee5a2 to 9c03423 Jun 18, 2019
@cro
Copy link
Member Author

@cro cro commented Jun 18, 2019

And rebased against 2018.3. I think this will be ready for merge if all the tests pass again.

@waynew
Copy link
Contributor

@waynew waynew commented Jun 18, 2019

@cro I think we also need some tests, or is this fixing some existing tests?

@cro
Copy link
Member Author

@cro cro commented Jun 18, 2019

I would like to write tests, but according to my understanding of the test suite as it stands:

After banging my head against this for some time I think we will need to resort to a manual test.

  1. The test suite currently starts a master but never restarts one so unless we make this config the default there's no way to alter the configuration
  2. There are no tests (that I can find) in the suite that verify functionality of non-default master config options.

If there is an approach I am unaware of I'm happy to explore it.

Copy link
Member

@thatch45 thatch45 left a comment

I approve this merge as an exception to the tests writing rule.

@cro
Copy link
Member Author

@cro cro commented Jun 28, 2019

Re-run all

@cro cro merged commit 3270c5b into saltstack:2018.3 Jun 28, 2019
29 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants