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

Ensure a sensible timeout for pgsql commands #61433

Merged
merged 4 commits into from
Nov 1, 2022

Conversation

edevil
Copy link
Contributor

@edevil edevil commented Jan 9, 2022

The psql command has no default timeout, so if the server is not
responding or if some lock cannot be obtained, we will be waiting
forever.

What does this PR do?

Introduce a default timeout of 60s for all psql commands.

Previous Behavior

Module would wait forever for commands to return.

New Behavior

Module will wait at most 60s for psql command to complete.

Merge requirements satisfied?

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

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.

The psql command has no default timeout, so if the server is not
responding or if some lock cannot be obtained, we will be waiting
forever.

Introduce a default timeout of 60s for all psql commands.
@edevil edevil requested a review from a team as a code owner January 9, 2022 10:22
@edevil edevil requested review from krionbsd and removed request for a team January 9, 2022 10:22
@welcome
Copy link

welcome bot commented Jan 9, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@Ch3LL Ch3LL added the P1 Priority 1 label Sep 19, 2022
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Can this be made configurable with the default being 60 secs.
There should also be tests to check that the value is operational

Also needs a changelog entry (one liner describing the fix)

@dmurphy18 dmurphy18 added Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases Sulfur v3006.0 release code name and version labels Sep 22, 2022
@MKLeb
Copy link
Contributor

MKLeb commented Oct 10, 2022

@edevil Have you been able to look at implementing @dmurphy18 's suggestion?

@edevil
Copy link
Contributor Author

edevil commented Oct 10, 2022

Hey @MKLeb. Sorry but no, I've been a bit busy.

Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

This needs a test and a changelog

@Ch3LL Ch3LL removed Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases needs-changelog-file labels Nov 1, 2022
@Ch3LL Ch3LL merged commit 637530a into saltstack:master Nov 1, 2022
@welcome
Copy link

welcome bot commented Nov 1, 2022

Congratulations on your first PR being merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Priority 1 Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants