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.

Sign up for GitHub

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

Jump to bottom

Fix ping crash on Enimga2 linux distribution (mipsle) #3877

Merged
merged 3 commits into from
May 1, 2018
Merged

Fix ping crash on Enimga2 linux distribution (mipsle) #3877

merged 3 commits into from
May 1, 2018

Conversation

marianob85
Copy link
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Ping command crash on enimga2 stb ( linux mipsle ).

  • Fix problem with output parsing
  • Fix problem with arguments

danielnelson
danielnelson reviewed Mar 9, 2018
View reviewed changes
plugins/inputs/ping/ping.go
@@ -171,14 +171,24 @@ func (p *Ping) args(url string) []string {
// Build the ping command args based on toml config
args := []string{"-c", strconv.Itoa(p.Count), "-n", "-s", "16"}
if p.PingInterval > 0 {
args = append(args, "-i", strconv.FormatFloat(p.PingInterval, 'f', 1, 64))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just change this to use the -1 precision:

args = append(args, "-i", strconv.FormatFloat(p.PingInterval, 'f', -1, 64))

It still will require users to specify an even value, but I think it is better than switching on arch.

plugins/inputs/ping/ping.go Outdated
}
if p.Timeout > 0 {
switch runtime.GOOS {
case "darwin":
args = append(args, "-W", strconv.FormatFloat(p.Timeout*1000, 'f', 1, 64))
case "linux":
args = append(args, "-W", strconv.FormatFloat(p.Timeout, 'f', 1, 64))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use -1 precision here too

plugins/inputs/ping/ping_test.go Outdated
@@ -41,6 +41,18 @@ PING www.google.com (216.58.218.164) 56(84) bytes of data.
rtt min/avg/max/mdev = 35.225/43.628/51.806/5.325 ms
`

var enigma2PingOutput = `
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment here with the output of ping -V (or the equivalent version option).

@marianob85
Copy link
Contributor Author

Honestly it is a problem because of BusyBox and I don't think this is a right way of fixing it. I make a condition based on mipsle compilation (GOARCH) but I think it should be done different.
Maybe user should specify which arguments are supported and what is a prefix of them ? Maybe You have some idea.

@marianob85
Copy link
Contributor Author

This is how ping command look like on BusyBox

BusyBox v1.24.1 (2017-02-28 03:28:13 CET) multi-call binary.

Usage: ping [OPTIONS] HOST

Send ICMP ECHO_REQUEST packets to network hosts

    -4,-6           Force IP or IPv6 name resolution
    -c CNT          Send only CNT pings
    -s SIZE         Send SIZE data bytes in packets (default:56)
    -t TTL          Set TTL
    -I IFACE/IP     Use interface or IP address as source
    -W SEC          Seconds to wait for the first response (default:10)
                    (after all -c CNT packets are sent)
    -w SEC          Seconds until ping exits (default:infinite)
                    (can exit earlier with -c CNT)
    -q              Quiet, only display output at start
                    and when finished
    -p              Pattern to use for payload

@marianob85
Copy link
Contributor Author

Maybe add new parameter, something like 'target= busybox' ?

@danielnelson
Copy link
Contributor

I like the idea to introduce a variable where you can pick a dialect, we did something similar for procstat plugin recently to support Windows. On ping, we also want to support a pure Go implementation in addition to the current method of exec'ing another command.

I think all of that would be a moderate amount of work though, as it's probably best to just start from scratch on the implementation.

danielnelson
danielnelson reviewed Mar 9, 2018
View reviewed changes
plugins/inputs/ping/ping.go Outdated
args = append(args, "-i", strconv.FormatFloat(p.PingInterval, 'f', 1, 64))
switch runtime.GOARCH {
case "mipsle":
// -i argument not supported in busybox
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add anything for this then, if the ping_interval is set to "0s" it will not add this option.

@danielnelson
Copy link
Contributor

Can you take a look at the failed testcase?

@marianob85
Copy link
Contributor Author

Ping is already fixed but tests crash somewhere on statsd

danielnelson
danielnelson reviewed Apr 12, 2018
View reviewed changes
plugins/inputs/ping/ping.go Outdated
@@ -268,7 +271,7 @@ func init() {
inputs.Add("ping", func() telegraf.Input {
return &Ping{
pingHost: hostPinger,
PingInterval: 1.0,
PingInterval: 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the delay merging, should/can we change this back to 1.0?

@danielnelson danielnelson added this to the 1.7.0 milestone Apr 12, 2018
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Apr 12, 2018
@danielnelson danielnelson merged commit 0768022 into influxdata:master May 1, 2018
@marianob85 marianob85 deleted the enigma2_ping branch May 3, 2018 11:07
arkady-emelyanov pushed a commit to arkady-emelyanov/telegraf that referenced this pull request May 18, 2018
@marianob85 @arkady-emelyanov
Support busybox ping in the ping input (influxdata#3877)
cee28e2
maxunt pushed a commit that referenced this pull request Jun 26, 2018
@marianob85 @danielnelson
Support busybox ping in the ping input (#3877)
a19eaf0
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
@marianob85 @otherpirate
Support busybox ping in the ping input (influxdata#3877)
43d65e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson left review comments

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@marianob85 @danielnelson