Merge pull request #13321 from PiotrProkop/fix-vfio-init

runtime: Hotplug VFIO network devices before any container starts

src/runtime/virtcontainers/kata_agent.go

@@ -861,7 +861,7 @@ func (k *kataAgent) startSandbox(ctx context.Context, sandbox *Sandbox) error {
 
 	if sandbox.config.HypervisorType != RemoteHypervisor {
 		// Setup network interfaces and routes
-		err = k.setupNetworks(ctx, sandbox, nil)
+		err = k.setupNetworks(ctx, sandbox)
 		if err != nil {
 			return err
 		}
@@ -1473,7 +1473,7 @@ func (k *kataAgent) rollbackFailingContainerCreation(ctx context.Context, c *Con
 	}
 }
 
-func (k *kataAgent) setupNetworks(ctx context.Context, sandbox *Sandbox, c *Container) error {
+func (k *kataAgent) setupNetworks(ctx context.Context, sandbox *Sandbox) error {
 	if sandbox.network.NetworkID() == "" {
 		return nil
 	}
@@ -1503,64 +1503,46 @@ func (k *kataAgent) setupNetworks(ctx context.Context, sandbox *Sandbox, c *Cont
 		}
 	}
 
-	var err error
-	var endpoints []Endpoint
-	if c == nil || c.id == sandbox.id {
-		// TODO: VFIO network device has not been hotplugged when creating the Sandbox,
-		// so need to skip VFIO endpoint here.
-		// After KEP #4113(https://github.com/kubernetes/enhancements/pull/4113)
-		// is implemented, the VFIO network devices will be attached before container
-		// creation, so no need to skip them here anymore.
-		for _, ep := range sandbox.network.Endpoints() {
-			if ep.Type() != VfioEndpointType {
-				// For cold-plugged SR-IOV VFs that appear as PhysicalEndpoints,
-				// the guest PCI path is known after resolveColdPlugVFIOGuestPciPaths
-				// has run (during createContainers). Look it up and stamp it on the
-				// endpoint so that generateVCNetworkStructures emits a non-empty
-				// devicePath in the agent Interface proto. Without this the agent
-				// receives devicePath="" and falls back to a by-MAC link lookup,
-				// which fails when the VF firmware MAC differs from the OVN MAC.
-				if ep.Type() == PhysicalEndpointType && ep.PciPath().IsNil() {
-					if pe, ok := ep.(*PhysicalEndpoint); ok && pe.BDF != "" {
-						guestPath := sandbox.GetVfioDeviceGuestPciPath(pe.BDF)
-						if !guestPath.IsNil() {
-							ep.SetPciPath(guestPath)
-							k.Logger().WithFields(logrus.Fields{
-								"endpoint-name":  ep.Name(),
-								"host-bdf":       pe.BDF,
-								"guest-pci-path": guestPath.String(),
-							}).Info("setupNetworks: filled guest PCI path for PhysicalEndpoint cold-plug")
-						}
-					}
-				}
-				endpoints = append(endpoints, ep)
-			}
-		}
-	} else if !sandbox.hotplugNetworkConfigApplied {
-		// Apply VFIO network devices' configuration after they are hot-plugged.
-		for _, ep := range sandbox.network.Endpoints() {
-			if ep.Type() == VfioEndpointType {
-				hostBDF := ep.(*VfioEndpoint).HostBDF
-				pciPath := sandbox.GetVfioDeviceGuestPciPath(hostBDF)
-				if pciPath.IsNil() {
-					return fmt.Errorf("PCI path for VFIO interface '%s' not found", ep.Name())
-				}
-				ep.SetPciPath(pciPath)
-				endpoints = append(endpoints, ep)
-			}
-		}
-
-		defer func() {
-			if err == nil {
-				sandbox.hotplugNetworkConfigApplied = true
-			}
-		}()
-	}
-
+	endpoints := sandbox.network.Endpoints()
 	if len(endpoints) == 0 {
 		return nil
 	}
 
+	// VFIO network devices (DAN) are not attached by the generic endpoint
+	// attach path. Hotplug them now, before any container is created, so
+	// that init containers that do not reference the device in their spec
+	// get working networking as well.
+	for _, ep := range endpoints {
+		if ep.Type() == VfioEndpointType {
+			if err := sandbox.hotplugVfioNetworkDevice(ctx, ep.(*VfioEndpoint)); err != nil {
+				return err
+			}
+		}
+	}
+
+	// For cold-plugged SR-IOV VFs that appear as PhysicalEndpoints, the
+	// guest PCI path is known after the ResolveColdPlugVFIOGuestPciPaths
+	// call above. Look it up and stamp it on the endpoint so that
+	// generateVCNetworkStructures emits a non-empty devicePath in the agent
+	// Interface proto. Without this the agent receives devicePath="" and
+	// falls back to a by-MAC link lookup, which fails when the VF firmware
+	// MAC differs from the OVN MAC.
+	for _, ep := range endpoints {
+		if ep.Type() == PhysicalEndpointType && ep.PciPath().IsNil() {
+			if pe, ok := ep.(*PhysicalEndpoint); ok && pe.BDF != "" {
+				guestPath := sandbox.GetVfioDeviceGuestPciPath(pe.BDF)
+				if !guestPath.IsNil() {
+					ep.SetPciPath(guestPath)
+					k.Logger().WithFields(logrus.Fields{
+						"endpoint-name":  ep.Name(),
+						"host-bdf":       pe.BDF,
+						"guest-pci-path": guestPath.String(),
+					}).Info("setupNetworks: filled guest PCI path for PhysicalEndpoint cold-plug")
+				}
+			}
+		}
+	}
+
 	interfaces, routes, neighs, err := generateVCNetworkStructures(ctx, endpoints)
 	if err != nil {
 		return err
@@ -1745,10 +1727,6 @@ func (k *kataAgent) createContainer(ctx context.Context, sandbox *Sandbox, c *Co
 		return nil, err
 	}
 
-	if err = k.setupNetworks(ctx, sandbox, c); err != nil {
-		return nil, err
-	}
-
 	return buildProcessFromExecID(req.ExecId)
 }
 

src/runtime/virtcontainers/sandbox.go

@@ -50,6 +50,7 @@ import (
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils"
 
+	"golang.org/x/sys/unix"
 	"google.golang.org/grpc/codes"
 	grpcStatus "google.golang.org/grpc/status"
 )
@@ -264,11 +265,6 @@ type Sandbox struct {
 	seccompSupported  bool
 	disableVMShutdown bool
 	isVCPUsPinningOn  bool
-
-	// hotplugNetworkConfigApplied prevents network config API being called
-	// multiple times for hot-plugged network device when Sandbox has multiple
-	// containers.
-	hotplugNetworkConfigApplied bool
 }
 
 // ID returns the sandbox identifier string.
@@ -2384,6 +2380,65 @@ func (s *Sandbox) GetVfioDeviceGuestPciPath(hostBDF string) types.PciPath {
 	return types.PciPath{}
 }
 
+// hotplugVfioNetworkDevice hotplugs the VFIO device backing a DAN network
+// endpoint into the VM and records its guest PCI path on the endpoint.
+//
+// The device is attached at sandbox scope so that it is available before any
+// container is created -- in particular before init containers, which do not
+// reference the device in their spec. When a workload container later
+// references the same VFIO group through a device plugin, the device manager
+// finds the existing device by major:minor and only bumps its reference
+// counts, so the device is neither plugged twice nor unplugged when that
+// container exits.
+func (s *Sandbox) hotplugVfioNetworkDevice(ctx context.Context, ep *VfioEndpoint) error {
+	if !ep.PciPath().IsNil() {
+		// The device is already attached and configured.
+		return nil
+	}
+
+	if s.config.HypervisorConfig.HotPlugVFIO == config.NoPort {
+		return fmt.Errorf("cannot attach VFIO network interface %q (BDF %s): hot_plug_vfio port is not configured", ep.Name(), ep.HostBDF)
+	}
+
+	devPath, err := drivers.GetVFIODevPath(ep.HostBDF)
+	if err != nil {
+		return fmt.Errorf("failed to resolve VFIO device path for network interface %q (BDF %s): %v", ep.Name(), ep.HostBDF, err)
+	}
+
+	var stat unix.Stat_t
+	if err := unix.Stat(devPath, &stat); err != nil {
+		return fmt.Errorf("stat %q failed for network interface %q: %v", devPath, ep.Name(), err)
+	}
+
+	devInfo := config.DeviceInfo{
+		HostPath:      devPath,
+		ContainerPath: devPath,
+		DevType:       "c",
+		Major:         int64(unix.Major(uint64(stat.Rdev))),
+		Minor:         int64(unix.Minor(uint64(stat.Rdev))),
+		Port:          s.config.HypervisorConfig.HotPlugVFIO,
+	}
+
+	if _, err := s.AddDevice(ctx, devInfo); err != nil {
+		return fmt.Errorf("failed to hotplug VFIO device %q for network interface %q: %v", devPath, ep.Name(), err)
+	}
+
+	pciPath := s.GetVfioDeviceGuestPciPath(ep.HostBDF)
+	if pciPath.IsNil() {
+		return fmt.Errorf("PCI path for VFIO interface %q (BDF %s) not found after hotplug", ep.Name(), ep.HostBDF)
+	}
+	ep.SetPciPath(pciPath)
+
+	s.Logger().WithFields(logrus.Fields{
+		"interface": ep.Name(),
+		"host-bdf":  ep.HostBDF,
+		"device":    devPath,
+		"pci-path":  pciPath.String(),
+	}).Info("VFIO network device hotplugged")
+
+	return nil
+}
+
 // updateResources will:
 // - calculate the resources required for the virtual machine, and adjust the virtual machine
 // sizing accordingly. For a given sandbox, it will calculate the number of vCPUs required based

src/runtime/virtcontainers/sandbox_test.go

@@ -1719,3 +1719,80 @@ func TestCheckVCPUsPinningNUMABadHostCPUs(t *testing.T) {
 	assert.Error(err)
 	assert.Contains(err.Error(), "failed to parse HostCPUs")
 }
+
+func TestHotplugVfioNetworkDeviceAlreadyAttached(t *testing.T) {
+	assert := assert.New(t)
+
+	pciPath, err := types.PciPathFromString("02")
+	assert.NoError(err)
+
+	s := &Sandbox{
+		config: &SandboxConfig{
+			HypervisorConfig: HypervisorConfig{
+				HotPlugVFIO: config.NoPort,
+			},
+		},
+	}
+
+	// An endpoint whose guest PCI path is already known has been hotplugged
+	// before: the call must be a no-op, even when no hotplug port is
+	// configured.
+	ep := &VfioEndpoint{
+		EndpointType: VfioEndpointType,
+		HostBDF:      "0000:ff:1f.7",
+		PCIPath:      pciPath,
+		Iface:        NetworkInterface{Name: "eth1"},
+	}
+
+	assert.NoError(s.hotplugVfioNetworkDevice(context.Background(), ep))
+	assert.Equal(pciPath, ep.PciPath())
+}
+
+func TestHotplugVfioNetworkDeviceNoPort(t *testing.T) {
+	assert := assert.New(t)
+
+	s := &Sandbox{
+		config: &SandboxConfig{
+			HypervisorConfig: HypervisorConfig{
+				HotPlugVFIO: config.NoPort,
+			},
+		},
+	}
+
+	ep := &VfioEndpoint{
+		EndpointType: VfioEndpointType,
+		HostBDF:      "0000:ff:1f.7",
+		Iface:        NetworkInterface{Name: "eth1"},
+	}
+
+	err := s.hotplugVfioNetworkDevice(context.Background(), ep)
+	assert.Error(err)
+	assert.Contains(err.Error(), "hot_plug_vfio")
+	assert.True(ep.PciPath().IsNil())
+}
+
+func TestHotplugVfioNetworkDevicePortConfigured(t *testing.T) {
+	assert := assert.New(t)
+
+	s := &Sandbox{
+		config: &SandboxConfig{
+			HypervisorConfig: HypervisorConfig{
+				HotPlugVFIO: config.RootPort,
+			},
+		},
+	}
+
+	// With a hotplug port configured the guards are passed and the VFIO
+	// device path resolution is attempted. The malformed BDF cannot match
+	// any sysfs PCI device directory, so the resolution fails
+	// deterministically on every host.
+	ep := &VfioEndpoint{
+		EndpointType: VfioEndpointType,
+		HostBDF:      "not-a-bdf",
+		Iface:        NetworkInterface{Name: "eth1"},
+	}
+
+	err := s.hotplugVfioNetworkDevice(context.Background(), ep)
+	assert.Error(err)
+	assert.Contains(err.Error(), "failed to resolve VFIO device path")
+}