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 |
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 <brandon.ewing@warningg.com>
|
Oh what a difference 4 years makes. Updated branch, node_exporter tests no longer fail. Can we re-touch on the questions I had in the initial PR, and see if we can get this to a mergable state? |
It appears the pattern that was used in the Infiniband section was to include both: procfs/sysfs/class_infiniband.go Lines 113 to 116 in 506a32c |
|
Thanks for coming back to working on this. Looks like there's some testing issues. |
Pushed new commit with linting fixes. |
Update net_class.go with new structures to handle bonding driver information Add new true/false values to internal.util.parse.ParseBool Update AerCounters to ignore logical interfaces 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 <brandon.ewing@warningg.com>
|
Weird that gofmt didn't fix this for me. pushed new changeset after running golangci-lint. |
|
Yea, we have some |
3 participants
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 ?)