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

Re-introduce the check if the number of VFs is 0 #472

Merged
merged 1 commit into from
Jul 13, 2023
Merged

Re-introduce the check if the number of VFs is 0 #472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions pkg/plugins/generic/generic_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,17 @@ func needDrainNode(desired sriovnetworkv1.Interfaces, current sriovnetworkv1.Int
configured := false
for _, iface := range desired {
if iface.PciAddress == ifaceStatus.PciAddress {
// TODO: no need to perform further checks if ifaceStatus.NumVfs equals to 0
// once https://github.com/kubernetes/kubernetes/issues/109595 will be fixed
configured = true
if ifaceStatus.NumVfs == 0 {
adrianchiris marked this conversation as resolved.
Show resolved Hide resolved
glog.V(2).Infof("generic-plugin needDrainNode(): no need drain, for PCI address %s current NumVfs is 0", iface.PciAddress)
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: return here like we do below ? WDYT ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no we can't because we need to continue and check all the other status vs desire.

in the NeedUpdate case we return because if we find even one that needs to update we need to drain, but that is not the case if we find one that doesn't need to drain we need to check for all of them

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, didnt see that nested loop. (it was not showing in browser) :\

yes you are right !

}
if utils.NeedUpdate(&iface, &ifaceStatus) {
glog.V(2).Infof("generic-plugin needDrainNode(): need drain, PF %s request update", iface.PciAddress)
glog.V(2).Infof("generic-plugin needDrainNode(): need drain, for PCI address %s request update", iface.PciAddress)
needDrain = true
return
}
glog.V(2).Infof("generic-plugin needDrainNode(): no need drain, expect NumVfs %v, current NumVfs %v", iface.NumVfs, ifaceStatus.NumVfs)
glog.V(2).Infof("generic-plugin needDrainNode(): no need drain,for PCI address %s expect NumVfs %v, current NumVfs %v", iface.PciAddress, iface.NumVfs, ifaceStatus.NumVfs)
}
}
if !configured && ifaceStatus.NumVfs > 0 {
Expand Down