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.

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

Add pciAddresses selectors for VM usecase #195

Merged
merged 3 commits into from
Jun 3, 2020

Conversation

pperiyasamy
Copy link
Contributor

@pperiyasamy pperiyasamy commented Jan 27, 2020

This makes device plugin to support VM usecase in which
vf (or) virtio devices can be enumerated into a device
pool using its pci addresses.

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

@zshi-redhat
Copy link
Collaborator

zshi-redhat commented Jan 31, 2020

Hi @pperiyasamy thanks for your PR!
Is this to group individual VF into one resource pool for VM use case where PF is not accessable ?
Is it possible to use vendor and device selectors instead of pfNames selector to achieve the same purpose?

@pperiyasamy
Copy link
Contributor Author

@zshi-redhat Yes, this is for the use case where PF is not accessible and left/right VF (or virtio) devices having same vendor and device id. hence we need to enumerate using their names to create left and right resource pools.

@zshi-redhat
Copy link
Collaborator

@zshi-redhat Yes, this is for the use case where PF is not accessible and left/right VF (or virtio) devices having same vendor and device id. hence we need to enumerate using their names to create left and right resource pools.

From current implemention of GetPfName, it looks like if PF doesn't exist, it will return VF interface name. Would you mind trying to use VF interface name in pfName selector to see if it can satisfy your request? I didn't try that myself before, just thought that might be an option when looking at the PR.

@pperiyasamy
Copy link
Contributor Author

pperiyasamy commented Jan 31, 2020

From current implemention of GetPfName, it looks like if PF doesn't exist, it will return VF interface name.

@zshi-redhat There is no interface name associated with the VF/virtio device bound with dpdk driver. so no net directory. isn't it ?

@pperiyasamy
Copy link
Contributor Author

@zshi-redhat any update on this ?

@zshi-redhat
Copy link
Collaborator

I think we probably don't want to use PCI address as a fake PF name. It might be good to come up a solution to support using DP in VM instead of having small fixes for each use case.

@pperiyasamy
Copy link
Contributor Author

I think we probably don't want to use PCI address as a fake PF name. It might be good to come up a solution to support using DP in VM instead of having small fixes for each use case.

Okay, Can we properly fix it by adding vfNames in selectors? Here is the issue #205

@pperiyasamy pperiyasamy changed the title Add VF pciAddress as PF Name when PF is not found Add vfDevices support in selectors for VM usecase Feb 13, 2020
@pperiyasamy pperiyasamy changed the title Add vfDevices support in selectors for VM usecase Add vfDevices field in selectors for VM usecase Feb 13, 2020
@lynic
Copy link

lynic commented May 11, 2020

I think we probably don't want to use PCI address as a fake PF name. It might be good to come up a solution to support using DP in VM instead of having small fixes for each use case.

HI zshi, do you have any new ideas on this PR? We also need this filter to support VM case, any update for the moment?

@pperiyasamy
Copy link
Contributor Author

I think we probably don't want to use PCI address as a fake PF name. It might be good to come up a solution to support using DP in VM instead of having small fixes for each use case.

HI zshi, do you have any new ideas on this PR? We also need this filter to support VM case, any update for the moment?

@lynic I'm still hoping to rework on this PR depending on feedback here.

@ahalimx86
Copy link
Collaborator

ahalimx86 commented May 13, 2020

@pperiyasamy I think a selector for device pci address is desirable and possbly address the VM usecases. Let's come up with more meaningful parameter name instead of "vfDevices". This selector could be applied for any pci devices -regardless of VF or PF.

Could you please update this PR?

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@pperiyasamy
Copy link
Contributor Author

@pperiyasamy I think a selector for device pci address is desirable and possbly address the VM usecases. Let's come up with more meaningful parameter name instead of "vfDevices". This selector could be applied for any pci devices -regardless of VF or PF.

Could you please update this PR?

@ahalim-intel Thanks for your attention now, updated the PR replacing "vfDevices" parameter with "pciAddresses". Hope this is more relevant and generic.

@zshi-redhat
Copy link
Collaborator

might be worth updating the readme file.

@pperiyasamy
Copy link
Contributor Author

@zshi-redhat Thanks, will update the readme. I just noticed pciAddresses is part of netDevice selectors. shouldn't it move into Common selectors ?

@ahalimx86
Copy link
Collaborator

@zshi-redhat Thanks, will update the readme. I just noticed pciAddresses is part of netDevice selectors. shouldn't it move into Common selectors ?

Yes, this selector should be common and go in DeviceSelectors struct instead.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@pperiyasamy
Copy link
Contributor Author

Hi, addressed the comments on readme file and moving pciAddresses parameter.
can you have a look now ?

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

@zshi-redhat
Copy link
Collaborator

/lgtm

@ahalimx86 ahalimx86 changed the title Add vfDevices field in selectors for VM usecase Add pciAddresses selectors for VM usecase May 28, 2020
@ahalimx86
Copy link
Collaborator

updated commit header

@lynic
Copy link

lynic commented Jun 3, 2020

This PR looks ready to be merged, is there any plan to get it in?

@ahalimx86 ahalimx86 merged commit e116e9c into k8snetworkplumbingwg:master Jun 3, 2020
@pperiyasamy pperiyasamy deleted the sriovdp_nfvi branch June 11, 2020 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants