Make drivers.yaml file optional

[zeeke]

Change the path to the drivers.yaml from a constant to a user-definable parameter path.supported-drivers-version-db. If the user does not specify any value for the file path, then every ethernet driver is considered good to use.

@SchSeba @Eoghan1232 please take a look

[Eoghan1232]

Should we assume every driver is considered good to use?
I think the default values should be hard-set unless overwritten in the new driver yaml file?

if a user used an out of date driver, i.e. not supported - it would just fail?

wdyt?

[zeeke]

Should we assume every driver is considered good to use?

That's a good question. If the IsDriverSupported check is removed, the VfStats(...) function will log an error and return empty stats. That's almost the same behavior, but we'd get a slightly worse error message.

sriov-network-metrics-exporter/pkg/vfstats/netlink.go

Line 17 in 9f558ab

func VfStats(pf string) PerPF {

The problem with the supported driver map is that an in-tree driver inherits the version from the kernel. So it's not trivial to prepare a good drivers.yaml file.

[Eoghan1232]

right - but if we are assuming that if a yaml is not provided, then return empty and assume all are correct, this will be the default configuration for everyone

so there' not much point in checking it then.....

it's a catch 22, if you default it to some values, then folks will hit it and have to either set the values to empty and overwrite, or else explicitly set the values based on their env..........

I am not sure which approach I like better....

maybe don't check the version - and call out a minimum (we do this) in the README
and update the logs - to say it could be an issue with driver version ?

@zeeke @SchSeba not sure, what are your opinions here?

[zeeke]

Mentioning the supported hardware and driver version in the README is enough, IMO. I vote for removing the IsDriverSupported check, but I'm ok with anything that can simplify the deployment of this component

Alos, I'd like to hear what @SchSeba think about this

[SchSeba]

I will vote to remove this file as I don't think it really works for in-tree drivers.

for example, we have 1.9 version in the file for ICE driver but for example, in RHEL all the driver versions are based on the kernel. For example "4.18.0-513.18.1.el8_9.x86_64" will be marked as a "good" version but we don't really know if that driver is newer or older than the u/s 1.9 version requested in the file

[Eoghan1232]

@zeeke any progress on this?

[zeeke]

Sorry, I wasn't sure I got the plan here.

Can you confirm this:
a. the exporter does not read any drivers.yaml file.
b. netlink collector is used regardless the driver and/or the version
c. (optional) we log a warning if the driver version is older than some hardcoded value

is that right?

[Eoghan1232]

Sorry, I wasn't sure I got the plan here.

Can you confirm this: a. the exporter does not read any drivers.yaml file. b. netlink collector is used regardless the driver and/or the version c. (optional) we log a warning if the driver version is older than some hardcoded value

is that right?

my understanding is that we will remove the drivers.yaml file, remove the IsDriverSupported check.
add a note/couple lines in README calling out the minimum supported version.

I think if netlink fails, then we can fall back on sysfs collector, etc.

@zeeke @SchSeba is this what has been agreed?

[zeeke]

Sorry, I wasn't sure I got the plan here.
Can you confirm this: a. the exporter does not read any drivers.yaml file. b. netlink collector is used regardless the driver and/or the version c. (optional) we log a warning if the driver version is older than some hardcoded value
is that right?

my understanding is that we will remove the drivers.yaml file, remove the IsDriverSupported check. add a note/couple lines in README calling out the minimum supported version.

I think if netlink fails, then we can fall back on sysfs collector, etc.

@zeeke @SchSeba is this what has been agreed?

sounds very good to me. Going to provide changes

[SchSeba]

closing this one in favor of #37