-
Notifications
You must be signed in to change notification settings - Fork 116
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 more checks on generic plugin to discover discrepancies from the desired state #587
Add more checks on generic plugin to discover discrepancies from the desired state #587
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
7d4380a
to
2e88940
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
2e88940
to
f942ac6
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
f942ac6
to
54a1148
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 9206688050Details
💛 - Coveralls |
54a1148
to
efe6e46
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
Thanks for your PR,
To skip the vendors CIs use one of:
|
3826a39
to
c07958d
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
c07958d
to
69c9dd5
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
69c9dd5
to
d18aae1
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
d18aae1
to
7fe0497
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
7fe0497
to
4d50701
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
/test-e2e-all |
4d50701
to
f9e4337
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
f9e4337
to
1ffcbc3
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@adrianchiris PTAL |
edcd15e
to
b30ab24
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
api/v1/helper.go
Outdated
@@ -300,6 +306,28 @@ func NeedToUpdateSriov(ifaceSpec *Interface, ifaceStatus *InterfaceExt) bool { | |||
return true | |||
} | |||
|
|||
if strings.EqualFold(ifaceStatus.LinkType, consts.LinkTypeETH) { |
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.
nit: any chance to have one check to avoid duplication ?
e.g if (ETH && RDMA) || IB { ....}
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.
Yeap, makes sense. Addressing.
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.
overall LGTM, one minor nit, sorry for not raising it in the previous round.
b30ab24
to
83c8397
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
pkg/host/internal/network/network.go
Outdated
@@ -187,6 +190,36 @@ func (n *network) GetNetDevMac(ifaceName string) string { | |||
return link.Attrs().HardwareAddr.String() | |||
} | |||
|
|||
// GetNetDevNodeGUID returns the network interface node GUID if device is RDMA capable otherwise returns empty string | |||
func (n *network) GetNetDevNodeGUID(pciAddr string) string { | |||
log.Log.V(2).Info("GetNetDevNodeGUID(): get node GUID", "pciAddr", pciAddr) |
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.
This function gets called during the polling of NIC status, for each configured VF.
I would avoid this logging statement, as it increases config-daemon logs without adding specific value.
You can have a look at the end2end job artifact
https://github.com/k8snetworkplumbingwg/sriov-network-operator/actions/runs/9005014290/artifacts/1484967983
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.
Agreed, removed.
83c8397
to
0ecd227
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
sorry it took soo much time to merge this one.
It's almost remove just small comments and we can merge this PR
pkg/host/internal/network/network.go
Outdated
} | ||
|
||
if link.Attrs().Flags&net.FlagUp == 0 { | ||
return "down" |
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.
please put this one as a const and use it also in https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/587/files#diff-371d95e79c61d8cc1e6791d27e009289552efcaada38f9b2fc75ebb487c90ea7R275
pkg/host/internal/network/network.go
Outdated
return "down" | ||
} | ||
|
||
return "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.
please put this one as a const
pkg/host/internal/sriov/sriov.go
Outdated
@@ -571,7 +574,7 @@ func (s *sriov) configSriovDevice(iface *sriovnetworkv1.Interface, skipVFConfigu | |||
if err != nil { | |||
return err | |||
} | |||
if pfLink.Attrs().OperState != netlink.OperUp { | |||
if pfLink.Attrs().Flags&net.FlagUp == 0 { |
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.
please put this one as part of the netlinkLib
lets use it here and on the other place in the code
Today, we are missing 2 checks on settings that sriov netop is configuring: * PF is up * GUID is set Without checking for these 2, we risk that changes made by the user directly to the system are not reconciled by the netop leaving the system in a bad state. This commit is adding those checks. Signed-off-by: Vasilis Remmas <[email protected]>
0ecd227
to
5f8d1a1
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@SchSeba please have a look, I addressed your comments. |
Signed-off-by: Vasilis Remmas <[email protected]>
5f8d1a1
to
5f3c4e9
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
nice work!
sorry about the amount of time the review took
Merge issue coming from - k8snetworkplumbingwg#690 - k8snetworkplumbingwg#587 Signed-off-by: Andrea Panattoni <[email protected]>
Merge issue coming from - k8snetworkplumbingwg#690 - k8snetworkplumbingwg#587 Signed-off-by: Andrea Panattoni <[email protected]>
Merge issue coming from - k8snetworkplumbingwg#690 - k8snetworkplumbingwg#587 Signed-off-by: Andrea Panattoni <[email protected]>
Merge issue coming from - k8snetworkplumbingwg#690 - k8snetworkplumbingwg#587 Signed-off-by: Andrea Panattoni <[email protected]>
Summary
This PR does the following things:
OnNodeStateChange()
function to indicate when we are draining the nodeNeedToUpdateSriov()
helper function to ensure that:SriovNetworkNodeState
struct to facilitate the abovePartial Reconciliation
Today we reconcile on
.spec
change since we skip reconciliation if the generation of the object is the same as the object that was last reconciled successfully. Therefore, changes in.status
, like what we try to detect with that PR, are not reconciled unless the.spec
changes when the config daemon runs continuously (i.e. no restart). On daemon restart, the.status
field will be reconciled the first time until the last successfully reconciled generation is saved in memory.Decisions
OnNodeStateChange()
instead ofNeedToUpdateSriov()
because that one looks to be the most impactful for the whole system (i.e. decides whether to drain). I can add on bothApply()
andOnNodeStateChange()
if we choose to split theNeedToUpdateSriov()
(see open questions below).Open questions
.status
(i.e. full reconciliation of changes that the controller does to the system)? (relatively big change in the operator behaviour I suppose)Apply()
andOnNodeStateChange()
to be something different thanNeedToUpdateSriov()
./sys/class/net/*
or use netlink? I see both approaches:sriov-network-operator/pkg/host/network.go
Lines 195 to 205 in 09c987a
sriov-network-operator/pkg/host/sriov.go
Lines 618 to 635 in 09c987a
Based on the answer I will use or discard d18aae1