Update error messages to show why no interface is selected

When the SRIOV network node state is not properly initialized it can hit the error "no supported NIC is selected by the nicSelector" even though the NIC may be indeed be selected. This commit updates the error message to ensure that if the user is configuring a NIC that is supported, then the error is because the SRIOV network node state is not properly initialized.
k8snetworkplumbingwg · Aug 9, 2023 · 69911e0 · 69911e0
1 parent 680654f
commit 69911e0
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 19 deletions.
diff --git a/pkg/webhook/validate.go b/pkg/webhook/validate.go
@@ -29,8 +29,8 @@ const (
 )
 
 var (
-	nodesSelected     bool
-	interfaceSelected bool
+	nodesSelected          bool
+	interfaceSelected      bool
 )
 
 func validateSriovOperatorConfig(cr *sriovnetworkv1.SriovOperatorConfig, operation v1.Operation) (bool, []string, error) {
@@ -216,6 +216,7 @@ func staticValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePol
 func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePolicy) (bool, error) {
 	nodesSelected = false
 	interfaceSelected = false
+	nodeInterfaceErrorList = make(map[string][]string)
 
 	nodeList, err := kubeclient.CoreV1().Nodes().List(context.Background(), metav1.ListOptions{
 		LabelSelector: labels.Set(cr.Spec.NodeSelector).String(),
@@ -234,7 +235,7 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo
 	for _, node := range nodeList.Items {
 		if cr.Selected(&node) {
 			nodesSelected = true
-			err = validatePolicyForNodeStateAndPolicy(nsList, npList, &node, cr)
+			err = validatePolicyForNodeStateAndPolicy(nsList, npList, &node, cr, &nodeInterfaceErrorList)
 			if err != nil {
 				return false, err
 			}
@@ -245,20 +246,31 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo
 		return false, fmt.Errorf("no matched node is selected by the nodeSelector in CR %s", cr.GetName())
 	}
 	if !interfaceSelected {
+		for nodeName, messages := range nodeInterfaceErrorList {
+			for _, message := range messages {
+				glog.V(2).Infof("%s: %s", nodeName, message)
+			}
+		}
 		return false, fmt.Errorf("no supported NIC is selected by the nicSelector in CR %s", cr.GetName())
 	}
 
 	return true, nil
 }
 
-func validatePolicyForNodeStateAndPolicy(nsList *sriovnetworkv1.SriovNetworkNodeStateList, npList *sriovnetworkv1.SriovNetworkNodePolicyList, node *corev1.Node, cr *sriovnetworkv1.SriovNetworkNodePolicy) error {
+func validatePolicyForNodeStateAndPolicy(nsList *sriovnetworkv1.SriovNetworkNodeStateList, npList *sriovnetworkv1.SriovNetworkNodePolicyList, node *corev1.Node, cr *sriovnetworkv1.SriovNetworkNodePolicy, nodeInterfaceErrorList *map[string][]string) error {
 	for _, ns := range nsList.Items {
 		if ns.GetName() == node.GetName() {
-			if err := validatePolicyForNodeState(cr, &ns, node); err != nil {
+			interfaceAndErrorList, err := validatePolicyForNodeState(cr, &ns, node)
+			if err != nil {
 				return err
 			}
+			if interfaceAndErrorList != nil {
+				*nodeInterfaceErrorList[ns.GetName()] = interfaceAndErrorList
+			}
+			break
 		}
 	}
+
 	// validate current policy against policies in API (may not be converted to SriovNetworkNodeState yet)
 	for _, np := range npList.Items {
 		if np.GetName() != cr.GetName() && np.Selected(node) {
@@ -270,28 +282,38 @@ func validatePolicyForNodeStateAndPolicy(nsList *sriovnetworkv1.SriovNetworkNode
 	return nil
 }
 
-func validatePolicyForNodeState(policy *sriovnetworkv1.SriovNetworkNodePolicy, state *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node) error {
+func validatePolicyForNodeState(policy *sriovnetworkv1.SriovNetworkNodePolicy, state *sriovnetworkv1.SriovNetworkNodeState, node *corev1.Node) ([]string, error) {
 	glog.V(2).Infof("validatePolicyForNodeState(): validate policy %s for node %s.", policy.GetName(), state.GetName())
+	interfaceSelectedForNode := false
+	noInterfacesSelectedLog := []string{}
 	for _, iface := range state.Status.Interfaces {
 		err := validateNicModel(&policy.Spec.NicSelector, &iface, node)
 		if err == nil {
 			interfaceSelected = true
+			interfaceSelectedForNode = true
 			if policy.GetName() != constants.DefaultPolicyName && policy.Spec.NumVfs == 0 {
-				return fmt.Errorf("numVfs(%d) in CR %s is not allowed", policy.Spec.NumVfs, policy.GetName())
+				return nil, fmt.Errorf("numVfs(%d) in CR %s is not allowed", policy.Spec.NumVfs, policy.GetName())
 			}
 			if policy.Spec.NumVfs > iface.TotalVfs && iface.Vendor == IntelID {
-				return fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs)
+				return nil, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs)
 			}
 			if policy.Spec.NumVfs > MlxMaxVFs && iface.Vendor == MellanoxID {
-				return fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs)
+				return nil, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs)
 			}
 			// vdpa: only mellanox cards are supported
 			if policy.Spec.VdpaType == constants.VdpaTypeVirtio && iface.Vendor != MellanoxID {
-				return fmt.Errorf("vendor(%s) in CR %s not supported for virtio-vdpa", iface.Vendor, policy.GetName())
+				return nil, fmt.Errorf("vendor(%s) in CR %s not supported for virtio-vdpa", iface.Vendor, policy.GetName())
 			}
+		} else {
+			errorMessage := fmt.Sprintf("Interface: %s was not selected, since NIC model could not be validated due to the following error: %s \n", iface.Name, err)
+			noInterfacesSelectedLog = append(noInterfacesSelectedLog, errorMessage)
 		}
 	}
-	return nil
+
+	if !interfaceSelectedForNode {
+		return noInterfacesSelectedLog, nil
+	}
+	return nil, nil
 }
 
 func validatePolicyForNodePolicy(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {

diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go
@@ -253,7 +253,7 @@ func TestValidatePolicyForNodeStateWithValidPolicy(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
@@ -279,7 +279,7 @@ func TestValidatePolicyForNodeStateWithInvalidNumVfsPolicy(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("numVfs(%d) in CR %s exceed the maximum allowed value(%d)", policy.Spec.NumVfs, policy.GetName(), state.Status.Interfaces[0].TotalVfs))))
 }
 
@@ -608,7 +608,7 @@ func TestValidatePolicyForNodeStateVdpaWithNotSupportedVendor(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("vendor(%s) in CR %s not supported for virtio-vdpa", state.Status.Interfaces[0].Vendor, policy.Name))))
 }
 
@@ -635,7 +635,7 @@ func TestValidatePolicyForNodeStateWithInvalidDevice(t *testing.T) {
 	g.Expect(err).ToNot(HaveOccurred())
 	g.Expect(cfg).ToNot(BeNil())
 	kubeclient = kubernetes.NewForConfigOrDie(cfg)
-	err = validatePolicyForNodeState(policy, state, NewNode())
+	_, err = validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
@@ -657,7 +657,7 @@ func TestValidatePolicyForNodeStateWithInvalidPfName(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(interfaceSelected).To(Equal(false))
 }
@@ -680,7 +680,7 @@ func TestValidatePolicyForNodeStateWithValidPfName(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(interfaceSelected).To(Equal(true))
 }
@@ -720,7 +720,7 @@ func TestValidatePolicyForNodeStateWithValidNetFilter(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(interfaceSelected).To(Equal(true))
 }
@@ -784,7 +784,7 @@ func TestValidatePolicyForNodeStateWithValidVFAndNetFilter(t *testing.T) {
 		},
 	}
 	g := NewGomegaWithT(t)
-	err := validatePolicyForNodeState(policy, state, NewNode())
+	_, err := validatePolicyForNodeState(policy, state, NewNode())
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(interfaceSelected).To(Equal(true))
 }