Never reduce MTU on PF #489
Conversation
In case a policy specifying a different MTU, in the past, the MTU of the PF associated with the VFs would also be updated. Since reducing the MTU on PF could cause issues when the PF is used by other things too (i.e. primary network), reducing the MTU could cause connectivity issues. Therefore, take the conservative approach of never reducing the MTU on the PF when configuring the MTU on the VF. Signed-off-by: Balazs Nemeth <[email protected]>
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
LGTM |
pkg/utils/utils.go
Outdated
| @@ -263,10 +263,14 @@ func configSriovDevice(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetwor | |||
| } | |||
| // set PF mtu | |||
| if iface.Mtu > 0 && iface.Mtu != ifaceStatus.Mtu { | |||
There was a problem hiding this comment.
i would simply:
if iface.Mtu > 0 && iface.Mtu > ifaceStatus.Mtu {
err = setNetdevMTU(iface.PciAddress, iface.Mtu)
if err != nil {
glog.Warningf("configSriovDevice(): fail to set mtu for PF %s: %v", iface.PciAddress, err)
return err
}
}
no need to emit the warning IMO in case desired VF MTU is less than PF mtu.
There was a problem hiding this comment.
looking at previous patch set, i see thats how you originally implemented it.
personally i prefer this way because its an allowed configuration and a resonable one. we should not emit warning to clutter an already cluttered log.
There was a problem hiding this comment.
I have no preference either way. I thought having a log would make it easier for people to figure out that their MTUs aren't being applied. Otherwise users will need to read to code to know that this is the desired behavior.
|
I'm ok to revert the last change requested by William or to keep it. I do not have any preference. @wizhaoredhat What is your preference? It seems to me that @adrianchiris prefers a less polluted log file. |
|
Just wondering, Does this handle the case if the MTU for the PF changes after the policy has been applied? Should we consider this case or not? |
i dont think we consider it for any parameter which the daemon configures. its generally assumed that no one tinkers with these configurations during config daemon run. generally speaking, we could add similar behaviour to nodepolicy controller where we "periodically reconcile" the node. |
|
right, but imo that's out of scope for this PR. |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
I've reverted the requested change from @wizhaoredhat . @adrianchiris PTAL one final time and merge it if ok. |
In case a policy specifying a different MTU, in the past, the MTU of the PF associated with the VFs would also be updated. Since reducing the MTU on PF could cause issues when the PF is used by other things too (i.e. primary network), reducing the MTU could cause connectivity issues. Therefore, take the conservative approach of never reducing the MTU on the PF when configuring the MTU on the VF.