-
Notifications
You must be signed in to change notification settings - Fork 106
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 support for running in a virtual machine -- Remove PF dependency #369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Aaron for working on the patches, added few comments inline.
I think the overall design looks good to me:
- A virtual plugin for VM use case
- Auto detect platform type and invoke the virtual plugin
deploy/crds/sriovnetwork.openshift.io_sriovnetworknodepolicies_crd.yaml
Outdated
Show resolved
Hide resolved
iface.TotalVfs = 1 | ||
iface.NumVfs = 1 | ||
|
||
vf := sriovnetworkv1.VirtualFunction{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider the VM VF as a individual device (just like a regular PF device in the host) and not populate the vf fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could modify current numVfs range to allow 0 numVfs be configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to treat VFs as individual devices. The problem is that the current code seems to be keyed to TotalVFs/NumVfs and I took the approach of setting to 1 to reduce the number code changes. I can take a look at your suggestion.
6c9ddd6
to
29a1f46
Compare
794990d
to
b641804
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally good to me.
I think we might want to do two things in a follow-up PRs:
- enable use of numVfs=0 so that virtual-plugin doesn't need to populate
InterfaceExt.VFs
- move platform specific code into a separate file.
ec2b90a
to
69376a8
Compare
/cc @pliurh |
/retest |
var err error | ||
|
||
if platformType == Virtual { | ||
iface, err = utils.DiscoverSriovDevicesVirtual() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use one function to discover devices for both baremetal and virtual nodes? We can pass the platformType
to DiscoverSriovDevices
so that the VFs can be treated differently on a virtual node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to completely separate the virtual path from the baremetal path. I didn't think it was clean to include a bunch of checks in the discover function. I can change if you feel strongly about it...
format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed
var err error | ||
|
||
if platformType == Virtual { | ||
iface, err = utils.DiscoverSriovDevicesVirtual() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to completely separate the virtual path from the baremetal path. I didn't think it was clean to include a bunch of checks in the discover function. I can change if you feel strongly about it...
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atyronesmith, pliurh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
config-daemon: fix NM udev path
This PR adds support for running (initially) in an OpenShift on OpenStack environment. In a VM environment, only VFs are attached (no PF) and the VFs represent an underlying OpenStack network. This PR removes the necessity of accessing a PF and uses additional information passed into the VM by the OpenStack metadata service to include underlying network mappings in the sriovnetworknodestate. In addition, the exposed underlying network information can be used to select networks for a policy.
This PR is in conjunction with a PR in k8snetworkplumbingwg/sriov-network-device-plugin repository.
This PR is to allow a review of the concept.