Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add RootDevices selector #264

Merged
merged 4 commits into from
Sep 24, 2020
Merged

Add RootDevices selector #264

merged 4 commits into from
Sep 24, 2020

Conversation

zshi-redhat
Copy link
Collaborator

No description provided.

@zshi-redhat
Copy link
Collaborator Author

/cc @ahalim-intel @martinkennelly @adrianchiris

martinkennelly
martinkennelly reviewed Aug 27, 2020
View reviewed changes
Copy link
Member

@martinkennelly martinkennelly left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just wondering if we can log error as well as sending them to stdout.

pkg/resources/deviceSelectors.go
// in selector pool
fields := strings.Split(selector, "#")
if len(fields) != 2 {
fmt.Printf("Failed to parse %s PF (name|address) selector, probably incorrect separator character usage\n", selector)
Copy link
Member

@martinkennelly martinkennelly Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we start logging too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was previously located at L#109 and was moved here so logic may be shared between selectors.

i think moving Printf() calls to glog.<logging-call>() is a welcomed addition, but should be in a separate PR IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will submit another PR to use glog.

@killianmuldoon
Copy link
Collaborator

@zshi-redhat is this work intended to support the VF use case?

adrianchiris
adrianchiris reviewed Aug 30, 2020
View reviewed changes
Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting the patch @zshi-redhat , a couple of comments

pkg/netdevice/vhostNet.go Outdated
@@ -8,7 +8,7 @@ import (

// VhostNetDeviceExist returns true if /dev/vhost-net exists
func VhostNetDeviceExist() bool {
_, err := os.Stat("/dev/vhost-net");
_, err := os.Stat("/dev/vhost-net")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unrelated change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was merged in #263, I will rebase this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pkg/resources/deviceSelectors.go
// in selector pool
fields := strings.Split(selector, "#")
if len(fields) != 2 {
fmt.Printf("Failed to parse %s PF (name|address) selector, probably incorrect separator character usage\n", selector)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was previously located at L#109 and was moved here so logic may be shared between selectors.

i think moving Printf() calls to glog.<logging-call>() is a welcomed addition, but should be in a separate PR IMO

pkg/netdevice/pciNetDevice.go Outdated
@@ -75,6 +80,7 @@ func NewPciNetDevice(dev *ghw.PCIDevice, rFactory types.ResourceFactory, rc *typ
PciDevice: pciDev,
ifName: ifName,
pfName: pfName,
pfAddr: pfAddr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was the attribute moved from types.PciDevice ?

pfAddr seems like a common attribute of an SR-IOV pci device and not limited to net device

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pfAddr was in types.PciDevice, but it was not populated or used by any other functions. I moved the pfAddr to pciNetDevice where pfName is included.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pfName - represents the PF netdevice name so it makes sense for it to be in pciNetDevice

pfAddr - represents the device's PF PCI address if present (e.g for SR-IOV device) its a general PCI device attribute so it should really be part of PciDevice in my opinion regardless if its usage today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrianchiris do you have an example where we have a pfAddr, but not pfName?
I can move the pfAddr to pciDevice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general any PCI device that is not a network device has a PCI address but does not have a net device,
or even a PCI network device that is not bound to a network driver.

i understand that pfAddr was not used outside the context of PciNetDevice but having it in PciDevice is in my opinion a better representation of the system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, pfAddr was moved to PciDevice.

@zshi-redhat
Copy link
Collaborator Author

@zshi-redhat is this work intended to support the VF use case?

I wasn't intend to support VF use case as we already have the pciAddresses selector.
But looking at the GetPfAddr function, it returns VF pci address when PF doesn't exist, I think it could also work for VFs.

@killianmuldoon
Copy link
Collaborator

@zshi-redhat is this work intended to support the VF use case?

I wasn't intend to support VF use case as we already have the pciAddresses selector.
But looking at the GetPfAddr function, it returns VF pci address when PF doesn't exist, I think it could also work for VFs.

Apologies - typo from me! I mean the VM use case.

@zshi-redhat
Copy link
Collaborator Author

@zshi-redhat is this work intended to support the VF use case?

I wasn't intend to support VF use case as we already have the pciAddresses selector.
But looking at the GetPfAddr function, it returns VF pci address when PF doesn't exist, I think it could also work for VFs.

Apologies - typo from me! I mean the VM use case.

@killianmuldoon this is not intended to support VM case as rootDevices shall be used to specify PF PCI addresses, instead of VF PCI addresses.

adrianchiris
adrianchiris reviewed Sep 7, 2020
View reviewed changes
pkg/resources/pool_stub_test.go
@@ -27,7 +27,7 @@ var _ = Describe("PoolStub", func() {
fs = &utils.FakeFilesystem{
Dirs: []string{
"sys/bus/pci/devices/0000:00:00.1/net/enp2s0f1",
"sys/bus/pci/devices/0000:00:00.1/physfn/net/enp2s0f0",
"sys/bus/pci/devices/0000:01:00.0/net/enp2s0f0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: doesnt really affect the test, but should this point to a different netdev than the one on L#31 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! updated the pf/vf names.

@adrianchiris
Copy link
Contributor

Overall im LGTM :)

@martinkennelly
Copy link
Member

/LGTM - Thanks Zenghui.

ahalimx86
ahalimx86 approved these changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@ahalimx86 ahalimx86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@ahalimx86 ahalimx86 merged commit b7f6d3e into k8snetworkplumbingwg:master Sep 24, 2020
amorenoz pushed a commit to amorenoz/sriov-network-device-plugin that referenced this pull request Nov 4, 2020
@zshi-redhat @amorenoz
Add RootDevices selector (k8snetworkplumbingwg#264)
1450d3f 
* Add rootDevices selector for netDevice

* Add rootDevice unit tests

* Update README for rootDevices selector

* Move pfAddr in pciDevice
amorenoz pushed a commit to amorenoz/sriov-network-device-plugin that referenced this pull request Nov 5, 2020
@zshi-redhat @amorenoz
Add RootDevices selector (k8snetworkplumbingwg#264)
6f49d68 
* Add rootDevices selector for netDevice

* Add rootDevice unit tests

* Update README for rootDevices selector

* Move pfAddr in pciDevice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianchiris adrianchiris adrianchiris left review comments

@martinkennelly martinkennelly martinkennelly left review comments

@ahalimx86 ahalimx86 ahalimx86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zshi-redhat @killianmuldoon @adrianchiris @martinkennelly @ahalimx86