Process sysfs/class/net/*/bonding #439
Conversation
bewing
force-pushed
the
sysfs_bonding
branch
6 times, most recently
from
0a19628
to
da42abf
Compare
bewing
changed the title
yes, we use pointers to distinguish between a null value and e.g non existing file as in your example. Other reasons would be larger structs where you want to avoid copying them when you pass them to functions.
Yes we try to mimic the underlying sized ints
Not sure what you mean by that.
If you retrieved the data for them I see why not, but we probably shouldn't waste cycles on getting them if we don't need them in which case I'd just use some string? identifier.
I don't think we have any convention for that. |
For example, The actual underlying stored kernel value for ad_select is an integer, but bond_opt_get_val is used to resolve that integer 0 to the string "stable" via a constant lookup table bond_ad_select_tbl, and then both are present in the sysfs file. Should we return the integer, just the string, or a generic bond_options struct with both values? |
|
I'm not able to run tests in node_exporter using this branch, so there's still more digging/dev work needed before this is mergable |
sysfs/net_class.go
Outdated
| devices, err := util.SysReadFile(filepath.Join(path, "slaves")) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to read %s", filepath.Join(path, "slaves")) | ||
| } | ||
| for _, device := range strings.Split(devices, " ") { | ||
| if intf, exists := (*netClass)[device]; exists { | ||
| netClassIface.BondAttrs.Devices = append(netClassIface.BondAttrs.Devices, &intf) | ||
| } else { | ||
| return fmt.Errorf("unable to find device %s", device) | ||
| } | ||
| } |
There was a problem hiding this comment.
So it turns out the errors that I was seeing trying to test node_exporter against my branch are because they have an incomplete fixture set:
https://github.com/prometheus/node_exporter/blob/b52bf958f8e2d4ed1624b8122d08af4d12da9322/collector/fixtures/sys.ttar#L1094-L1098
Member devices eth1, eth4, and eth5 are not present in the fixtures. Does it make more sense to fix the upstream fixtures to be complete (either by removing the references to the non-existing devices, or adding dummy fixtures for them), or by not resolving them to pointers here and not returning an error at all?
There was a problem hiding this comment.
If we're sure that on real systems the member would be present, which I expect, I'd say to update the fixtures.
I'd include the other interfaces if it's not inflating the fixture sizes to much, otherwise remove the references.
Update net_class.go with new structures to handle bonding driver information Add new true/false values to internal.util.parse.ParseBool References: torvalds/linux/drivers/net/bonding/bond_sysfs.c torvalds/linux/drivers/net/bonding/bond_sysfs_slave.c torvalds/linux/include/net/bonding.h torvalds/linux/include/net/bond_options.h torvalds/linux/include/net/bond_3ad.h Signed-off-by: Brandon Ewing <[email protected]>
I'd say just the string but I wonder why they expose both the string and the int in procfs and if we should take that decisiopn into account here.. |
As part of change prometheus/procfs#439, update fixtures in node_exporter to expose all referenced interfaces in /sys/class/net This was naively accomplished by blindly copying eth0 data to other physical interfaces, but that does not appear to have resulted in any failing tests. Once we begin to actually export bonding/LACP metrics, additional modification of fixtures may be required. Signed-off-by: Brandon Ewing <[email protected]>
As part of prometheus/node_exporter#1604, parse information from
/sys/class/net/*/bonding*/*to provide information regarding current bonding state for bonding controllers and their devices.I have some style/format questions that I would appreciate input on as part of the process:
net_class.gouses pointers for integers, but not strings. Should I follow that, or is there a different preference from the maintainers? It would appear that using pointers would allow fornilvalues to indicate kernel-level support for a specific file (if the kernel doesn't have the file present, better to indicate with a nil pointer rather than a default empty string or integer?), but I'm curious what the consensus is in the project for indicating thisuint8,int64,uint64, etc) -- should I try to match what the current kernel defines these as, or just useuint64s as the worst case storage requirement?<uint> <human readable value string>-- should this be implemented as constants in the module itself, or imported constants from a reference module (assuming one exists), or some other approach I haven't thought of?NetClassIfacestructs to reference controllers and devices (IE, L34, L64 ?)