host-device: add pciBusID property #300
Conversation
zshi-redhat
force-pushed
the
host-device-pci-parameter
branch
from
56f972c to
acde1c6
Compare
plugins/main/host-device/README.md
Outdated
| * `device`: The device name, e.g. `eth0`, `can0` | ||
| * `hwaddr`: A MAC address | ||
| * `kernelpath`: The kernel device kobj, e.g. `/sys/devices/pci0000:00/0000:00:1f.6` | ||
| * `deviceID`: A PCI address of network device, e.g `0000:00:1f.6` |
There was a problem hiding this comment.
- how about
PCIBusIDinstead ofdeviceID? - you also mention that the parameter is optional, not required one.
There was a problem hiding this comment.
@s1061123 I didn't mark the deviceID as optional because it's similar to other existing options, such as hwaddr, device and kernelpath; at least one of these four options need to be provided.
Re the name of deviceID, I give a second thought on it and maybe it should not be changed as that's the name used by device plugin framework which in some cases might be referred with a different meaning other than PCI.
There was a problem hiding this comment.
In above example, deviceID in k8s sometimes uses other than PCI address. So user may confuse if we name 'PCIBusID' as 'deviceID'. This plugin, host-local plugin is for host local device, not only for PCI also USB (i.e. USB network adapter) as well as other bus, so we should give appropriate name for that.
There was a problem hiding this comment.
Ok, fair enough, updated a new version with pciBusID. Thanks @s1061123
| @@ -38,12 +39,17 @@ import ( | |||
| bv "github.com/containernetworking/plugins/pkg/utils/buildversion" | |||
| ) | |||
|
|
|||
| const ( | |||
| sysBusPci = "/sys/bus/pci/devices" | |||
There was a problem hiding this comment.
PTAL on https://github.com/golang/go/wiki/CodeReviewComments#initialisms - that should be named as sysBusPCI
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read net directory %s: %q", netDir, err) | ||
| } | ||
| return netlink.LinkByName(fInfo[0].Name()) |
There was a problem hiding this comment.
There is no check if this list has at least single entry. I'm not sure if this directory can be empty, but if it's possible - code will panic there. If you are sure that it has to have at least a single entry - just leave a comment about that there in code.
There was a problem hiding this comment.
added check for fInfo and return netlink by name when fInfo list larger than 0.
zshi-redhat
force-pushed
the
host-device-pci-parameter
branch
from
acde1c6 to
90b44d9
Compare
zshi-redhat
force-pushed
the
host-device-pci-parameter
branch
from
90b44d9 to
79b1c40
Compare
This allows host-device plugin to recognize Device PCI address passed from Multus. It is related to the change in host-device which enables use of device pci address as a config option: containernetworking/plugins#300
zshi-redhat
changed the title
This allows host-device plugin to recognize Device PCI address passed from Multus. It is related to the change in host-device which enables use of device pci address as a config option: containernetworking/plugins#300
This allows host-device plugin to recognize Device PCI address passed from Multus. It is related to the change in host-device which enables use of device pci address as a config option: containernetworking/plugins#300
This allows host-device plugin to recognize Device PCI address passed from Multus. It is related to the change in host-device which enables use of device pci address as a config option: containernetworking/plugins#300
Fixes #253