-
Notifications
You must be signed in to change notification settings - Fork 114
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
Reload VF driver only when rdma is enabled #207
Conversation
/cc @Mmduh-483 |
@pliurh Will this fix be backported to lower versions? If so, I'd prefer to not changing API. My understanding is we will eventually remove this once kernel fix is in place. |
If we want to fix that 4.6 bug, we need to backport it. Without modifying API, the config daemon cannot know if a VF will be used for ROCE. I don't know when it will be fixed in the kernel. |
pkg/utils/utils.go
Outdated
func setVfsGuid(iface *sriovnetworkv1.Interface) error { | ||
glog.Infof("setVfsGuid(): device %s", iface.PciAddress) | ||
pfLink, err := netlink.LinkByName(iface.Name) | ||
func setVfsGuid(vfAddr string, pfLink netlink.Link) error { |
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.
imo the name: setVfGUID
or setVfGuid
is more appropriate now.
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.
fixed
pkg/utils/utils.go
Outdated
func setVfsAdminMac(iface *sriovnetworkv1.InterfaceExt) error { | ||
glog.Infof("setVfsAdminMac(): device %s", iface.PciAddress) | ||
pfLink, err := netlink.LinkByName(iface.Name) | ||
func setVfsAdminMac(vfAddr string, pfLink netlink.Link, isRdma bool) error { |
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.
imo the name: setVFAdminMac or setVFAdminMAC is more appropriate now.
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.
fixed
pkg/utils/utils.go
Outdated
return err | ||
} | ||
if isRdma { | ||
glog.Infof("setVfsAdminMac(): unbind driver for %s", vfAddr) |
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 add a method to handle the driver unbind and move the unbind logic out of SetVfAdminMac and SetVfGuid ?
so that these functions do exactly what their name say ?
e.g add a function : UnbindDriverIfNeeded()
and call it from configureSriovDevice
?
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.
fixed
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.
LGTM, thanks Peng !
When merging please add the description of this PR to the merge commit so its clear why this change was introduced
To set the GUID to VF, we need to set the admin MAC and reload the VF driver. It will take a long time if users set big 'numVFs'. This PR changes the logic to only reload the VF driver when isRdma is set. So that, users not using ROCE will no longer need to wait for the VF driver reload.
SriovNetworkNodeState is outdated. IsRdma field was introduced in the PR k8snetworkplumbingwg#207. This patch syncs CRDs between config/crd/bases and helm chart.
SriovNetworkNodeState is outdated. IsRdma field was introduced in the PR k8snetworkplumbingwg#207. This patch syncs CRDs between config/crd/bases and helm chart.
To set the GUID to VF, we need to set the admin MAC and reload the VF driver. It will take a long time if users set big 'numVFs'. This PR changes the logic to only reload the VF driver when
isRdma
is set. So that, users not using ROCE will no longer need to wait for the VF driver reload.