Merge pull request #924 from SchSeba/merge-bot-master

Merge https://github.com/k8snetworkplumbingwg/sriov-network-operator:master into master

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
 }

bundle/manifests/sriov-network-operator.clusterserviceversion.yaml

@@ -100,7 +100,7 @@ metadata:
     categories: Networking
     certified: "false"
     containerImage: quay.io/openshift/origin-sriov-network-operator:4.16
-    createdAt: "2024-04-18T09:27:02Z"
+    createdAt: "2024-04-18T13:41:29Z"
     description: An operator for configuring SR-IOV components and initializing SRIOV
       network devices in Openshift cluster.
     features.operators.openshift.io/cnf: "false"

cmd/sriov-network-config-daemon/service.go

@@ -182,7 +182,7 @@ func callPlugin(setupLog logr.Logger, phase string, conf *systemd.SriovConfig, h
 
 	nodeState, err := getNetworkNodeState(setupLog, conf, hostHelpers)
 	if err != nil {
-		return nil
+		return err
 	}
 	_, _, err = configPlugin.OnNodeStateChange(nodeState)
 	if err != nil {

hack/env.sh

@@ -22,10 +22,10 @@ if [ -z $SKIP_VAR_SET ]; then
         OPERATOR_IMAGE_DIGEST=$(skopeo inspect docker://quay.io/openshift/origin-sriov-network-operator | jq --raw-output '.Digest')
         export SRIOV_NETWORK_OPERATOR_IMAGE=${SRIOV_NETWORK_OPERATOR_IMAGE:-quay.io/openshift/origin-sriov-network-operator@${OPERATOR_IMAGE_DIGEST}}
 else
+        # ensure that OVS_CNI_IMAGE is set, empty string is a valid value
+        OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-}
         [ -z $SRIOV_CNI_IMAGE ] && echo "SRIOV_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
         [ -z $SRIOV_INFINIBAND_CNI_IMAGE ] && echo "SRIOV_INFINIBAND_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
-        # check that OVS_CNI_IMAGE is set to any value, empty string is a valid value
-        [ -z ${OVS_CNI_IMAGE+set} ] && echo "OVS_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
         [ -z $SRIOV_DEVICE_PLUGIN_IMAGE ] && echo "SRIOV_DEVICE_PLUGIN_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
         [ -z $NETWORK_RESOURCES_INJECTOR_IMAGE ] && echo "NETWORK_RESOURCES_INJECTOR_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
         [ -z $SRIOV_NETWORK_CONFIG_DAEMON_IMAGE ] && echo "SRIOV_NETWORK_CONFIG_DAEMON_IMAGE is empty but SKIP_VAR_SET is set" && exit 1

manifests/stable/sriov-network-operator.clusterserviceversion.yaml

@@ -100,7 +100,7 @@ metadata:
     categories: Networking
     certified: "false"
     containerImage: quay.io/openshift/origin-sriov-network-operator:4.16
-    createdAt: "2024-04-18T09:27:02Z"
+    createdAt: "2024-04-18T13:41:29Z"
     description: An operator for configuring SR-IOV components and initializing SRIOV
       network devices in Openshift cluster.
     features.operators.openshift.io/cnf: "false"

pkg/daemon/daemon.go

@@ -484,10 +484,14 @@ func (dn *Daemon) nodeStateSyncHandler() error {
 	if reqDrain || !utils.ObjectHasAnnotation(dn.desiredNodeState,
 		consts.NodeStateDrainAnnotationCurrent,
 		consts.DrainIdle) {
-		if err := dn.handleDrain(reqReboot); err != nil {
+		drainInProcess, err := dn.handleDrain(reqReboot)
+		if err != nil {
 			log.Log.Error(err, "failed to handle drain")
 			return err
 		}
+		if drainInProcess {
+			return nil
+		}
 	}
 
 	if !reqReboot && !vars.UsingSystemdMode {
@@ -560,54 +564,61 @@ func (dn *Daemon) nodeStateSyncHandler() error {
 	return nil
 }
 
-func (dn *Daemon) handleDrain(reqReboot bool) error {
+// handleDrain: adds the right annotation to the node and nodeState object
+// returns true if we need to finish the reconcile loop and wait for a new object
+func (dn *Daemon) handleDrain(reqReboot bool) (bool, error) {
+	// done with the drain we can continue with the configuration
 	if utils.ObjectHasAnnotation(dn.desiredNodeState, consts.NodeStateDrainAnnotationCurrent, consts.DrainComplete) {
 		log.Log.Info("handleDrain(): the node complete the draining")
-		return nil
+		return false, nil
 	}
 
+	// the operator is still draining the node so we reconcile
 	if utils.ObjectHasAnnotation(dn.desiredNodeState, consts.NodeStateDrainAnnotationCurrent, consts.Draining) {
 		log.Log.Info("handleDrain(): the node is still draining")
-		return nil
+		return true, nil
 	}
 
+	// drain is disabled we continue with the configuration
 	if dn.disableDrain {
 		log.Log.Info("handleDrain(): drain is disabled in sriovOperatorConfig")
-		return nil
+		return false, nil
 	}
 
 	if reqReboot {
 		log.Log.Info("handleDrain(): apply 'Reboot_Required' annotation for node")
 		err := utils.AnnotateNode(context.Background(), vars.NodeName, consts.NodeDrainAnnotation, consts.RebootRequired, dn.client)
 		if err != nil {
 			log.Log.Error(err, "applyDrainRequired(): Failed to annotate node")
-			return err
+			return false, err
 		}
 
 		log.Log.Info("handleDrain(): apply 'Reboot_Required' annotation for nodeState")
 		if err := utils.AnnotateObject(context.Background(), dn.desiredNodeState,
 			consts.NodeStateDrainAnnotation,
 			consts.RebootRequired, dn.client); err != nil {
-			return err
+			return false, err
 		}
 
-		return nil
+		// the node was annotated we need to wait for the operator to finish the drain
+		return true, nil
 	}
 	log.Log.Info("handleDrain(): apply 'Drain_Required' annotation for node")
 	err := utils.AnnotateNode(context.Background(), vars.NodeName, consts.NodeDrainAnnotation, consts.DrainRequired, dn.client)
 	if err != nil {
 		log.Log.Error(err, "handleDrain(): Failed to annotate node")
-		return err
+		return false, err
 	}
 
 	log.Log.Info("handleDrain(): apply 'Drain_Required' annotation for nodeState")
 	if err := utils.AnnotateObject(context.Background(), dn.desiredNodeState,
 		consts.NodeStateDrainAnnotation,
 		consts.DrainRequired, dn.client); err != nil {
-		return err
+		return false, err
 	}
 
-	return nil
+	// the node was annotated we need to wait for the operator to finish the drain
+	return true, nil
 }
 
 func (dn *Daemon) restartDevicePluginPod() error {

pkg/daemon/daemon_test.go

@@ -194,8 +194,9 @@ var _ = Describe("Config Daemon", func() {
 
 			nodeState := &sriovnetworkv1.SriovNetworkNodeState{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:       "test-node",
-					Generation: 123,
+					Name:        "test-node",
+					Generation:  123,
+					Annotations: map[string]string{consts.NodeStateDrainAnnotationCurrent: consts.DrainIdle},
 				},
 				Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{},
 				Status: sriovnetworkv1.SriovNetworkNodeStateStatus{
@@ -253,8 +254,9 @@ var _ = Describe("Config Daemon", func() {
 
 			nodeState1 := &sriovnetworkv1.SriovNetworkNodeState{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:       "test-node",
-					Generation: 123,
+					Name:        "test-node",
+					Generation:  123,
+					Annotations: map[string]string{consts.NodeStateDrainAnnotationCurrent: consts.DrainIdle},
 				},
 			}
 			Expect(
@@ -263,8 +265,9 @@ var _ = Describe("Config Daemon", func() {
 
 			nodeState2 := &sriovnetworkv1.SriovNetworkNodeState{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:       "test-node",
-					Generation: 777,
+					Name:        "test-node",
+					Generation:  777,
+					Annotations: map[string]string{consts.NodeStateDrainAnnotationCurrent: consts.DrainIdle},
 				},
 			}
 			Expect(

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

@@ -777,6 +777,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/conformance/tests/test_sriov_operator.go

@@ -1015,7 +1015,7 @@ var _ = Describe("[sriov] operator", func() {
 
 				testPod := createTestPod(node, []string{sriovNetworkName})
 
-				recentMultusLogs := getMultusPodLogs(testPod.Spec.NodeName, testPod.ObjectMeta.CreationTimestamp.Time)
+				recentMultusLogs := getMultusPodLogs(testPod.Spec.NodeName, testPod.Status.StartTime.Time)
 
 				Expect(recentMultusLogs).To(
 					ContainElement(

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))