Skip to content

Commit

Permalink
Merge pull request #434 from vrindle/fix_error_message_revised
Browse files Browse the repository at this point in the history 
Update error messages to show why no interface is selected
  • Loading branch information
@bn222
bn222 authored Oct 16, 2023
2 parents 49fff50 + efb48ad commit a3ea7b1
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 31 deletions.
49 changes: 36 additions & 13 deletions pkg/webhook/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,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(),
Expand All @@ -241,7 +242,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
}
Expand All @@ -252,20 +253,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 {
return fmt.Errorf("%s node(%s)", err.Error(), node.Name)
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) {
Expand All @@ -277,42 +289,53 @@ 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
var 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) interface(%s)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs, iface.Name)
return nil, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d) interface(%s)", policy.Spec.NumVfs, policy.GetName(), iface.TotalVfs, iface.Name)
}
if policy.Spec.NumVfs > MlxMaxVFs && iface.Vendor == MellanoxID {
return fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d) interface(%s)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs, iface.Name)
return nil, fmt.Errorf("numVfs(%d) in CR %s exceed the maximum allowed value(%d) interface(%s)", policy.Spec.NumVfs, policy.GetName(), MlxMaxVFs, iface.Name)
}

// Externally create validations
if policy.Spec.ExternallyManaged {
if policy.Spec.NumVfs > iface.NumVfs {
return fmt.Errorf("numVfs(%d) in CR %s is higher than the virtual functions allocated for the PF externally value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.NumVfs)
return nil, fmt.Errorf("numVfs(%d) in CR %s is higher than the virtual functions allocated for the PF externally value(%d)", policy.Spec.NumVfs, policy.GetName(), iface.NumVfs)
}

if policy.Spec.Mtu != 0 && policy.Spec.Mtu > iface.Mtu {
return fmt.Errorf("MTU(%d) in CR %s is higher than the MTU for the PF externally value(%d)", policy.Spec.Mtu, policy.GetName(), iface.Mtu)
return nil, fmt.Errorf("MTU(%d) in CR %s is higher than the MTU for the PF externally value(%d)", policy.Spec.Mtu, policy.GetName(), iface.Mtu)
}

if policy.Spec.LinkType != "" && strings.ToLower(policy.Spec.LinkType) != strings.ToLower(iface.LinkType) {
return fmt.Errorf("LinkType(%s) in CR %s is not equal to the LinkType for the PF externally value(%s)", policy.Spec.LinkType, policy.GetName(), iface.LinkType)
return nil, fmt.Errorf("LinkType(%s) in CR %s is not equal to the LinkType for the PF externally value(%s)", policy.Spec.LinkType, policy.GetName(), iface.LinkType)
}
}
// vdpa: only mellanox cards are supported
if (policy.Spec.VdpaType == constants.VdpaTypeVirtio || policy.Spec.VdpaType == constants.VdpaTypeVhost) && iface.Vendor != MellanoxID {
return fmt.Errorf("vendor(%s) in CR %s not supported for vdpa interface(%s)", iface.Vendor, policy.GetName(), iface.Name)
return nil, fmt.Errorf("vendor(%s) in CR %s not supported for vdpa interface(%s)", iface.Vendor, policy.GetName(), iface.Name)
}
} 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 {
Expand Down
36 changes: 18 additions & 18 deletions pkg/webhook/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ func TestValidatePolicyForNodeStateWithValidPolicy(t *testing.T) {
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).NotTo(HaveOccurred())
}

Expand All @@ -282,7 +282,7 @@ func TestValidatePolicyForNodeStateWithInvalidNumVfsPolicy(t *testing.T) {
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).To(MatchError("numVfs(65) in CR p1 exceed the maximum allowed value(64) interface(ens803f0)"))
}

Expand All @@ -309,7 +309,7 @@ func TestValidatePolicyForNodeStateWithInvalidNumVfsExternallyCreated(t *testing
},
}
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 is higher than the virtual functions allocated for the PF externally value(%d)", policy.Spec.NumVfs, policy.GetName(), state.Status.Interfaces[0].NumVfs))))
}

Expand All @@ -336,7 +336,7 @@ func TestValidatePolicyForNodeStateWithValidNumVfsExternallyCreated(t *testing.T
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())
}

Expand All @@ -363,7 +363,7 @@ func TestValidatePolicyForNodeStateWithValidLowerNumVfsExternallyCreated(t *test
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())
}

Expand Down Expand Up @@ -500,7 +500,7 @@ func TestValidatePolicyForNodeStateWithExternallyManageAndMTU(t *testing.T) {
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())
}

Expand Down Expand Up @@ -528,7 +528,7 @@ func TestValidatePolicyForNodeStateWithExternallyManageAndDifferentMTU(t *testin
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).To(HaveOccurred())
}

Expand Down Expand Up @@ -556,16 +556,16 @@ func TestValidatePolicyForNodeStateWithExternallyManageAndLinkType(t *testing.T)
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())

policy.Spec.LinkType = "eth"
err = validatePolicyForNodeState(policy, state, NewNode())
_, err = validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())

policy.Spec.LinkType = "ETH"
state.Status.Interfaces[0].LinkType = "eth"
err = validatePolicyForNodeState(policy, state, NewNode())
_, err = validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).ToNot(HaveOccurred())
}

Expand Down Expand Up @@ -594,7 +594,7 @@ func TestValidatePolicyForNodeStateWithExternallyManageAndDifferentLinkType(t *t
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).To(HaveOccurred())
}

Expand Down Expand Up @@ -994,7 +994,7 @@ func TestValidatePolicyForNodeStateVirtioVdpaWithNotSupportedVendor(t *testing.T
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).To(MatchError("vendor(8086) in CR p1 not supported for vdpa interface(ens803f0)"))
}

Expand All @@ -1021,7 +1021,7 @@ func TestValidatePolicyForNodeStateVhostVdpaWithNotSupportedVendor(t *testing.T)
},
}
g := NewGomegaWithT(t)
err := validatePolicyForNodeState(policy, state, NewNode())
_, err := validatePolicyForNodeState(policy, state, NewNode())
g.Expect(err).To(MatchError("vendor(8086) in CR p1 not supported for vdpa interface(ens803f0)"))
}

Expand All @@ -1048,7 +1048,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())
}

Expand All @@ -1070,7 +1070,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))
}
Expand All @@ -1093,7 +1093,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))
}
Expand Down Expand Up @@ -1133,7 +1133,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))
}
Expand Down Expand Up @@ -1197,7 +1197,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))
}

0 comments on commit a3ea7b1

Please sign in to comment.