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

Check timeout on ssh calls (bsc#1213550) #601

Open
wants to merge 1 commit into
base: openSUSE/release/3006.0
Choose a base branch
from

Conversation

nadvornik
Copy link
Contributor

What does this PR do?

What issues does this PR fix or reference?

Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1213550

Previous Behavior

When executing ssh or scp command, the timeout was checked only on connection.
When the command got stuck after connecting, it could run forever.

New Behavior

Total execution time is limited.

Merge requirements satisfied?

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

Commits signed with GPG?

Yes/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.

@agraul
Copy link
Member

agraul commented Sep 7, 2023

I am a bit concerned about introducing this type of timeout. It's hard to find a good value for it, since we don't know what will be executed via Salt SSH and how long that will take. We don't want to run into a timeout when state.apply is doing its things as expected. But using a value that's too large won't help us much either.

In Uyuni we use SALT_SSH_CONNECT_TIMEOUT = 180 which would time out after 6 minutes with this implementation. That might cause quite a number of false positives.

We probably want to have this discussion upstream. If this is accepted, there are additional changes needed (e.g. updating the documentation and tests).

@agraul
Copy link
Member

agraul commented Sep 7, 2023

I just had a discussion with @mcalmer about this and we both think that a general timeout does not make sense. We just can't predict what someone wants to execute over Salt SSH and whether the execution is slow or came to a stall.

If it is possible to detect that nothing is being transferred in the scp case, that would be a better alternative. Maybe ClientAliveInterval helps, but we'd need to test that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants