Process sysfs/class/net/*/bonding

[bewing]

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:

• There does not appear to be consistency on pointers vs values in many modules of this project. I notice that net_class.go uses 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 for nil values 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 this
• The kernel driver has many different sized ints (uint8, int64, uint64, etc) -- should I try to match what the current kernel defines these as, or just use uint64s as the worst case storage requirement?
• The bonding driver has many files of the format <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?
• Should I be using pointers to other NetClassIface structs to reference controllers and devices (IE, L34, L64 ?)
• Should any special care/documentation be taken regarding fields that need CAP_NET_ADMIN to read?

[discordianfish]

There does not appear to be consistency on pointers vs values in many modules of this project. I notice that net_class.go uses 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 for nil values 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 this

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.

The kernel driver has many different sized ints (uint8, int64, uint64, etc) -- should I try to match what the current kernel defines these as, or just use uint64s as the worst case storage requirement?

Yes we try to mimic the underlying sized ints

The bonding driver has many files of the format -- 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?

Not sure what you mean by that.

Should I be using pointers to other NetClassIface structs to reference controllers and devices (IE, L34, L64 ?)

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.

Should any special care/documentation be taken regarding fields that need CAP_NET_ADMIN to read?

I don't think we have any convention for that.

[bewing]

The bonding driver has many files of the format -- 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?

Not sure what you mean by that.

For example, sys/class/net/bond0/bonding/ad_select :

$ cat /sys/class/net/bond0/bonding/ad_select
stable 0

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?

[bewing]

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

[discordianfish]

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'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..