RFC: Enhance Environment Variable support

[amorenoz]

The Device Plugin API allows Device Plugins to set any number of environment variables.

Currently, the SR-IOV Network Device Plugin uses this API to set the following ENV var per allocation call:

EnvName: "PCIDEVICE_${resourceNamePrefix}_${resourceName}",
EnvValue: Comma-separated list of PCI Addresses of the allocated resources

This approach can be considered to limit extensibility (more information cannot be easily added without overcomplicating the EnvValue) and flexibility (only PCIDEVICEs can be exposed).

I'd like to raise the question of: Can this mechanism be improved to allow more/different information to be added to the Pod through this mechanism?

An example proposal

As a discussion-trigger I'll put forward a possible enhancement (assuming #281 gets merged):

• The resourceServer adds the current key-value to the response message by using the deviceID, which is the PCI Address. So backwards compatibility is ensured.
• The DeviceInfoProvider API is changed so that GetEnvVal returns a map (map[string] string of EnvName - EnvValue)
• Each DeviceInfoProvider can add ENV variables.
• The pciAddress gets appended or prepended to the ENV names of the InfoProvider (e.g: by PciDevice.GetEnvVal() or by 'resourcePool.GetEnvs()')
• The resourceServer adds all the key-values to the AllocateResponse message.

The result on an allocate call with two devices the result would be something like:
(The values of each Info Provider are just examples to show the point)

EXAMPLE_IO_MYPOOL_PCIDEVICE=0000:01:05.1,0000:01:05.2
RDMA_0000_01_05_1=mlx5_3
RDMA_0000_01_05_2=mlx5_4
VHOSTNET_0000_01_05_1=/dev/vhost-net
VFIO_0000_01_05_1=/dev/vfio/4
VFIO_0000_01_05_2=/dev/vfio/5

Does it make sense to consider something like this?

[amorenoz]

IIRC, we discussed this sometime back @adrianchiris. Any comments?

[adrianchiris]

Hi @amorenoz indeed i remember !
Im a bit Swamped with downstream work, will try to get this some thoughts during next week.

with device info spec landing, this information (Some of this ?) may be expoesd thorugh downward API ?

[amorenoz]

Sure, no rush @adrianchiris.
Indeed this information will be exposed through downwards API. And I don't think any feature depends on this, but it would be a good idea to make the ENV vars somewhat consistent with the rest of the information we expose.

The way I came to remember this issue was while coding the vDPA support:
8485329#diff-1db50ac160017009a2f0b6bd7718dedc47eb9871eac658a77aa5c71262ad3519

[adrianchiris]

Hi @amorenoz,

Im thinking of a different approach, instead of having the information exposed across multiple environment variables per allocated device, have a single extendable environment variable per allocated device with all the information from the different info providers as json format.

As a side effect This would also make query of device information for a specific device from within the container fairly easy.
(e.g In the Pod you can then use simple jq command to extract the data if needed for the workload)

So, if i take your example above and apply this approach we would have:

PCIDEVICE_EXAMPLE_IO_MYPOOL=0000:01:05.1,0000:01:05.2
PCIDEVICE_EXAMPLE_IO_MYPOOL_0000_01_05_1='{"rdma": "mlx5_3", "vhostNet": "/dev/vhost-net", "vfio": "/dev/vfio/4"}'
PCIDEVICE_EXAMPLE_IO_MYPOOL_0000_01_05_2='{"rdma": "mlx5_4", "vhostNet": "/dev/vhost-net", "vfio": "/dev/vfio/5"}'

We can make this as consistent as possible with device-info-spec so later maybe DeviceInfoProviders may be used in the creation of the file (today it is stated that the goal of the interface is to retrieve Device Plugin API specific information).

DeviceInfoProvider::GetEnvVal() would change to return a map[sting]interface{}

The maps from the various providers will be merged to a single map ensuring uniqueness of top level keys in PciDevice::GetEnvVal (or extend DeviceInfoProvider with a name() method and use it as top level key). This is then propagated up to resourceServer

[amorenoz]

Hi @amorenoz,

Im thinking of a different approach, instead of having the information exposed across multiple environment variables per allocated device, have a single extendable environment variable per allocated device with all the information from the different info providers as json format.

As a side effect This would also make query of device information for a specific device from within the container fairly easy.
(e.g In the Pod you can then use simple jq command to extract the data if needed for the workload)

So, if i take your example above and apply this approach we would have:

PCIDEVICE_EXAMPLE_IO_MYPOOL=0000:01:05.1,0000:01:05.2
PCIDEVICE_EXAMPLE_IO_MYPOOL_0000_01_05_1='{"rdma": "mlx5_3", "vhostNet": "/dev/vhost-net", "vfio": "/dev/vfio/4"}'
PCIDEVICE_EXAMPLE_IO_MYPOOL_0000_01_05_2='{"rdma": "mlx5_4", "vhostNet": "/dev/vhost-net", "vfio": "/dev/vfio/5"}'

We can make this as consistent as possible with device-info-spec so later maybe DeviceInfoProviders may be used in the creation of the file (today it is stated that the goal of the interface is to retrieve Device Plugin API specific information).

DeviceInfoProvider::GetEnvVal() would change to return a map[sting]interface{}

The maps from the various providers will be merged to a single map ensuring uniqueness of top level keys in PciDevice::GetEnvVal (or extend DeviceInfoProvider with a name() method and use it as top level key). This is then propagated up to resourceServer

I agree, that's a cleaner approach. I thought this kind of approach was out of the table, maybe because it was considered in the initial device-info-spec discussion days.

If this is an option, I'd say, keep the json device-info-spec compliant to avoid confusion and work towards API unification. If we go down this road, I would try tu unify the sources of such information and use the DeviceInfoProvider to generate this information and reuse it to write the device-info-spec file. We could rename the function to something like DeviceInfoProvider::GetInfo() map[sting]interface{} and call it from both netResourcePool::StoreDeviceInfoFile() and netResourcePool::getEnvs()

[adrianchiris]

I must have missed the notification for your reply here Adrian.

In regards to:

I thought this kind of approach was out of the table, maybe because it was considered in the initial device-info-spec discussion days.

Im probably missing some history here with past discussions about extending environment variables created by device plugin.
perhaps @zshi-redhat or @ahalim-intel remember.

[adrianchiris]

This was discussed in today's community meeting.

There is agreement to expose extended information via environment variables as this RFC proposes.
it should be well document, and backward compatible so users can rely on this API.
In addition it should keep things as consistent as possible with https://github.com/k8snetworkplumbingwg/device-info-spec