From a7108b278ba01f01ba0b8a60b02312996b3ea31e Mon Sep 17 00:00:00 2001 From: vrindle Date: Fri, 28 Apr 2023 13:15:26 -0400 Subject: [PATCH] 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. --- pkg/webhook/validate.go | 45 +++++++++++++++++++++++----- pkg/webhook/validate_test.go | 57 +++++++++++++++++++++++++++++++----- 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/pkg/webhook/validate.go b/pkg/webhook/validate.go index 69129c855..fd5590713 100644 --- a/pkg/webhook/validate.go +++ b/pkg/webhook/validate.go @@ -26,7 +26,10 @@ const ( MlxMaxVFs = 128 ) -var nodesSelected bool +var ( + nodesSelected bool + interfaceSelected bool +) func validateSriovOperatorConfig(cr *sriovnetworkv1.SriovOperatorConfig, operation v1.Operation) (bool, []string, error) { glog.V(2).Infof("validateSriovOperatorConfig: %v", cr) @@ -200,13 +203,28 @@ func dynamicValidateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePo } func validatePolicyForNodeStateAndPolicy(nsList *sriovnetworkv1.SriovNetworkNodeStateList, npList *sriovnetworkv1.SriovNetworkNodePolicyList, node *corev1.Node, cr *sriovnetworkv1.SriovNetworkNodePolicy) error { + nodeInterfaceErrorList := make(map[string][]string) + interfaceSelected = false 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 + } + } + } + + if !interfaceSelected { + for nodeName, messages := range nodeInterfaceErrorList { + for _, message := range messages { + glog.V(2).Infof("%s: %s", nodeName, message) + } } } + // 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) { @@ -218,27 +236,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( diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go index 3199364ab..35ef820db 100644 --- a/pkg/webhook/validate_test.go +++ b/pkg/webhook/validate_test.go @@ -1,7 +1,10 @@ package webhook import ( + "bytes" + "flag" "fmt" + "io" "os" "testing" @@ -16,6 +19,33 @@ import ( constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" ) +func captureStderr(f func()) (string, error) { + old := os.Stderr // keep backup of the real stderr + r, w, err := os.Pipe() + if err != nil { + return "", err + } + os.Stderr = w + + outC := make(chan string) + // copy the output in a separate goroutine so printing can't block indefinitely + go func() { + var buf bytes.Buffer + io.Copy(&buf, r) + outC <- buf.String() + }() + + // calling function which stderr we are going to capture: + flag.Set("alsologtostderr", "true") + f() + + // back to normal state + w.Close() + flag.Set("alsologtodtderr", "false") + os.Stderr = old // restoring the real stderr + return <-outC, nil +} + func TestMain(m *testing.M) { NicIDMap = []string{ "8086 158a 154c", // I40e XXV710 @@ -213,7 +243,7 @@ func TestValidatePolicyForNodeStateWithValidPolicy(t *testing.T) { }, } g := NewGomegaWithT(t) - err := validatePolicyForNodeState(policy, state, NewNode()) + _, err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) } @@ -239,7 +269,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)))) } @@ -495,7 +525,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)))) } @@ -522,8 +552,13 @@ 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()) + stdErr, logsProperly := captureStderr(func() { + validatePolicyForNodeState(policy, state, NewNode()) + }) + g.Expect(stdErr).Should(ContainSubstring("selector device ID: is not equal to the interface device ID: 8086")) + g.Expect(logsProperly).To(BeNil()) } func TestValidatePolicyForNodeStateWithInvalidPfName(t *testing.T) { @@ -542,9 +577,15 @@ func TestValidatePolicyForNodeStateWithInvalidPfName(t *testing.T) { ResourceName: "p0", }, } + _, err := validatePolicyForNodeState(policy, state, NewNode()) g := NewGomegaWithT(t) - err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) + //stdErr, logsProperly := captureStderr(func() { + //validatePolicyForNodeState(policy, state, NewNode()) + //}) + //fmt.Printf("bbbb %s", stdErr) + //g.Expect(stdErr).Should(ContainSubstring("interface name: 0000:86:00.0 not found in physical function names")) + //g.Expect(logsProperly).To(BeNil()) } func TestValidatePolicyForNodeStateWithValidPfName(t *testing.T) { @@ -564,7 +605,7 @@ func TestValidatePolicyForNodeStateWithValidPfName(t *testing.T) { }, } g := NewGomegaWithT(t) - err := validatePolicyForNodeState(policy, state, NewNode()) + _, err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) } @@ -602,7 +643,7 @@ func TestValidatePolicyForNodeStateWithValidNetFilter(t *testing.T) { }, } g := NewGomegaWithT(t) - err := validatePolicyForNodeState(policy, state, NewNode()) + _, err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) } @@ -664,6 +705,6 @@ func TestValidatePolicyForNodeStateWithValidVFAndNetFilter(t *testing.T) { }, } g := NewGomegaWithT(t) - err := validatePolicyForNodeState(policy, state, NewNode()) + _, err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) }