New environment variable to expose informers details in json format

[SchSeba]

This commit adds support for the sriov-device-plugin to inject extra env variables

example:

{
	"resourceList": [{
		"resourceName": "dpdk_nic_1",
		"extraEnvVariables": {
			"*": {
				"token": "test",
				"bla": "1"
			}
		}]
}

inside the container:

env | grep DPDK
PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_1=0000:5e:00.6
PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_1_INFO={"0000:5e:00.6":{"netdevice":{"pci":"0000:5e:00.6"},"rdma":{"issm":"/dev/infiniband/issm8","rdma_cm":"/dev/infiniband/rdma_cm","umad":"/dev/infiniband/umad8","uverbs":"/dev/infiniband/uverbs8"}}}

to access the info the container the user can use something like jq

sh-4.4# echo $PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_2_INFO | jq .\"0000:5e:00.2\"
{
  "netdevice": {
    "pci": "0000:5e:00.2"
  },
  "rdma": {
    "mounts": "/dev/infiniband/issm4,/dev/infiniband/umad4,/dev/infiniband/uverbs4,/dev/infiniband/rdma_cm"
  }
}
sh-4.4# echo $PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_2_INFO | jq .\"0000:5e:00.6\".netdevice.pci
"0000:5e:00.2"
sh-4.4# echo $PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_2_INFO | jq .\"0000:5e:00.6\".rdma.issm  
"/dev/infiniband/issm8"

Signed-off-by: Sebastian Sch [email protected]

[SchSeba]

waiting for #441

[adrianchiris]

Hey @SchSeba

as #441 was merged could you rebase your PR ?

i wonder if we should look at a different way of implementing this.
like having a new "extraInfoProvider" InfoProvider that will provide env vars information for device based on resource pool config.

this is related to: #303

in this case, we may expose the information i a bit of a different way than whats proposed here.
with the benefit of:

1. having information stored in a structured format
2. reduce ammount of env variables defined ( currently if we have a pod with 10 devices from a pool which has 10 env vars, we would have 100 env variables)

WDYT ?

[coveralls]

Pull Request Test Coverage Report for Build 4016059018

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 73 of 123 (59.35%) changed or added relevant lines in 14 files are covered.
• 24 unchanged lines in 6 files lost coverage.
• Overall coverage decreased (-1.8%) to 78.409%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/devices/host.go	2	4	50.0%
pkg/netdevice/pciNetDevice.go	2	4	50.0%
pkg/infoprovider/uioInfoProvider.go	7	10	70.0%
pkg/infoprovider/vdpaInfoProvider.go	10	13	76.92%
pkg/infoprovider/vfioInfoProvider.go	8	11	72.73%
pkg/infoprovider/vhostNetInfoProvider.go	6	9	66.67%
pkg/resources/server.go	7	10	70.0%
pkg/infoprovider/genericInfoProvider.go	0	7	0.0%
pkg/resources/pool_stub.go	0	24	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/infoprovider/genericInfoProvider.go	1	63.64%
pkg/auxnetdevice/auxNetDevice.go	3	78.95%
pkg/infoprovider/rdmaInfoProvider.go	3	93.02%
pkg/resources/pool_stub.go	4	43.16%
pkg/factory/factory.go	6	89.93%
pkg/netdevice/pciNetDevice.go	7	68.18%

Totals
Change from base Build 4015505129:	-1.8%
Covered Lines:	1932
Relevant Lines:	2464

---

💛 - Coveralls

[SchSeba]

i wonder if we should look at a different way of implementing this.
like having a new "extraInfoProvider" InfoProvider that will provide env vars information for device based on resource pool config.

I can try to implement it that way sure. A bit more complicated but It may be nicer :)

this is related to: #303
in this case, we may expose the information i a bit of a different way than whats proposed here.
with the benefit of:
having information stored in a structured format
reduce ammount of env variables defined ( currently if we have a pod with 10 devices from a pool which has 10 env vars, we would have 100 env variables)
WDYT ?

I must say I have a problem with the structured format as it will be more complicated for the applications that want to use it

for example instead of just using pure bash

echo PCIDEVICE__DPDK_NIC_1_`echo $PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_1 | tr :. _`_TOKEN

they will need to install jq for the parsing
Another theoretical problem we can have is the size limitation for env variables

In general, I don't expect a large number of extra env variables here.

[adrianchiris]

I can try to implement it that way sure. A bit more complicated but It may be nicer :)

cool ! thx :)

I must say I have a problem with the structured format as it will be more complicated for the applications that want to use it
for example instead of just using pure bash

echo PCIDEVICE__DPDK_NIC_1_echo $PCIDEVICE_OPENSHIFT_IO_DPDK_NIC_1 | tr :. __TOKEN

they will need to install jq for the parsing

yes jq or similar will be needed but in turn it would be easier to understand imo

echo $PCI_DEVICE_X_Y_Z | jq .token

Another theoretical problem we can have is the size limitation for env variables

i dont think we will hit issues max size is quite large.
see: https://stackoverflow.com/questions/1078031/what-is-the-maximum-size-of-a-linux-environment-variable-value

[adrianchiris]

Example in the cover message (1st message) is not accurate as it does not reflect the added env keys and values ("token": "test" and "bla": 1)

also could you update commit message to reflect changes done by this PR ?
ideally, if possible, split this to multiple commits for easier review.
like:

1. api changes
2. changes to existing components
3. addition of new env variable (_INFO)
4. addition of the new info provider
   (or something else that makes sense)
5. documentation (missing currently)

Also since the first iteration, i think we should change the topic of this PR as we essentially introduce a single new env variable which contains additional information about allocated devices. we then introduce a new info provider that adds information into this env variable.

[adrianchiris]

just an initial review. i still need to dig deeper into this. thx for working on it @SchSeba !

[SchSeba]

@adrianchiris when you have time please give another look

[adrianchiris]

replied on current comments. will now review again

[SchSeba]

Hi @adrianchiris when you have time please give it another review I hope now it's in a final state to get merge :)

[adrianchiris]

There is one (minor) comment otherwise LGTM

[adrianchiris]

Merging this one.

@SchSeba if you could followup with a PR to adjust the comment (to address the last open issue here) once you are back :)