Closed jak3kaj closed 1 year ago
@jak3kaj thanks for this PR! Helpful and appreciated I think the project should gradually reduce the dependencies to external tools. How complex/hard would be to NOT depend on ethtool
at all and just read data from /sys/class/net
?
I think that simplifying the code path (there's preferred + fallback no more, just one flow) should bring some advantages at last.
I'll review the PR shortly.
Indeed, thank you for this great PR @jak3kaj! I'm halfway through a full review. Currently at Kubernetes contributor summit in Amsterdam so responses might be a bit delayed :)
Did a quick pass. Besides the other high-level concern I already shared (it would be very nice to NOT depend on external tools in new code if we can help it), we need to check two more things:
Regarding dependence on ethtool.
I agree it would be better to not depend on an external tool. There are some minimal distros where ethtool is not installed by default, so yes, this should be avoided.
I am actually pulling out all of the useful info from the /sys/class/net
filesystem, unfortunately it isn't very comprehensive.
While doing this work, I did notice that there are actually two ethtool
projects that are being used. The latest one is 100% Go; which gave me the crazy idea to implement some of the ethtool functions directly in my code. Doing a little more research I found someone may have done this already, since there is a go ethtool library. So this is a possibilty. I can't commit to refactoring this patch to use a native Go implementation of ethtool
at the moment, but I agree it is a better way to do this.
OK, I've had a little time to digest this and I think I would prefer the following:
- For the following things, make them
NICCapability
objects:
- AutoNegotiation (string value of
auto-negotiation
)- PauseFrameUse (string value of
pause-frame-use
)So, in other words, instead of having individual fields called
AutoNegotiation
,SupportedPauseFrameUse
andSupportsAutoNegotiation
you would append aNICCapability
struct to theNIC.Capabilities
collection that represents that particular feature's enablement and supportability.
Sure, Makes sense to me too.
- Put the following fields directly on the
NIC
struct:
Speed
Duplex
SupportedLinkModes
SupportedPorts
SupportedFECModes
AdvertisedLinkModes
AdvertisedFECModes
Cool, I like these fields being easier to access.
I'd like to see LinkDetected
(bool
) here as well.
- Put these new fields directly on the
NIC
struct:
SupportedWakeOnModes
([]string
)AdvertisedWakeOnModes
([]string
)WakeOn is not a boolean, it's a string containing the supported wake on modes... and so best to have this in a separate
[]string
field.
Both of those fields are string
types, so I'll convert them to []string
so its more obvious that each letter represents a different feature.
- For now, ignore all the other fields that you have in your
NICLinkInfo
struct. I'd be happy to discuss those other fields in a future PR. I'd also be happy to see totally separate PRs for 1) , 2) and 3) above :)
So we will ignore Port
, PHYAD
, Transceiver
, MDIX
, LinkDetected
, and NETIFMsgLevel
.
Most of those give inconsistent/useless output and/or have been obsolete for 20 years; but the LinkDetected
field is consistent and seems useful to me.
Happy to discuss this further!
Best, -jay
Thanks Jay. I appreciate your feedback. I'll make these changes and submit 3 PRs for 1), 2) and 3). Let me know what you think about LinkDetected
.
Jacob
OK, I've had a little time to digest this and I think I would prefer the following:
- For the following things, make them
NICCapability
objects:
- AutoNegotiation (string value of
auto-negotiation
)- PauseFrameUse (string value of
pause-frame-use
)So, in other words, instead of having individual fields called
AutoNegotiation
,SupportedPauseFrameUse
andSupportsAutoNegotiation
you would append aNICCapability
struct to theNIC.Capabilities
collection that represents that particular feature's enablement and supportability.Sure, Makes sense to me too.
- Put the following fields directly on the
NIC
struct:
Speed
Duplex
SupportedLinkModes
SupportedPorts
SupportedFECModes
AdvertisedLinkModes
AdvertisedFECModes
Cool, I like these fields being easier to access.
I'd like to see
LinkDetected
(bool
) here as well.
- Put these new fields directly on the
NIC
struct:
SupportedWakeOnModes
([]string
)AdvertisedWakeOnModes
([]string
)WakeOn is not a boolean, it's a string containing the supported wake on modes... and so best to have this in a separate
[]string
field.Both of those fields are
string
types, so I'll convert them to[]string
so its more obvious that each letter represents a different feature.
:+1: Alternately, you could create long-form string constants instead of the single-letter strings. Up to you! :)
- For now, ignore all the other fields that you have in your
NICLinkInfo
struct. I'd be happy to discuss those other fields in a future PR. I'd also be happy to see totally separate PRs for 1) , 2) and 3) above :)So we will ignore
Port
,PHYAD
,Transceiver
,MDIX
,LinkDetected
, andNETIFMsgLevel
.Most of those give inconsistent/useless output and/or have been obsolete for 20 years; but the
LinkDetected
field is consistent and seems useful to me.
For this particular PR, let's leave LinkDetected
out. I'd like to do that one in a followup PR after discussing a little further.
My primary concern with LinkDetected
is that it is a field that mutates often (think of a WLAN-connected interface for a laptop that is moving in between tech conference hallways; this is top-of-mind since I'm currently at kubeconEU ;) ). ghw
has traditionally been for discovery of resource quantities and capabilities that don't change often and that don't represent a point-in-time amount like amount of memory currently in use, etc.
That said, I'd like to hear from @ffromani to see what his take on this is. Adding support for LinkDetected
would bring ghw
into more use cases such as network monitoring tools that will periodically poll for device status information. This would be expanding ghw
's traditional scope but I'm OK doing that if we'd like to.
Closing out in favor of rebased code in #338 and #342
Default to parsing the output of/ directory. Information such as interface Speed, Duplex, Link Detected, and Auto Negotiation will be available for each NIC.
ethtool
, but fall back to reading a few parameters from the /sys/class/net/ghw.NIC.LinkInfo
is a pointer to aghw.NICLinkInfo
struct, which describes parameters of the NIC's link information and status. This will contain the fields returned byethtool <DEVICE>
call on Linux, but will fall back to returning link information available in the /sys/class/net/The
gwh.NICLinkInfo
struct contains the following fields:ghw.NICLinkInfo.Speed
is a string showing the current link speed. This field will be present even if ethtool is not available.ghw.NICLinkInfo.Duplex
is a string showing the current link duplex. This field will be present even if ethtool is not available.ghw.NICLinkInfo.AutoNegotiation
is a boolean indicating whether Auto-Negotiation is enabledghw.NICLinkInfo.Port
is a string showing the current link port typeghw.NICLinkInfo.PHYAD
is a string showing the device driver's settingghw.NICLinkInfo.Transceiver
is a string showing the transceiver's stateghw.NICLinkInfo.MDIX
is a string slice indicating the state of MDI-X capability of the interface.ghw.NICLinkInfo.SupportsWakeOn
is a string containing a list of supported WakeOnLAN events.ghw.NICLinkInfo.WakeOn
is a string indicating which WakeOnLAN events are will trigger wake up.ghw.NICLinkInfo.LinkDetected
is a boolean indicating whether there is an active link detected on this interface. This field will be present even if ethtool is not available.ghw.NICLinkInfo.SupportedPorts
is a string slice containing the list of supported port types (MII, TP, FIBRE)ghw.NICLinkInfo.SupportedLinkModes
is a string slice containing a list of supported link modesghw.NICLinkInfo.SupportedPauseFrameUse
is a boolean indicating if pause frame use is enabled.ghw.NICLinkInfo.SupportsAutoNegotiation
is a boolean indicating if the network device supports Auto Negotiation.ghw.NICLinkInfo.SupportedFECModes
is a string slice containing a list of supported FEC Modes.ghw.NICLinkInfo.AdvertisedLinkModes
is a string slice containing the link modes being advertised during auto negotiation.ghw.NICLinkInfo.AdvertisedPauseFrameUse
is a boolean indicating if pause frame use is being advertised durinig auto negotiation.ghw.NICLinkInfo.AdvertisedAutoNegotiation
is a boolean indicating if auto negotiation is advertised.ghw.NICLinkInfo.AdvertisedFECModes
is a string slice containing the FEC modes advertised during auto negotiation.ghw.NICLinkInfo.NETIFMsgLevel
is a string slice containing the network driver's debug level and other misc driver information from the kernel.