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

Support for acquiring DDP profile using devlink #387

Merged
merged 21 commits into from
Jun 2, 2024
Merged
27 changes: 23 additions & 4 deletions pkg/netdevice/pciNetDevice.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,30 @@ func NewPciNetDevice(dev *ghw.PCIDevice,
}

func (nd *pciNetDevice) GetDDPProfiles() string {
ddpProfile := ""
pciAddr := nd.GetPciAddr()
ddpProfile, err := utils.GetDDPProfiles(pciAddr)
if err != nil {
glog.Infof("GetDDPProfiles(): unable to get ddp profiles for device %s : %q", pciAddr, err)
return ""
if utils.IsDevlinkDDPSupportedByDevice(nd.GetPfPciAddr()) {
var err error
ddpProfile, err = utils.DevlinkGetDDPProfiles(pciAddr)
if err != nil {
pfPCI := nd.GetPfPciAddr()
ddpProfile, err = utils.DevlinkGetDDPProfiles(pfPCI)
if err != nil {
Eoghan1232 marked this conversation as resolved.
Show resolved Hide resolved
// default to ddptool if devlink failed
ddpProfile, err = utils.GetDDPProfiles(pciAddr)
adrianchiris marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
glog.Infof("GetDDPProfiles(): unable to get ddp profiles for PCI %s and PF PCI device %s : %q", pciAddr, pfPCI, err)
return ""
}
}
}
} else if utils.IsDDPToolSupportedByDevice(pciAddr) {
var err error
ddpProfile, err = utils.GetDDPProfiles(pciAddr)
if err != nil {
glog.Infof("GetDDPProfiles(): unable to get ddp profiles for PCI %s and PF PCI device %s : %q", pciAddr, nd.GetPfPciAddr(), err)
return ""
}
}
Eoghan1232 marked this conversation as resolved.
Show resolved Hide resolved
return ddpProfile
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/utils/ddp.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import (
"encoding/json"
"fmt"
"os/exec"
"strings"

"github.com/pkg/errors"
)

/*
Expand Down Expand Up @@ -47,20 +50,44 @@ type DDPpackage struct {
Name string `json:"name"`
}

// 8 is an exit code of ddptool when profile was not found
const ddpNoDDPProfile = 8

var ddpExecCommand = exec.Command

// IsDDPToolSupportedByDevice checks if DDPTool can be used with device
func IsDDPToolSupportedByDevice(dev string) bool {
if _, err := GetDDPProfiles(dev); err != nil && !errors.Is(err, ErrProfileNameNotFound) {
SchSeba marked this conversation as resolved.
Show resolved Hide resolved
if exitError, ok := err.(*exec.ExitError); ok {
if exitError.ExitCode() == ddpNoDDPProfile {
return true
}
}

return false
}

return true
}

// GetDDPProfiles returns running DDP profile name if available
func GetDDPProfiles(dev string) (string, error) {
var stdout bytes.Buffer
cmd := ddpExecCommand("ddptool", "-l", "-a", "-j", "-s", dev)
cmd.Stdout = &stdout
if err := cmd.Run(); err != nil {
if strings.Contains(err.Error(), ErrProfileNameNotFound.Error()) {
return "", fmt.Errorf("error while getting DDP profiles: %w", ErrProfileNameNotFound)
}
return "", err
}

return getDDPNameFromStdout(stdout.Bytes())
}

// ErrProfileNameNotFound error when DDPTool is supported, but package name is empty
Eoghan1232 marked this conversation as resolved.
Show resolved Hide resolved
var ErrProfileNameNotFound = errors.New("DDP profile name not found")
adrianchiris marked this conversation as resolved.
Show resolved Hide resolved

func getDDPNameFromStdout(in []byte) (string, error) {
ddpInfo := &DDPInfo{}
if err := json.Unmarshal(in, ddpInfo); err != nil {
Expand Down
74 changes: 74 additions & 0 deletions pkg/utils/devlink.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package utils
Eoghan1232 marked this conversation as resolved.
Show resolved Hide resolved

import (
"fmt"

"github.com/pkg/errors"
"github.com/vishvananda/netlink"
)

// NetlinkFunction is a type of function used by netlink to get data
type NetlinkFunction func(string, string) (map[string]string, error)

const (
// fwAppNameKey is a key to extract DDP name
fwAppNameKey = "fw.app.name"
pciBus = "pci"
)

var (
netlinkDevlinkGetDeviceInfoByNameAsMap = netlink.DevlinkGetDeviceInfoByNameAsMap
Copy link
Contributor

@adrianchiris adrianchiris May 22, 2024

Choose a reason for hiding this comment

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

we already have a netlink provider [1] shouldnt we extend it with the new method ? it already has netlink calls to devlink.

then, the rest of the functions in this file would be moved to ddp.go as they are pretty specific for ddp.

in tests (ddp_test.go) we would use a mocked version of NetlinkProvider instead of defining a fake function.

[1]

GetDevLinkDeviceEswitchAttrs(ifName string) (*nl.DevlinkDevEswitchAttr, error)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @adrianchiris yes 100% agree.

I migrated the code, but have hit an issue with the mockery.

2024/05/23 14:26:14 internal error: package "bytes" without types was imported from "github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/utils"

I am trying to figure out the reason, but this is blocking me from making the changes, as I cannot regenerate the mocked version of the NetlinkProvider Interface.

I also did a test of moving the code to a test project and trying to compile but still hit the same issues with mockery.

Has anyone come across an issue like this with mockery before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found this one argoproj/argo-workflows#8251
can you try to build the mockery version please?

)

// IsDevlinkDDPSupportedByDevice checks if DDP of device can be acquired with devlink info command
func IsDevlinkDDPSupportedByDevice(device string) bool {
if _, err := devlinkGetDeviceInfoByNameAndKey(device, fwAppNameKey); err != nil {
return false
}

return true
}

// DevlinkGetDDPProfiles returns DDP for selected device
func DevlinkGetDDPProfiles(device string) (string, error) {
return devlinkGetDeviceInfoByNameAndKey(device, fwAppNameKey)
}

// DevlinkGetDeviceInfoByNameAndKeys returns values for selected keys in device info
Eoghan1232 marked this conversation as resolved.
Show resolved Hide resolved
func DevlinkGetDeviceInfoByNameAndKeys(device string, keys []string) (map[string]string, error) {
data, err := netlinkDevlinkGetDeviceInfoByNameAsMap(pciBus, device)
if err != nil {
return nil, err
}

info := make(map[string]string)

for _, key := range keys {
if value, exists := data[key]; exists {
info[key] = value
} else {
return nil, keyNotFoundError("DevlinkGetDeviceInfoByNameAndKeys", key)
}
}

return info, nil
}

// DevlinkGetDeviceInfoByNameAndKey returns values for selected key in device infol
func devlinkGetDeviceInfoByNameAndKey(device, key string) (string, error) {
keys := []string{key}
info, err := DevlinkGetDeviceInfoByNameAndKeys(device, keys)
if err != nil {
return "", err
}

return info[key], nil
}

// ErrKeyNotFound error when key is not found in the parsed response
var ErrKeyNotFound = errors.New("key could not be found")

// KeyNotFoundError returns ErrKeyNotFound
func keyNotFoundError(function, key string) error {
return fmt.Errorf("%s - %w: %s", function, ErrKeyNotFound, key)
}
71 changes: 71 additions & 0 deletions pkg/utils/devlink_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package utils

import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/ginkgo/extensions/table"
. "github.com/onsi/gomega"
"github.com/pkg/errors"
)

const (
fakeFwAppName = "fakeFwAppName"
)

var fakeNetlinkError error

var _ = BeforeSuite(func() {
fakeNetlinkError = errors.New("Fake netlink error")
})

var _ = Describe("In the devlink related functions", func() {
DescribeTable("IsDevlinkDDPSupportedByDevice",
func(nf NetlinkFunction, device string, expectedValue bool) {
netlinkDevlinkGetDeviceInfoByNameAsMap = nf
result := IsDevlinkDDPSupportedByDevice(device)
Expect(result).To(Equal(expectedValue))
},
Entry("should return true", FakeNetlinkSuccess, "fakeDevice", true),
Entry("should return true", FakeNetlinkError, "fakeDevice", false),
)
DescribeTable("DevlinkGetDDPProfiles",
func(nf NetlinkFunction, device, expectedValue string, errorValue func() error) {
netlinkDevlinkGetDeviceInfoByNameAsMap = nf
profile, err := DevlinkGetDDPProfiles(device)
if errorValue() != nil {
Expect(err).To(HaveOccurred())
} else {
Expect(err).ToNot(HaveOccurred())
}
Expect(profile).To(Equal(expectedValue))
},
Entry("should return fakeFwAppName and no error", FakeNetlinkSuccess, "fakeDevice", fakeFwAppName, func() error { return nil }),
Entry("should return empty string and error", FakeNetlinkError, "fakeDevice", "", getFakeNetlinkError),
)
DescribeTable("DevlinkGetDeviceInfoByNameAndKeys",
func(nf NetlinkFunction, device, keyToGet string, isSuccessful bool) {
netlinkDevlinkGetDeviceInfoByNameAsMap = nf
_, err := DevlinkGetDeviceInfoByNameAndKeys(device, []string{keyToGet})
if isSuccessful {
Expect(err).ToNot(HaveOccurred())
} else {
Expect(err).To(HaveOccurred())
}
},
Entry("should return no error", FakeNetlinkSuccess, "fakeDevice", fwAppNameKey, true),
Entry("should return error", FakeNetlinkSuccess, "fakeDevice", "fakeKey", false),
)
})

func FakeNetlinkSuccess(bus, device string) (map[string]string, error) {
values := make(map[string]string)
values[fwAppNameKey] = fakeFwAppName
return values, nil
}

func FakeNetlinkError(bus, device string) (map[string]string, error) {
return nil, fakeNetlinkError
}

func getFakeNetlinkError() error {
return fakeNetlinkError
}
Loading