Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver #1

TheRealSibasishBehera · 2024-06-25T19:01:32Z

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver [WIP]

This PR introduces the KubeVirt Device Resource Allocation (DRA) Driver. The driver is responsible for managing the lifecycle of devices and ensuring the correct allocation of resources to pods. In KubeVirt, host device passthrough is facilitated through environment variables. If the correct environment variable is set, it can be converted to a libvirt domain XML. This allows the QEMU process to access host devices. This PR aims to provide the admin with a similar style of defining permitted host devices as in KubeVirtCR , with the help of DeviceClassParameters . The user can specify the exact resource required using PciClaimParameters. This driver currently supports management of PCI devices.

Key Components

Virt-DRA-Controller Deployment: The controller communicates with the kube-scheduler, making allocation decisions based on the overall cluster state and resource availability.
Kubelet Plugin DaemonSet: The plugin maintains and advertises the state of devices, communicating with the centralized controller to provide local state information and receive resource allocation instructions.
CRDs - NodeAllocationState, PciClaimParameters & DeviceClassParameters: These CRDs track resource state in a node, specify parameters for ResourceClaims, and provide parameters for ResourceClass respectively.
Manifests: These include manifests to deploy the controller and kubelet-plugin, as well as ServiceAccount and related objects to provide appropriate permissions to these components.

Future Scope

Enable direct communication between controller and plugin.
Setting up environment variables in the virt-launcher pod.
Add support for mdev and other device types.

More details regarding the problems of device plugin framework in kubevirt and usecase of DRA can be found here:
kubevirt/community#254 (comment)

Signed-off-by: TheRealSibasishBehera <fangedhamster3114@gmail.com>

alicefr · 2024-06-26T09:27:58Z

api/kubevirt.io/resource/pci/nas/v1alpha1/client/client.go

+}
+
+func (c *Client) Create(ctx context.Context) error {
+ crd := c.nas.DeepCopy()


Is this needed? You are overwriting in the next line

The DeepCopy is necessary to create a separate copy of the object that can be modified and sent to the API without affecting the original object (c.nas).

TheCreate(ctx, crd, metav1.CreateOptions{})call sends this copy to the Kubernetes API, which will modify the object

alicefr · 2024-06-26T09:28:21Z

api/kubevirt.io/resource/pci/nas/v1alpha1/client/client.go

+}
+
+func (c *Client) Update(ctx context.Context, spec *nascrd.NodeAllocationStateSpec) error {
+ crd := c.nas.DeepCopy()


Same comment as above

alicefr · 2024-06-26T09:31:18Z

cmd/virt-dra-controller/driver.go

+
+func (d driver) GetClaimParameters(ctx context.Context, claim *resourcev1.ResourceClaim, class *resourcev1.ResourceClass, classParameters interface{}) (interface{}, error) {
+ if claim.Spec.ParametersRef == nil {
+ return pcicrd.DefaultPciClaimParametersSpec(), nil


Why not returning nil here? The default PCI parameter looks wrong to me, like the device name shouldn't never be empty

alicefr · 2024-06-26T09:32:52Z

api/kubevirt.io/resource/pci/v1alpha1/pciclaimparameters.go

+type PciClaimParametersSpec struct {
+ //matches the ResourceName from class parameters
+ DeviceName string `json:"deviceName"`
+ Count int `json:"count"`


As we discussed, please remove the count since a single claim = single device

alicefr · 2024-06-26T09:37:46Z

cmd/kubelet-plugin/common.go

+ GetDevicePCIID(basepath string, pciAddress string) (string, error)
+}
+
+type DeviceUtilsHandler struct{}


Do we need to expose this type or can we simply use the interface? Usually, the type is private to the package and there is a costructor that returns the interface:

func NewDeviceHandler() DeviceHandler { return &deviceUtilsHandler{} }

yes exposing this is not required , I will add the NewDeviceHandler() function

alicefr · 2024-06-26T10:11:44Z

cmd/virt-dra-controller/pci.go

+}
+
+func (p *pcidriver) ValidateClaimParameters(claimParams *pcicrd.PciClaimParametersSpec) error {
+ if claimParams.DeviceName != "devices.kubevirt.io/nvme" {


I guess this can be any arbitary string, right?

No , DeviceName is supposed to be same as one of the supported resourceName in DeviceClassParameters .

I think we need to have somekind of mapping of what devices this driver supports . For now i have kept only devices.kubevirt.io/nvme

Currently VMI declaration [1] in kubevirt uses something similar as

hostDevices: - name: nvme deviceName: devices.kubevirt.io/nvme

I also think a name field needs to added in the PciClaimParameters , which will be arbitary defined by user

[1] https://kubevirt.io/user-guide/compute/host-devices/#nvme-pci-passthrough

alicefr · 2024-06-26T10:17:45Z

cmd/kubelet-plugin/pci_device.go

+ iommuGroup, err := Handler.GetDeviceIOMMUGroup(pciBasePath, info.Name())
+ if err != nil {
+ log.Printf("IOMMU group error: %v", err)
+ return nil


nit: indentation

alicefr · 2024-06-26T10:18:17Z

cmd/kubelet-plugin/pci_device.go

+ return nil
+ })
+ if err != nil {
+ log.Printf("Failed to discover host devices, error: %v", err)


nit: indentation

alicefr · 2024-06-26T10:19:26Z

cmd/kubelet-plugin/pci_device.go

+ pciID string
+}
+
+func DiscoverPermittedHostPCIDevices(supportedPCIDeviceMap map[string]string) (map[string][]*PCIDevice) {


Won't it make sense to return an error as well here?

I am not sure , because , in any case we need return a empty map[string][]*PCIDevice) , in case there are error while reading the PCIdevice , its logged right now

But it is in general best practise to let the caller handle errors

alicefr · 2024-06-26T10:22:09Z

cmd/kubelet-plugin/pci_device.go

+}
+
+// MockDiscoverPermittedHostPCIDevices returns predefined data for testing
+func MockDiscoverPermittedHostPCIDevices(supportedPCIDeviceMap map[string]string) (map[string][]*PCIDevice, map[string]string) {


The signature of this function is different from the function DiscoverPermittedHostPCIDevices, is it done on purpose?

DiscoverPermittedHostPCIDevices(supportedPCIDeviceMap map[string]string) (map[string][]*PCIDevice)

Yes its not right , we dont need the iommuToPCIMap for any allocation decision , I will update that

alicefr · 2024-06-26T11:00:53Z

cmd/virt-dra-controller/pci.go

+ switch device.Type() {
+ case nascrd.PciDeviceType:
+ available[device.Pci.UUID] = device.Pci
+ default:
+ // skip other devices
+ }


nit: for a single value you can use a if

makes sense , we can use switch case once this driver supports other kind of devices

alicefr · 2024-06-26T11:08:10Z

cmd/virt-dra-controller/pci.go

+ for _, device := range crd.Spec.AllocatableDevices {
+ switch device.Type() {
+ case nascrd.PciDeviceType:
+ available[device.Pci.UUID] = device.Pci
+ default:
+ // skip other devices
+ }
+ }
+
+ for _, allocation := range crd.Spec.AllocatedClaims {
+ switch allocation.Type() {
+ case nascrd.PciDeviceType:
+ for _, device := range allocation.Pci.Devices {
+ delete(available, device.UUID)
+ }
+ default:
+ }
+ }


I think this can be simplified as:

Suggested change

for _, device := range crd.Spec.AllocatableDevices {

switch device.Type() {

case nascrd.PciDeviceType:

available[device.Pci.UUID] = device.Pci

default:

// skip other devices

}

}

for _, allocation := range crd.Spec.AllocatedClaims {

switch allocation.Type() {

case nascrd.PciDeviceType:

for _, device := range allocation.Pci.Devices {

delete(available, device.UUID)

}

default:

}

}

for _, device := range crd.Spec.AllocatableDevices {

if device.Type() != nascrd.PciDeviceType {

continue

}

if _, exist := crd.Spec.AllocatedClaims[device.UUID]; exist {

continue

}

available[device.Pci.UUID] = device.Pci

}

alicefr · 2024-06-26T11:14:32Z

cmd/virt-dra-controller/pci.go

+ if _, exists := crd.Spec.AllocatedClaims[claimUID]; exists {
+ devices := crd.Spec.AllocatedClaims[claimUID].Pci.Devices


nit: use the value of the mapping

Suggested change

if _, exists := crd.Spec.AllocatedClaims[claimUID]; exists {

devices := crd.Spec.AllocatedClaims[claimUID].Pci.Devices

if v, exists := crd.Spec.AllocatedClaims[claimUID]; exists {

devices := v.Pci.Devices

initial commit

9272104

Signed-off-by: TheRealSibasishBehera <fangedhamster3114@gmail.com>

TheRealSibasishBehera changed the title ~~Introducing KubeVirt Device Resource Allocation (DRA) Driver~~ Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver Jun 25, 2024

alicefr reviewed Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver #1

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver #1

TheRealSibasishBehera commented Jun 25, 2024 •

edited

Loading

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 28, 2024

alicefr Jun 26, 2024

alicefr Jun 26, 2024

alicefr Jun 26, 2024

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 27, 2024 •

edited

Loading

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 27, 2024

alicefr Jun 26, 2024

alicefr Jun 26, 2024

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 27, 2024

alicefr Jul 24, 2024

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 27, 2024

alicefr Jun 26, 2024

TheRealSibasishBehera Jun 27, 2024 •

edited

Loading

alicefr Jun 26, 2024

alicefr Jun 26, 2024

		if _, exists := crd.Spec.AllocatedClaims[claimUID]; exists {
		devices := crd.Spec.AllocatedClaims[claimUID].Pci.Devices

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver #1

Are you sure you want to change the base?

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver #1

Conversation

TheRealSibasishBehera commented Jun 25, 2024 • edited Loading

Introducing KubeVirt Dynamic Resource Allocation (DRA) Driver [WIP]

Key Components

Future Scope

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealSibasishBehera Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealSibasishBehera Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealSibasishBehera commented Jun 25, 2024 •

edited

Loading

TheRealSibasishBehera Jun 27, 2024 •

edited

Loading

TheRealSibasishBehera Jun 27, 2024 •

edited

Loading