Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: generate a file per resourcePool #583

Merged
merged 3 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/k8snetworkplumbingwg/sriovnet v1.2.1-0.20240128120937-3ca5e43034e6
github.com/onsi/ginkgo v1.16.5
github.com/onsi/gomega v1.33.1
github.com/opencontainers/go-digest v1.0.0
github.com/pkg/errors v0.9.1
github.com/stretchr/testify v1.9.0
github.com/vishvananda/netlink v1.2.1-beta.2
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7J
github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk=
github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16AVYg0=
github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
github.com/opencontainers/runc v1.1.12 h1:BOIssBaW1La0/qbNZHXOOa71dZfZEQOzW7dqQf3phss=
github.com/opencontainers/runc v1.1.12/go.mod h1:S+lQwSfncpBha7XTy/5lBwWgm5+y5Ma/O44Ekby9FK8=
github.com/opencontainers/runtime-spec v1.0.3-0.20220825212826-86290f6a00fb h1:1xSVPOd7/UA+39/hXEGnBJ13p6JFB0E1EvQFlrRDOXI=
Expand Down
32 changes: 29 additions & 3 deletions pkg/cdi/cdi.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ type CDI interface {

// impl implements CDI interface
type impl struct {
// lastGeneratedName is the last computed file name to which the CDI spec
// was written. This is used to remove the spec file when the resource pool
// is removed or updated.
lastGeneratedName string
souleb marked this conversation as resolved.
Show resolved Hide resolved
}

// New returns an instance of CDI interface implementation
Expand All @@ -48,9 +52,9 @@ func New() CDI {

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

Choose a reason for hiding this comment

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

question here: are we still going to cover a case where we remove a specific resource and create a new one using the same devices?

I don't see a cleanup removing resources that we don't expose anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

all CDI files prefixed with "sriov-dp" are deleted on device plugin start. (existing logic)

err := c.removeOldSpec()
souleb marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
glog.Errorf("CreateCDISpecForPool(): can not cleanup old spec files: %v", err)
glog.Errorf("CreateCDISpecForPool(): can not cleanup old spec file: %v", err)
return err
}
cdiDevices := make([]cdiSpecs.Device, 0)
Expand Down Expand Up @@ -80,12 +84,26 @@ func (c *impl) CreateCDISpecForPool(resourcePrefix string, rPool types.ResourceP
cdiSpec.Devices = append(cdiSpec.Devices, device)
}

err = cdi.GetRegistry().SpecDB().WriteSpec(&cdiSpec, cdiSpecPrefix+resourcePrefix)
// calculate hash of the cdiSpec
digest, err := extractEncodedFromDigest(Digest(cdiSpec).String())
if err != nil {
glog.Errorf("CreateCDISpecForPool(): can not calculate hash of the cdiSpec: %v", err)
return err
}
name, err := cdi.GenerateNameForTransientSpec(&cdiSpec, digest)
if err != nil {
glog.Errorf("CreateCDISpecForPool(): can not generate transient name: %v", err)
souleb marked this conversation as resolved.
Show resolved Hide resolved
return err
}

err = cdi.GetRegistry().SpecDB().WriteSpec(&cdiSpec, cdiSpecPrefix+name)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use cdiSpecPrefix+ the pool name here ?

e.g : "sriov-dp-mellanox.com-rdma_resource_1"

that way we write the same file then, no need to worry about c.lastGenratedName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with this to assure a unique name. There is no guarantee for the pool name to be unique right?

Copy link
Contributor

Choose a reason for hiding this comment

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

we can assume that same as we assume no one else will create device plugin (unix) socket per resoruce. under kubelet dir (/var/lib/kubelet/plugins_registry/).

moreover for debuggability it would be better to understand which file belongs to which resource.

if err != nil {
glog.Errorf("CreateCDISpecForPool(): can not create CDI json: %v", err)
return err
}

// save the last generated name to remove the spec file later
c.lastGeneratedName = cdiSpecPrefix + name
return nil
}

Expand All @@ -111,6 +129,14 @@ func (c *impl) CreateContainerAnnotations(devicesIDs []string, resourcePrefix, r
return annotations, nil
}

func (c *impl) removeOldSpec() error {
if c.lastGeneratedName == "" {
return nil
}
registry := cdi.GetRegistry()
return registry.SpecDB().RemoveSpec(c.lastGeneratedName)
}

// CleanupSpecs removes previously-created CDI specs
func (c *impl) CleanupSpecs() error {
for _, dir := range cdi.GetRegistry().GetSpecDirectories() {
Expand Down
51 changes: 51 additions & 0 deletions pkg/cdi/digest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cdi

import (
"encoding/json"
"fmt"
"strings"

cdiSpecs "github.com/container-orchestrated-devices/container-device-interface/specs-go"
"github.com/opencontainers/go-digest"
)

// Digest returns the digest of the given CDI spec.
func Digest(spec cdiSpecs.Spec) digest.Digest {
digester := digest.Canonical.Digester()
enc := json.NewEncoder(digester.Hash())
if err := enc.Encode(spec); err != nil {
return ""
}
return digester.Digest()
}

func extractEncodedFromDigest(digest string) (string, error) {

Check failure on line 39 in pkg/cdi/digest.go

View workflow job for this annotation

GitHub Actions / Golangci-lint

importShadow: shadow of imported from 'github.com/opencontainers/go-digest' package 'digest' (gocritic)
var encoded string
// expects a digest in the <algorithm>:<encoded> format
if pair := strings.Split(digest, ":"); len(pair) != 2 {

Check failure on line 42 in pkg/cdi/digest.go

View workflow job for this annotation

GitHub Actions / Golangci-lint

mnd: Magic number: 2, in <condition> detected (gomnd)
return "", fmt.Errorf("invalid digest %s", digest)
} else {
encoded = pair[1]
}
if len(encoded) < 12 {

Check failure on line 47 in pkg/cdi/digest.go

View workflow job for this annotation

GitHub Actions / Golangci-lint

mnd: Magic number: 12, in <condition> detected (gomnd)
return "", fmt.Errorf("invalid digest %s", digest)
}
return encoded[0:12], nil
}
52 changes: 52 additions & 0 deletions pkg/cdi/digest_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package cdi_test

import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

"github.com/container-orchestrated-devices/container-device-interface/specs-go"
"github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/cdi"
)

var _ = Describe("Digest", func() {
Context("successfully create digest", func() {
It("should return digest", func() {
spec := specs.Spec{
Version: specs.CurrentVersion,
Kind: "nvidia.com/net-pci",
Devices: []specs.Device{
{
ContainerEdits: specs.ContainerEdits{
DeviceNodes: []*specs.DeviceNode{
{
Path: "/dev/infiniband/issm0",
HostPath: "/dev/infiniband/issm0",
Permissions: "rw",
},
},
},
},
},
}
got := cdi.Digest(spec)
Expect(got.String()).To(Equal("sha256:250959bdd6c927d5eb06860bdbbd974011c3c26f06980aef3c0621a7c027140d"))
})
})
})
Loading