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

cmd/scollector: Add additional Linux interfaces #1124

Merged
merged 1 commit into from
Jul 28, 2015
Merged

cmd/scollector: Add additional Linux interfaces #1124

merged 1 commit into from
Jul 28, 2015

Conversation

pdf
Copy link
Contributor

@pdf pdf commented Jul 1, 2015

  • ppp+
  • tun+
  • tap+

@kylebrandt
Copy link
Member

IIRC these are all virtual interfaces for vpn? If that is the case I think
we should put this all under a different metric namespace like we do with
bonds
On Jul 1, 2015 7:50 AM, "Peter Fern" notifications@github.com wrote:

  • ppp+
  • tun+
  • tap+

You can view, comment on, or merge this pull request online at:

#1124
Commit Summary

  • Add additional Linux interface names

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1124.

@pdf
Copy link
Contributor Author

pdf commented Jul 1, 2015

tun and tap devices are commonly used for VPN or other tunnel/virtual interfaces, ppp is point-to-point, commonly used for dialup and *DSL connections, frequently used as WAN for satellite offices. Recommendations?

@kylebrandt
Copy link
Member

@pdf Two main things I'm thinking:

  1. We don't want to double count bytes under the metric, or else aggregation will get messed up (i.e. give me the total network traffic for this host)
  2. I think we should keep any non-physical traffic out of os.net.bytes and have a different name space for it. So interfaces should correspond to actual hardware (or at least what the OS believes is actual hardware, i.e. VMWare may provide a virtual NIC, but the OS mostly considers it "real").

Wonder if people agree with me...?

@vitorboschi
Copy link
Contributor

@kylebrandt: I was initially leaning toward using the same namespace for all interfaces, but your arguments made me think it's better to keep them in different namespaces. I'm not sure about ppp interfaces though, because traffic may run over a serial line (i.e., actually a different interface)

@kylebrandt
Copy link
Member

@vitorboschi Maybe sys or proc provides a way to distinguish between hardware vs non-hardware ppp devices?

@vitorboschi
Copy link
Contributor

Maybe checking /sys/class/net//type, but I don't have any ppp device here to test

@pdf
Copy link
Contributor Author

pdf commented Jul 1, 2015

@vitorboschi unfortunately there's no distinction (types are in the kernel include/uapi/linux/if_arp.h).

@kylebrandt shall we just put all these other interfaces under a virtual or other namespace? I also really wonder if the regex whitelist is the right approach. You can name interfaces anything, and people may want accounting on other virtual interfaces like bridges, VM backing interfaces, etc. We're also missing wlan, maybe other items that may appear on routers, etc.

Perhaps we should include all devices by default and look at sysfs to determine type based on the contents of type, followed by the presence of bonding/bridge/tun_flags/etc nodes. This should let us make pretty decent decisions, though we might miss some more esoteric devices that may get mis-classified as 'physical'.

@kylebrandt
Copy link
Member

I'm okay with putting them under *.net.virtual. - makes sense to me. I'm not that concerned about the method we use, but I think I like the "don't include" default for interfaces prevents a lot of crap from showing up and is important (in particular with opentsdb where deletion isn't so much of a thing). That and nobody has ever said they needed support for custom interface names yet.

@pdf
Copy link
Contributor Author

pdf commented Jul 1, 2015

@kylebrandt if not all interfaces are included by default, I think included interfaces need to be configurable. I've done an audit, and I have a bunch of devices around that don't match the regex, and that I'd like monitored.

@d10v
Copy link
Contributor

d10v commented Jul 1, 2015

AFAIK physical interfaces link to real devices in, e.g.

ls -lh /sys/class/net                            
total 0
lrwxrwxrwx 1 root root 0 Jul  1 23:32 docker0 -> ../../devices/virtual/net/docker0
lrwxrwxrwx 1 root root 0 Jul  1 23:32 eth0 -> ../../devices/pci0000:00/0000:00:19.0/net/eth0
lrwxrwxrwx 1 root root 0 Jul  1 23:32 lo -> ../../devices/virtual/net/lo
lrwxrwxrwx 1 root root 0 Jul  2 00:40 tun0 -> ../../devices/virtual/net/tun0
lrwxrwxrwx 1 root root 0 Jul  1 23:32 wlan0 -> ../../devices/pci0000:00/0000:00:1c.1/0000:03:00.0/net/wlan0

@pdf
Copy link
Contributor Author

pdf commented Jul 1, 2015

Here's a first pass, collecting all interfaces by default, categorizing based on type and then /virtual/ device path, and finally bonding for backwards compatibility.

I don't have any teamed interfaces configured, I'll set one up later and make sure that gets tagged as bond. like bonding interfaces do.

If we're sure we don't want to collect all interfaces, I'll add logic to omit virtual devices by default, and allow configuration of which interfaces to include.

@pdf
Copy link
Contributor Author

pdf commented Jul 2, 2015

Looks like team devices don't expose anything to identify them via sysfs, so I've fallen back to a regex match for them. Considering team interface names are completely arbitrary that's not reliable, but no worse off than the previous code.

Also, FYI, interface names will likely change for many distros shipping full systemd: https://wiki.freedesktop.org/www/Software/systemd/PredictableNetworkInterfaceNames/

This has already occurred in some distros, making them incompatible with current scollector.

So, just need to decide if we really don't want to collect all interfaces by default, and if that's the case, what the configuration should look like.

@pdf pdf changed the title Add additional Linux interface names cmd/scollector: Add additional Linux interfaces Jul 3, 2015
@pdf
Copy link
Contributor Author

pdf commented Jul 27, 2015

What's needed to move forward on this one?

@kylebrandt
Copy link
Member

@pdf sorry I'll test this on a few machines today, thanks for the ping

@kylebrandt
Copy link
Member

@captncraig Can you advise why the build is failing here? I would like to get a clean build so I can do some testing and get this merged - @pdf has been pretty patient on this one

@captncraig
Copy link
Contributor

This might be from when esc was borked. If you git commit --amend and then force push, you can re-run the build and my guess is it will pass

@pdf
Copy link
Contributor Author

pdf commented Jul 27, 2015

Passing now.

captncraig added a commit that referenced this pull request Jul 28, 2015
cmd/scollector: Add additional Linux interfaces
@captncraig captncraig merged commit fd4ad17 into bosun-monitor:master Jul 28, 2015
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.

5 participants