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

Implement TCP transport reqchannel retry logic #59163

Merged
merged 7 commits into from Feb 24, 2021

Conversation

lukasraska
Copy link
Contributor

What does this PR do?

Implements retry mechanism for TCP transports ReqChannel as it's currently done for ZeroMQ.

What issues does this PR fix or reference?

Fixes #59162

Previous Behavior

Once first publish fails, event is lost forever

New Behavior

Event publish is retried multiple times

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@lukasraska lukasraska requested a review from a team as a code owner December 17, 2020 21:02
@lukasraska lukasraska requested review from krionbsd and removed request for a team December 17, 2020 21:02
@lukasraska
Copy link
Contributor Author

FYI @waynew.
I would love to get your insight into the test implementation, the currently implemented integration test requires setting fixed return_retry_timer in default conf files in order to have it at least a bit deterministic and I haven't found an easy option how to set specific opts for single test (except creating new fixture with own minion factory).

Thanks

@lukasraska
Copy link
Contributor Author

re-run pr-ubuntu1604-py3-pytest

@mattp-
Copy link
Contributor

mattp- commented Dec 29, 2020

nice 👍

@lukasraska
Copy link
Contributor Author

Should probably be merged after #59172 , so there is no conflict with tests

tests/pytests/integration/test_minion.py Outdated Show resolved Hide resolved
tests/pytests/integration/test_minion.py Outdated Show resolved Hide resolved
@s0undt3ch
Copy link
Member

s0undt3ch commented Jan 6, 2021

Also, fyi, #59172 has been merged.

@lukasraska
Copy link
Contributor Author

re-run pr-amazon2-py3-pytest

krionbsd
krionbsd previously approved these changes Jan 7, 2021
@mattp-
Copy link
Contributor

mattp- commented Feb 1, 2021

bump. any reason not to merge this?

@lukasraska
Copy link
Contributor Author

I expect due to the upcoming CVE release the merging has been postponed.

Anyway, I've rebased it, so it's up-to-date with master, updated the test with larger return_retry_tries (as the PR for that has been merged, so it could work much better without such large sleep) and changed the slowTest decorator to pytest mark, FYI @s0undt3ch

@lukasraska
Copy link
Contributor Author

re-run pr-centos7-py3-pytest

@lukasraska
Copy link
Contributor Author

re-run pr-freebsd122-py3-pytest

@lukasraska
Copy link
Contributor Author

re-run pr-centos7-py3-pytest

@waynew waynew added the Aluminium Release Post Mg and Pre Si label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Minion event publish doesn't retry on TCP transport
6 participants