-
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
Set link state up for target ports of SriovNetworkNodePolicy for kubernetes cluster #72
Set link state up for target ports of SriovNetworkNodePolicy for kubernetes cluster #72
Conversation
// Set PF link up for kubernetes cluster | ||
if ClusterType == ClusterTypeKubernetes && ifaceStatus.Name != "" { | ||
pfLink, err := netlink.LinkByName(ifaceStatus.Name) | ||
if err != nil { |
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.
you can check if the pfLink is not up with
pfLink.Attrs().OperState == OperUp or something like that.
Also in what case ifaceStatus.Name will be ""?
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.
If some modules are not loaded like ipoib for IB interfaces
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.
Added condition for state and removed the condition for name
304caa3
to
de56cca
Compare
pkg/utils/utils.go
Outdated
@@ -315,6 +315,19 @@ func configSriovDevice(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetwor | |||
} | |||
} | |||
} | |||
// Set PF link up for kubernetes cluster | |||
if ClusterType == ClusterTypeKubernetes { |
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.
Is there a reason this is special for k8s cluster?
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.
So we encounter this is k8s vanilla, we can remove this check if you think it make sense openshift.
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 Moshe!
I think it's fine to enable it for both cluster types, as long as there is numvfs configured for that device, link state will be set to up.
/cc @pliurh
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.
In openshift, the NetworkManager will do the job. Shall we check whether the link-state first before setting it to up?
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.
There is a check for the state if pfLink.Attrs().OperState != netlink.OperUp
…rnetes cluster Signed-off-by: Mamduh Alassi <[email protected]>
de56cca
to
ab7165e
Compare
fixes #45