PCI DRA driver #1

TheRealSibasishBehera · 2024-08-12T22:39:50Z

What this PR does / why we need it:

This PR introduces the initial implementation of the KubeVirt DRA PCI Driver. The driver is designed to enhance the management of PCI passthrough devices in KubeVirt by leveraging the Dynamic Resource Allocation (DRA) framework in Kubernetes.

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

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note:

NONE

Makefile

alicefr · 2024-08-13T06:45:07Z

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

+// DeviceSelector allows one to match on a specific type of Device as part of the class.
+type DeviceSelector struct {
+	Type              string `json:"type"`
+	ResourceName      string `json:"resourceName"`


Is it the ResourceName required? I think this could be replaced by the claim name

permitted host devices in kubevirt CR uses the same name

configuration: permittedHostDevices: pciHostDevices: - pciVendorSelector: 8086:5845 resourceName: devices.kubevirt.io/nvme

For this the resource class would look like this

apiVersion: pci.resource.kubevirt.io/v1alpha1 kind: DeviceClassParameters metadata: name: pci-params spec: deviceSelector: - resourceName: "devices.kubevirt.io/nvme" pciVendorSelector: "1b36:0010" type: "pci"

So I think resourceName should be consistent

I think, we will need to modify kubevirt CR in order to list the allowed device classes/drivers instead of the resource names, but we can leave as it it for now.

alicefr · 2024-08-13T06:46:26Z

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

+
+// PciClaimParametersSpec is the spec for the PciClaimParameters CRD.
+type PciClaimParametersSpec struct {
+	DeviceName string `json:"deviceName"`


Right now, I don't think we have any specification. Could we use the Name in the ObjectMetadata?

I think it would be better to keep it separate from name in spec
kubernetes resource names has its own validation like its cannot contain slashes ('/') etc , for its own API , which would prevent us from doing something like

apiVersion: pci.resource.kubevirt.io/v1alpha1 kind: PciClaimParameters metadata: name: devices.kubevirt.io/nvme namespace: pci-nvme-test1

We would have map to some other object like config map or use label/annotation etc if we want to remove spec

I don't think we will need the slash, that name is typical for the device plugin. DRA is much more flexible. Anyway, as the comment above, right now we can leave as it is.

alicefr · 2024-08-13T06:49:56Z

cmd/kubelet-plugin/common.go

+	uid := uint32(107)
+	gid := uint32(107)


nit: small comment that this is the QEMU user

alicefr · 2024-08-13T06:55:50Z

cmd/set-nas-status/main.go

+	"os"
+	"strings"
+
+	"github.com/urfave/cli/v2"


In general, we use cobra for the cli, if you have time to change, otherwise, we can do it in a follow-up PR

makes sense . virt-dra-controller , kubelet-plugin and set-nas-status also uses this library . I will try to change it in this PR .

This will require changing the flag structure in pkg/flag + some API changes. It would be better to handle in a new PR

alicefr · 2024-08-13T06:57:30Z

cmd/virt-dra-controller/allocations.go

+	p.RLock()
+	for claimUID := range p.allocations {
+		for node, allocation := range p.allocations[claimUID] {
+			p.RUnlock()


Are this unlock and lock necessary inside the loop?

Yes removing them would work similarly

alicefr · 2024-08-13T06:58:22Z

cmd/virt-dra-controller/driver.go

+}
+
+func (d driver) allocate(ctx context.Context, claim *resourcev1.ResourceClaim, claimParameters interface{}, class *resourcev1.ResourceClass, classParameters interface{}, selectedNode string) (*resourcev1.AllocationResult, error) {
+


nit:empty line

alicefr · 2024-08-13T07:02:47Z

common.mk

@@ -0,0 +1,32 @@
+# Copyright 2022 The Kubernetes Authors.


Same here as above for the license

alicefr · 2024-08-13T07:03:03Z

demo/build-driver.sh

@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+
+# Copyright 2023 The Kubernetes Authors.


Please adjust the license header

alicefr · 2024-08-13T07:03:52Z

demo/scripts/common.sh

@@ -0,0 +1,54 @@
+#!/usr/bin/env bash
+
+# Copyright 2023 The Kubernetes Authors.


Same as above, please adjust the license header

alicefr · 2024-08-13T07:04:23Z

deployments/container/Dockerfile

@@ -0,0 +1,39 @@
+# Copyright 2023 The Kubernetes Authors.


Same as above for the license

alicefr · 2024-08-13T07:04:35Z

deployments/container/Makefile

@@ -0,0 +1,40 @@
+# Copyright 2023 The Kubernetes Authors.


Same as above for the license

alicefr · 2024-08-13T07:06:40Z

docker/Dockerfile.devel

@@ -0,0 +1,23 @@
+# Copyright (c) 2021, NVIDIA CORPORATION.  All rights reserved.


Please adjust the license header

alicefr · 2024-08-13T07:07:06Z

hack/tools/tools.go

@@ -0,0 +1,23 @@
+/*
+Copyright 2023 The Kubernetes Authors.


Please adjust the license header

alicefr · 2024-08-13T07:07:11Z

pkg/flags/kubeclient.go

@@ -0,0 +1,114 @@
+/*
+ * Copyright 2023 The Kubernetes Authors.


alicefr · 2024-08-13T07:07:20Z

pkg/flags/logging.go

@@ -0,0 +1,87 @@
+/*
+ * Copyright 2023 The Kubernetes Authors.


alicefr · 2024-08-13T07:07:26Z

pkg/flags/nodeallocationstate.go

@@ -0,0 +1,79 @@
+/*
+ * Copyright 2023 The Kubernetes Authors.


alicefr · 2024-08-13T07:08:45Z

@TheRealSibasishBehera I did a first pass, there are a couple of nits that you can ignore. The important parts imo are the licenses header, and then I think we can simplify further the API, but we can discuss this of Friday.

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

alicefr · 2024-08-19T10:06:22Z

Thannks! Let merge the first code then :)
/approve
/lgtm

alicefr · 2024-08-19T12:11:05Z

/approve

victortoso · 2024-08-19T12:39:52Z

/approve
/lgtm

alicefr · 2024-08-20T06:55:27Z

/approve

alicefr · 2024-08-20T10:24:48Z

/approve

victortoso · 2024-08-20T14:13:39Z

/approve

victortoso · 2024-08-20T14:14:16Z

/test tide

kubevirt-bot · 2024-08-20T14:14:19Z

@victortoso: No presubmit jobs available for kubevirt/dra-pci-driver@main

In response to this:

/test tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alicefr · 2024-08-21T06:53:50Z

/approve

alicefr · 2024-08-21T06:55:12Z

/override tide

kubevirt-bot · 2024-08-21T06:55:16Z

@alicefr: Overrode contexts on behalf of alicefr: tide

In response to this:

/override tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alicefr · 2024-08-21T06:58:23Z

/override tide

kubevirt-bot · 2024-08-21T06:58:26Z

@alicefr: Overrode contexts on behalf of alicefr: tide

In response to this:

/override tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alicefr · 2024-08-21T10:54:56Z

/approve

kubevirt-bot · 2024-08-21T10:55:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr, victortoso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alicefr,victortoso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot · 2024-08-21T10:55:23Z

@alicefr: No presubmit jobs available for kubevirt/dra-pci-driver@main

In response to this:

/test tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alicefr · 2024-08-21T10:55:42Z

/override tide

kubevirt-bot · 2024-08-21T10:55:46Z

@alicefr: Overrode contexts on behalf of alicefr: tide

In response to this:

/override tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Aug 12, 2024

alicefr reviewed Aug 13, 2024

View reviewed changes

hack/tools/tools.go Outdated

@@ -0,0 +1,23 @@

/*

Copyright 2023 The Kubernetes Authors.

Copy link

Member

alicefr Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust the license header

alicefr reviewed Aug 13, 2024

View reviewed changes

pkg/flags/kubeclient.go Outdated

@@ -0,0 +1,114 @@

/*

* Copyright 2023 The Kubernetes Authors.

Copy link

Member

alicefr Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

alicefr reviewed Aug 13, 2024

View reviewed changes

pkg/flags/logging.go Outdated

@@ -0,0 +1,87 @@

/*

* Copyright 2023 The Kubernetes Authors.

Copy link

Member

alicefr Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

alicefr reviewed Aug 13, 2024

View reviewed changes

pkg/flags/nodeallocationstate.go Outdated

@@ -0,0 +1,79 @@

/*

* Copyright 2023 The Kubernetes Authors.

Copy link

Member

alicefr Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

initial commit

d8bef78

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

TheRealSibasishBehera force-pushed the main branch from 0ba2302 to d8bef78 Compare August 19, 2024 09:33

kubevirt-bot assigned alicefr Aug 19, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2024

kubevirt-bot assigned victortoso Aug 19, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024

xpivarc mentioned this pull request Aug 21, 2024

Missing dra-pci-driver for approve and lgtm kubevirt/project-infra#3590

Merged

8 tasks

alicefr merged commit 145b62e into kubevirt:main Aug 21, 2024
1 of 2 checks passed

		}

		func (d driver) allocate(ctx context.Context, claim resourcev1.ResourceClaim, claimParameters interface{}, class resourcev1.ResourceClass, classParameters interface{}, selectedNode string) (*resourcev1.AllocationResult, error) {

		@@ -0,0 +1,42 @@
		#!/usr/bin/env bash

		# Copyright 2023 The Kubernetes Authors.

		@@ -0,0 +1,54 @@
		#!/usr/bin/env bash

		# Copyright 2023 The Kubernetes Authors.

		@@ -0,0 +1,23 @@
		# Copyright (c) 2021, NVIDIA CORPORATION. All rights reserved.

PCI DRA driver #1

PCI DRA driver #1

Conversation

TheRealSibasishBehera commented Aug 12, 2024

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

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

Choose a reason for hiding this comment

alicefr commented Aug 13, 2024

alicefr commented Aug 19, 2024

alicefr commented Aug 19, 2024

victortoso commented Aug 19, 2024

alicefr commented Aug 20, 2024

alicefr commented Aug 20, 2024

victortoso commented Aug 20, 2024

victortoso commented Aug 20, 2024

kubevirt-bot commented Aug 20, 2024

alicefr commented Aug 21, 2024

alicefr commented Aug 21, 2024

kubevirt-bot commented Aug 21, 2024

alicefr commented Aug 21, 2024

kubevirt-bot commented Aug 21, 2024

alicefr commented Aug 21, 2024

kubevirt-bot commented Aug 21, 2024

kubevirt-bot commented Aug 21, 2024

alicefr commented Aug 21, 2024

kubevirt-bot commented Aug 21, 2024