-
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
Fix equality comparison of policy specs #538
Fix equality comparison of policy specs #538
Conversation
When the last policy is deleted, the network node states objects are not updated because the DeepDerivative function only compares the DPConfigVersion and ignores the Interfaces. The new spec has no interfaces. The node states are eventually updated when the resync period forces the reconcilation loop to trigger again. This causes a long delay on the processing of the deletion by the sriov config daemon. This started to be an issue when we changed the order of the methdos syncAllSriovNetworkNodeStates and syncDevicePluginConfigMap. The last one was run first and the param DPConfigVersion was always different for this particular case.
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 6767949555
💛 - Coveralls |
Please, consider adding a unit test to cover this case: func Test_NodeStateIsUpdatedWhenTheLastPolicyIsDeleted(t *testing.T) {
defaultPolicy := sriovnetworkv1.SriovNetworkNodePolicy{
ObjectMeta: metav1.ObjectMeta{Name: consts.DefaultPolicyName},
Spec: v1.SriovNetworkNodePolicySpec{
NumVfs: 0,
NodeSelector: make(map[string]string),
NicSelector: sriovnetworkv1.SriovNetworkNicSelector{},
},
}
p1 := sriovnetworkv1.SriovNetworkNodePolicy{
ObjectMeta: metav1.ObjectMeta{Name: "p1"},
Spec: v1.SriovNetworkNodePolicySpec{
NicSelector: v1.SriovNetworkNicSelector{PfNames: []string{"ens1f0"}},
NumVfs: 4,
},
}
reconciler := SriovNetworkNodePolicyReconciler{}
node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}}
nodeState := sriovnetworkv1.SriovNetworkNodeState{
ObjectMeta: metav1.ObjectMeta{Name: node.Name, Namespace: namespace},
Status: v1.SriovNetworkNodeStateStatus{
Interfaces: []sriovnetworkv1.InterfaceExt{{
Name: "ens1f0",
}},
},
}
scheme := runtime.NewScheme()
utilruntime.Must(sriovnetworkv1.AddToScheme(scheme))
reconciler.Client = fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(&nodeState).
Build()
reconciler.Scheme = scheme
policyList := sriovnetworkv1.SriovNetworkNodePolicyList{Items: []sriovnetworkv1.SriovNetworkNodePolicy{defaultPolicy, p1}}
err := reconciler.syncSriovNetworkNodeState(context.Background(),
&defaultPolicy,
&policyList,
&sriovnetworkv1.SriovNetworkNodeState{ObjectMeta: metav1.ObjectMeta{Name: node.Name, Namespace: namespace}},
&node,
"111")
assert.NoError(t, err)
err = reconciler.Client.Get(context.Background(), types.NamespacedName{Name: node.Name, Namespace: namespace}, &nodeState)
assert.NoError(t, err)
assert.Equal(t, len(nodeState.Spec.Interfaces), 1)
err = reconciler.syncSriovNetworkNodeState(context.Background(),
&defaultPolicy,
&sriovnetworkv1.SriovNetworkNodePolicyList{Items: []sriovnetworkv1.SriovNetworkNodePolicy{defaultPolicy}},
&sriovnetworkv1.SriovNetworkNodeState{ObjectMeta: metav1.ObjectMeta{Name: node.Name, Namespace: namespace}},
&node,
"111")
assert.NoError(t, err)
err = reconciler.Client.Get(context.Background(), types.NamespacedName{Name: node.Name, Namespace: namespace}, &nodeState)
assert.NoError(t, err)
assert.Equal(t, len(nodeState.Spec.Interfaces), 0)
} |
Please create a pr with the test you posted. Let's not delay the merge of this fix |
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.
Fine to have the unit test in a different PR to avoid regressions on this.
When the last policy is deleted, the network node states objects are not updated because the DeepDerivative function only compares the DPConfigVersion and ignores the Interfaces. The new spec has no interfaces. The node states are eventually updated when the resync period forces the reconcilation loop to trigger again.
This causes a long delay on the processing of the deletion by the sriov config daemon. This started to be an issue when we changed the order of the methdos syncAllSriovNetworkNodeStates and syncDevicePluginConfigMap. The last one was run first and the param DPConfigVersion was always different for this particular case.