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

Increase ping timeout based on ping count and interval #1305

Closed
wants to merge 5 commits into from

Conversation

kodek
Copy link
Contributor

@kodek kodek commented Jun 1, 2016

This should fix a bug where the ping command will time out when specifying ping count > 1 with a long interval. Feel free to rewrite and/or add tests if necessary.

@sparrc
Copy link
Contributor

sparrc commented Jun 1, 2016

seems more like user-error to me, why don't you just increase the timeout?

I'd prefer the plugin to behave the same as the ping unix command.

@sparrc sparrc closed this Jun 1, 2016
@kodek
Copy link
Contributor Author

kodek commented Jun 1, 2016

The timeout parameter is used both for the -W argument in ping (a per-ping timeout, "time to wait for a response" from the man pages) and the total ping command exec timeout (via internal.CombinedOutputTimeout()). You can reproduce the bug by passing in the following parameters:

count = 4
ping_interval = 15.0
timeout = 1.0

It will fail due to "Command timed out."

The corresponding ping command does not time out when sent from a terminal:
$ ping -c 4 -n -s 16 -i 15 -W 1 google.com

Timeout should be one "-W timeout" per count, plus one interval between each count.
@sparrc
Copy link
Contributor

sparrc commented Jun 1, 2016

hmm I see. I think that this plugin was meant to be using the -w flag instead of -W. I'm not sure whether we should change that to -w or patch the current behavior as your PR does.

@sparrc sparrc reopened this Jun 1, 2016
@kodek
Copy link
Contributor Author

kodek commented Jun 1, 2016

I see that some checks failed (maybe just formatting-related?). Could I defer any further changes to you?

Regarding -w vs -W, either approach is fine. If you decide to use -w, could you add a comment on the sample config or the type definition?

Thank you!

@sparrc
Copy link
Contributor

sparrc commented Jun 1, 2016

could you just run go fmt ./... on the code? it's a standard Go practice and needs to be done before tests can run

@sparrc
Copy link
Contributor

sparrc commented Jun 10, 2016

closing for #1359

@sparrc sparrc closed this Jun 10, 2016
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