Fix root device overlapping

api/v1/helper.go

@@ -406,7 +406,7 @@ func (p *SriovNetworkNodePolicy) Apply(state *SriovNetworkNodeState, equalPriori
 				ExternallyManaged: p.Spec.ExternallyManaged,
 			}
 			if p.Spec.NumVfs > 0 {
-				group, err := p.generateVfGroup(&iface)
+				group, err := p.generatePfNameVfGroup(&iface)
 				if err != nil {
 					return err
 				}
@@ -475,13 +475,13 @@ func (gr VfGroup) isVFRangeOverlapping(group VfGroup) bool {
 	return IndexInRange(rngSt, group.VfRange) || IndexInRange(rngEnd, group.VfRange)
 }
 
-func (p *SriovNetworkNodePolicy) generateVfGroup(iface *InterfaceExt) (*VfGroup, error) {
+func (p *SriovNetworkNodePolicy) generatePfNameVfGroup(iface *InterfaceExt) (*VfGroup, error) {
 	var err error
 	pfName := ""
 	var rngStart, rngEnd int
 	found := false
 	for _, selector := range p.Spec.NicSelector.PfNames {
-		pfName, rngStart, rngEnd, err = ParsePFName(selector)
+		pfName, rngStart, rngEnd, err = ParseVfRange(selector)
 		if err != nil {
 			log.Error(err, "Unable to parse PF Name.")
 			return nil, err
@@ -534,15 +534,27 @@ func parseRange(r string) (rngSt, rngEnd int, err error) {
 	return
 }
 
-// Parse PF name with VF range
-func ParsePFName(name string) (ifName string, rngSt, rngEnd int, err error) {
+// SplitDeviceFromRange return the device name and the range.
+// the split is base on #
+func SplitDeviceFromRange(device string) (string, string) {
+	if strings.Contains(device, "#") {
+		fields := strings.Split(device, "#")
+		return fields[0], fields[1]
+	}
+
+	return device, ""
+}
+
+// ParseVfRange: parse a device with VF range
+// this can be rootDevices or PFName
+// if no range detect we just return the device name
+func ParseVfRange(device string) (rootDeviceName string, rngSt, rngEnd int, err error) {
 	rngSt, rngEnd = invalidVfIndex, invalidVfIndex
-	if strings.Contains(name, "#") {
-		fields := strings.Split(name, "#")
-		ifName = fields[0]
-		rngSt, rngEnd, err = parseRange(fields[1])
+	rootDeviceName, splitRange := SplitDeviceFromRange(device)
+	if splitRange != "" {
+		rngSt, rngEnd, err = parseRange(splitRange)
 	} else {
-		ifName = name
+		rootDeviceName = device
 	}
 	return
 }

pkg/webhook/validate.go

@@ -367,6 +367,11 @@ func validatePolicyForNodePolicy(current *sriovnetworkv1.SriovNetworkNodePolicy,
 		return err
 	}
 
+	err = validateRootDevices(current, previous)
+	if err != nil {
+		return err
+	}
+
 	err = validateExludeTopologyField(current, previous)
 	if err != nil {
 		return err
@@ -377,28 +382,18 @@ func validatePolicyForNodePolicy(current *sriovnetworkv1.SriovNetworkNodePolicy,
 
 func validatePfNames(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
 	for _, curPf := range current.Spec.NicSelector.PfNames {
-		curName, curRngSt, curRngEnd, err := sriovnetworkv1.ParsePFName(curPf)
+		curName, curRngSt, curRngEnd, err := sriovnetworkv1.ParseVfRange(curPf)
 		if err != nil {
 			return fmt.Errorf("invalid PF name: %s", curPf)
 		}
 		for _, prePf := range previous.Spec.NicSelector.PfNames {
-			// Not validate return err of ParsePFName for previous PF
+			// Not validate return err for previous PF
 			// since it should already be evaluated in previous run.
-			preName, preRngSt, preRngEnd, _ := sriovnetworkv1.ParsePFName(prePf)
+			preName, preRngSt, preRngEnd, _ := sriovnetworkv1.ParseVfRange(prePf)
 			if curName == preName {
-				// reject policy with externallyManage if there is a policy on the same PF without it
-				if current.Spec.ExternallyManaged != previous.Spec.ExternallyManaged {
-					return fmt.Errorf("externallyManage is inconsistent with existing policy %s", previous.GetName())
-				}
-
-				// reject policy with externallyManage if there is a policy on the same PF with switch dev
-				if current.Spec.ExternallyManaged && previous.Spec.EswitchMode == sriovnetworkv1.ESwithModeSwitchDev {
-					return fmt.Errorf("externallyManage overlap with switchdev mode in existing policy %s", previous.GetName())
-				}
-
-				// reject policy with externallyManage if there is a policy on the same PF with switch dev
-				if previous.Spec.ExternallyManaged && current.Spec.EswitchMode == sriovnetworkv1.ESwithModeSwitchDev {
-					return fmt.Errorf("switchdev overlap with externallyManage mode in existing policy %s", previous.GetName())
+				err = validateExternallyManage(current, previous)
+				if err != nil {
+					return err
 				}
 
 				// Check for overlapping ranges
@@ -413,6 +408,37 @@ func validatePfNames(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *s
 	return nil
 }
 
+func validateRootDevices(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
+	for _, curRootDevice := range current.Spec.NicSelector.RootDevices {
+		for _, preRootDevice := range previous.Spec.NicSelector.RootDevices {
+			// TODO: (SchSeba) implement range for root devices
+			if curRootDevice == preRootDevice {
+				return fmt.Errorf("root device %s is overlapped with existing policy %s", curRootDevice, previous.GetName())
+			}
+		}
+	}
+	return nil
+}
+
+func validateExternallyManage(current, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
+	// reject policy with externallyManage if there is a policy on the same PF without it
+	if current.Spec.ExternallyManaged != previous.Spec.ExternallyManaged {
+		return fmt.Errorf("externallyManage is inconsistent with existing policy %s", previous.GetName())
+	}
+
+	// reject policy with externallyManage if there is a policy on the same PF with switch dev
+	if current.Spec.ExternallyManaged && previous.Spec.EswitchMode == sriovnetworkv1.ESwithModeSwitchDev {
+		return fmt.Errorf("externallyManage overlap with switchdev mode in existing policy %s", previous.GetName())
+	}
+
+	// reject policy with externallyManage if there is a policy on the same PF with switch dev
+	if previous.Spec.ExternallyManaged && current.Spec.EswitchMode == sriovnetworkv1.ESwithModeSwitchDev {
+		return fmt.Errorf("switchdev overlap with externallyManage mode in existing policy %s", previous.GetName())
+	}
+
+	return nil
+}
+
 func validateExludeTopologyField(current *sriovnetworkv1.SriovNetworkNodePolicy, previous *sriovnetworkv1.SriovNetworkNodePolicy) error {
 	if current.Spec.ResourceName != previous.Spec.ResourceName {
 		return nil

pkg/webhook/validate_test.go

@@ -774,6 +774,31 @@ func TestValidatePoliciesWithSameExcludeTopologyForTheSameResource(t *testing.T)
 	g.Expect(err).NotTo(HaveOccurred())
 }
 
+func TestValidatePoliciesWithDifferentNumVfForTheSameResourceAndTheSameRootDevice(t *testing.T) {
+	current := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "currentPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName: "resourceX",
+			NumVfs:       10,
+			NicSelector:  SriovNetworkNicSelector{RootDevices: []string{"0000:86:00.1"}},
+		},
+	}
+
+	previous := &SriovNetworkNodePolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "previousPolicy"},
+		Spec: SriovNetworkNodePolicySpec{
+			ResourceName: "resourceX",
+			NumVfs:       5,
+			NicSelector:  SriovNetworkNicSelector{RootDevices: []string{"0000:86:00.1"}},
+		},
+	}
+
+	err := validatePolicyForNodePolicy(current, previous)
+
+	g := NewGomegaWithT(t)
+	g.Expect(err).To(MatchError("root device 0000:86:00.1 is overlapped with existing policy previousPolicy"))
+}
+
 func TestStaticValidateSriovNetworkNodePolicyWithValidVendorDevice(t *testing.T) {
 	policy := &SriovNetworkNodePolicy{
 		Spec: SriovNetworkNodePolicySpec{

test/e2e/sriovoperatornodepolicy_test.go

@@ -268,7 +268,7 @@ var _ = Describe("Operator", func() {
 							Expect(iface.VfGroups[0].DeviceType).To(Equal(policy.Spec.DeviceType))
 							Expect(iface.VfGroups[0].ResourceName).To(Equal(policy.Spec.ResourceName))
 
-							pfName, rngStart, rngEnd, err := sriovnetworkv1.ParsePFName(policy.Spec.NicSelector.PfNames[0])
+							pfName, rngStart, rngEnd, err := sriovnetworkv1.ParseVfRange(policy.Spec.NicSelector.PfNames[0])
 							Expect(err).NotTo(HaveOccurred())
 							rng := strconv.Itoa(rngStart) + "-" + strconv.Itoa(rngEnd)
 							Expect(iface.Name).To(Equal(pfName))