CDI implementation #489

e0ne · 2023-06-04T17:16:06Z

No description provided.

cmd/sriovdp/manager.go

coveralls · 2023-06-20T09:00:15Z

Pull Request Test Coverage Report for Build 5313388064

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
8 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.9%) to 76.4%

Files with Coverage Reduction	New Missed Lines	%
pkg/utils/testing.go	8	56.6%

Totals
Change from base Build 5055671149:	-1.9%
Covered Lines:	573
Relevant Lines:	750

💛 - Coveralls

cmd/sriovdp/manager.go

pkg/resources/server.go

adrianchiris

please note that device plugin daemonset would need to mount cdi dir
(/var/run/cdi) IIRC

cmd/sriovdp/main.go

cmd/sriovdp/manager.go

moshe010 · 2023-06-29T07:09:33Z

cmd/sriovdp/manager.go

 		rPool, err := rm.rFactory.GetResourcePool(rc, filteredDevices)
 		if err != nil {
 			glog.Errorf("initServers(): error creating ResourcePool with config %+v: %q", rc, err)
 			return err
 		}
+
+		if rm.useCdi {
+			err = cdi.CreateCDISpec(rm.resourcePrefix, filteredDevices, rPool)


I see you regenerated it because the logic is in device plugin and you don't want to duplicate it

moshe010 · 2023-06-29T07:11:02Z

deployments/cdi/sriovdp-daemonset.yaml

+              name: default-cdi
+            - mountPath: /var/run/cdi
+              name: dynamic-cdi
+            - mountPath: /host//etc/pcidp/


remove extra /

moshe010 · 2023-06-29T07:14:21Z

pkg/cdi/cdi.go

+func CreateCDISpec(resourcePrefix string, filteredDevices []types.HostDevice, rPool types.ResourcePool) error {
+	cdiDevices := make([]cdiSpecs.Device, 0)
+	cdiSpec := cdiSpecs.Spec{
+		Version: "0.5.0",


please use constant for version in https://github.com/container-orchestrated-devices/container-device-interface/blob/v0.5.0/specs-go/config.go#L6

moshe010 · 2023-06-29T07:15:05Z

pkg/cdi/cdi.go

+			deviceNode := cdiSpecs.DeviceNode{
+				Path:        spec.ContainerPath,
+				HostPath:    spec.HostPath,
+				Permissions: "rwm",


did you check if the m in Permissions is needed?

it doesn't needed for VFs. I'll check with SFs and create a separate PR to fix it over the whole project

Did you check with and without RDMA CM?

yes, I checked both for VFs

cmd/sriovdp/manager.go

deployments/cdi/systemd/cdi-spec.service

adrianchiris · 2023-09-11T13:36:58Z

deployments/cdi/README.md

+```yaml
+    - mountPath: /etc/cdi/
+      name: default-cdi
+    - mountPath: /var/run/cdi


we need just /var/run/cdi imo we dont deal with statically configured files right ?

we should align daemonsets as well

adrianchiris · 2023-09-11T13:38:48Z

go.mod

@@ -4,6 +4,7 @@ go 1.20

 require (
 	github.com/Mellanox/rdmamap v1.1.0
+	github.com/container-orchestrated-devices/container-device-interface v0.5.4


reminder to check if we prefer to:

bump this to v0.6.0 and require containerd 1.7.5 or cri-o 1.28 or keep as is.

According to https://github.com/containerd/containerd/blob/v1.7.5/go.mod#L10, we need to keep v0.5.4 now

adrianchiris · 2023-09-11T13:47:33Z

pkg/resources/server.go

@@ -324,6 +365,9 @@ func (rs *resourceServer) cleanUp() error {
 	if err := rs.resourcePool.CleanDeviceInfoFile(rs.resourceNamePrefix); err != nil {
 		errors = append(errors, err.Error())
 	}
+	if err := rs.cdi.CleanupSpecs(); err != nil {


i think we should:

cleanup spec files which are related only to this resource server her

in the resource manager we should cleanup any remaining cdi spec files before starting servers just in case on un-gracefull exit of previous device plugin instance.

thoughts ?

IMO, it's better to move this logic to resource manager only: it will cleanup all spec files before resorce servers start, so no raise condidion in this case and all outdated specs will be removed. With such behaviour we don't need to filter for orphaned spec files if config changed during device plugin restart

adrianchiris · 2023-09-11T13:49:56Z

pkg/cdi/cdi.go

+// Impl implements CDI interface
+type Impl struct {
+}
+


nit : define a New() method

func New() CDI { return &impl{} }

and use it in resource server

also consider making Impl struct private

adrianchiris · 2023-09-11T13:51:13Z

pkg/cdi/cdi.go

+
+// CreateCDISpecForPool creates CDI spec file with specified devices
+func (c *Impl) CreateCDISpecForPool(resourcePrefix string, rPool types.ResourcePool) error {
+	err := c.CleanupSpecs()


this will delete spec files of other resource servers. see other comments related to where/how we should clean up.

please loot at #489 (comment)

adrianchiris · 2023-09-11T13:52:44Z

pkg/cdi/cdi.go

+func (c *Impl) CleanupSpecs() error {
+	for _, dir := range cdi.GetRegistry().GetSpecDirectories() {
+		specs, err := filepath.Glob(filepath.Join(dir, cdiSpecPrefix+"*"))
+		if err != nil {


will this one fail if dir doesnt exist ?

if yes, then for such error we need to skip, else disregard my comment.

Glob returns the names of all files matching pattern or nil if there is no matching file. The syntax of patterns is the same as in Match. The pattern may describe hierarchical names such as /usr/*/bin/ed (assuming the Separator is '/').
Glob ignores file system errors such as I/O errors reading directories. The only possible returned error is ErrBadPattern, when pattern is malformed.

https://pkg.go.dev/path/filepath#Glob

adrianchiris · 2023-09-11T13:53:24Z

pkg/cdi/cdi_test.go

+			annoKey := "cdi.k8s.io/example.com_net"
+			annoVal := "example.com/net=0000:00:00.2"
+			Expect(annotations[annoKey]).To(Equal(annoVal))
+		})


need to add test for cleanup method as well

adrianchiris

final comments on my side, once addressed im LGTM.

SchSeba

nice work!
I left some comments

SchSeba · 2023-09-20T16:45:45Z

cmd/sriovdp/main.go

 func flagInit(cp *cliParams) {
 	flag.StringVar(&cp.configFile, "config-file", defaultConfig,
 		"JSON device pool config file location")
 	flag.StringVar(&cp.resourcePrefix, "resource-prefix", "intel.com",
-		"resource name prefix used for K8s extended resource")
+		"resource name prefix used for K8s extended re"+


nit: why a new line here?

SchSeba · 2023-09-20T16:47:41Z

cmd/sriovdp/main.go

@@ -43,7 +46,7 @@ func main() {

 	glog.Infof("resource manager reading configs")
 	if err := rm.readConfig(); err != nil {
-		glog.Errorf("error getting resources from file %v", err)
+		glog.Error("error getting resources from file", err)


I think here we should have the errorf

SchSeba · 2023-09-20T16:50:03Z

cmd/sriovdp/manager.go

@@ -101,12 +109,16 @@ func (rm *resourceManager) readConfig() error {
 }

 func (rm *resourceManager) initServers() error {
+	err := rm.cleanupCDISpecs()
+	if err != nil {
+		glog.Infof("Unable to delete CDI specs: %s", err)


I think it should be error here if we return an error

SchSeba · 2023-09-20T16:51:06Z

cmd/sriovdp/manager.go

+func (rm *resourceManager) cleanupCDISpecs() error {
+	if rm.cliParams.useCdi {
+		if err := rm.cdi.CleanupSpecs(); err != nil {
+			return fmt.Errorf("unable to delete CDI specs: %s", err)


I think it should be %v here

SchSeba · 2023-09-20T16:56:18Z

pkg/accelerator/accelResourcePool.go

@@ -23,6 +23,10 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/types"
 )

+const (
+	accelPoolType = "net-accel"


why we add net-?

We do it for the consistency across all device types

SchSeba · 2023-09-21T17:12:08Z

pkg/cdi/cdi.go

+func (c *impl) CreateCDISpecForPool(resourcePrefix string, rPool types.ResourcePool) error {
+	err := c.CleanupSpecs()
+	if err != nil {
+		glog.Error("can not cleanup old spec files", err)


please add CreateCDISpecForPool(): so we can be consistent in the logs or remove it from the log below

SchSeba · 2023-09-21T17:16:25Z

pkg/cdi/cdi_test.go

+			annoVal := "example.com/net=0000:00:00.1"
+			Expect(annotations[annoKey]).To(Equal(annoVal))
+		})
+		It("should not fail on non-existing device", func() {


are you sure the test is right?

You're righ. I'll delete this test

SchSeba · 2023-09-21T17:19:15Z

pkg/resources/server.go

+			containerResp.Annotations, err = rs.cdi.CreateContainerAnnotations(
+				container.DevicesIDs, rs.resourceNamePrefix, rs.resourcePool.GetCDIName())
+			if err != nil {
+				return nil, fmt.Errorf("cant create container annotation: %s", err)


nit: cant -> can't

SchSeba · 2023-09-21T17:21:14Z

pkg/resources/server.go

@@ -147,8 +162,12 @@ func (rs *resourceServer) ListAndWatch(empty *pluginapi.Empty, stream pluginapi.
 		devs = append(devs, dev)
 	}
 	resp.Devices = devs
+	err := rs.updateCDISpec()
+	if err != nil {
+		glog.Error("cannot update CDI specs", err)


should be errorf with %v

SchSeba · 2023-09-21T17:21:50Z

pkg/resources/server.go

@@ -147,8 +162,12 @@ func (rs *resourceServer) ListAndWatch(empty *pluginapi.Empty, stream pluginapi.
 		devs = append(devs, dev)
 	}
 	resp.Devices = devs
+	err := rs.updateCDISpec()
+	if err != nil {


nit: you can have this in the same line

thanks, done!

zeeke · 2023-09-22T12:48:04Z

pkg/cdi/cdi.go

+func (c *impl) CreateCDISpecForPool(resourcePrefix string, rPool types.ResourcePool) error {
+	err := c.CleanupSpecs()
+	if err != nil {
+		glog.Error("CreateCDISpecForPool(): can not cleanup old spec files", err)


Shouldn't this be glog.Errorf("....%v", err)?

zeeke · 2023-09-22T12:48:28Z

pkg/cdi/cdi.go

+	annotations := make(map[string]string, 0)
+	annoKey, err := cdi.AnnotationKey(resourcePrefix, resourceKind)
+	if err != nil {
+		glog.Error("CreateContainerAnnotations(): can't create container annotation", err)


Shouldn't this be glog.Errorf("....%v", err)?
Same here and on line 106

zeeke

Left a few minor comments about error management.
Beside that, LGTM

e0ne · 2023-09-22T13:24:04Z

Left a few minor comments about error management. Beside that, LGTM

thanks, @zeeke

SchSeba

just some last small nits

can you please also add a section about CDI in the main document that point to the CDI readme?

cmd/sriovdp/manager.go

SchSeba · 2023-09-27T07:13:54Z

pkg/accelerator/accelResourcePool.go

@@ -23,6 +23,10 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/types"
 )

+const (
+	accelPoolType = "net-accel"


should this be just accel as the accelerators are not only network acceleration

for now we decide to leave it with the net-

pkg/resources/pool_stub.go

SchSeba · 2023-09-27T07:50:27Z

pkg/auxnetdevice/auxNetResourcePool.go

@@ -25,6 +25,10 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/types"
 )

+const (
+	auxPoolType = "net-sf"


NVIDIA use: scalable functions
Intel use: sub functions

so sf is good here :)

SchSeba

last comment and we can merge this PR!

Great work!

SchSeba · 2023-10-01T12:56:30Z

deployments/cdi/sriovdp-daemonset.yaml

+            memory: "200Mi"
+        volumeMounts:
+        - name: devicesock
+          mountPath: /var/lib/kubelet/


please update the mounts to be the same as #500

This commit implements Container Device Interface [1] support. [1] https://github.com/container-orchestrated-devices/container-device-interface

SchSeba

Great work!

e0ne · 2023-10-01T19:36:21Z

Thanks for the feedback, @SchSeba !

e0ne force-pushed the cdi branch 2 times, most recently from bde3b1a to a417105 Compare June 6, 2023 19:25

e0ne marked this pull request as ready for review June 12, 2023 06:47

e0ne force-pushed the cdi branch from a417105 to eccfbc7 Compare June 19, 2023 14:58

e0ne changed the title ~~WIP. CDI implementation~~ CDI implementation Jun 19, 2023