Remove PF from pools with Pfnames and rootDevices #410

SchSeba · 2022-02-22T16:06:31Z

This commit remove the PF from pools if a Pfnames or rootDevices
selector are implemented

Signed-off-by: Sebastian Sch sebassch@gmail.com

SchSeba · 2022-02-23T13:02:24Z

pkg/utils/utils_test.go

@@ -486,26 +478,6 @@ var _ = Describe("In the utils package", func() {
 			},
 			"0000:01:10.0", "fakeSwitchdevPF", false,
 		),
-		Entry("device is a PF in switchdev mode",


I remove all the PF verification

SchSeba · 2022-02-23T15:39:32Z

reading the documentation we have carefully when using PfName and rootDevice we should only include the Vfs

"pfNames"	N	VFs from PF matches list of PF names	string list Default: null	"pfNames": ["enp2s2f0"] (See follow-up sections for some advance usage of "pfNames")
"rootDevices"	N	VFs from PF matches list of PF PCI addresses	string list Default: null	"rootDevices": ["0000:86:00.0"] (See follow-up sections for some advance usage of "rootDevices")

SchSeba · 2022-02-23T15:41:26Z

I am not sure why the CI is not able to pull the go packages..

I test it locally and it pass

make test       
running gofmt...
Running golint...
running tests...
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp	(cached)
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/accelerator	(cached)
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/factory	(cached)
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/netdevice	(cached)
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/resources	(cached)
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/utils	(cached)

SchSeba · 2022-02-23T15:58:03Z

I also clean my cache and run it again

make test
running gofmt...
building golint...
go: downloading golang.org/x/text v0.3.7
go: downloading golang.org/x/sys v0.0.0-20211019181941-9d821ace8654
go: downloading golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f
go get: installing executables with 'go get' in module mode is deprecated.
	To adjust and download dependencies of the current module, use 'go get -d'.
	To install using requirements of the current module, use 'go install'.
	To install ignoring the current module, use 'go install' with a version,
	like 'go install example.com/cmd@latest'.
	For more information, see https://golang.org/doc/go-get-install-deprecation
	or run 'go help get' or 'go help install'.
go get: upgraded golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3 => v0.0.0-20210508222113-6edffad5e616
go get: upgraded golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d => v0.0.0-20211015210444-4f30a5c0130f
go get: upgraded golang.org/x/sys v0.0.0-20210809222454-d867a43fc93e => v0.0.0-20211019181941-9d821ace8654
go get: upgraded golang.org/x/tools v0.0.0-20210106214847-113979e3529a => v0.1.9
Running golint...
running tests...
go: downloading github.com/onsi/ginkgo v1.12.0
go: downloading github.com/onsi/gomega v1.9.0
go: downloading golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
go: downloading github.com/hpcloud/tail v1.0.0
go: downloading gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7
go: downloading gopkg.in/fsnotify.v1 v1.4.7
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/cmd/sriovdp	0.663s
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/accelerator	0.099s
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/factory	0.120s
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/netdevice	0.173s
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/resources	5.251s
ok  	github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/utils	0.959s

pkg/utils/utils.go

Signed-off-by: Sebastian Sch <sebassch@gmail.com>

zshi-redhat · 2022-02-24T10:03:46Z

/lgtm

adrianchiris · 2022-02-24T11:44:12Z

please do not merge until i have a chance to review.

adrianchiris · 2022-02-24T18:03:07Z

pkg/utils/utils.go

 		}
 		return "", fmt.Errorf("error getting PF for PCI device %s %v", pciAddr, err)
 	}
 	return filepath.Base(pciinfo), nil
 }

 // GetPfName returns SRIOV PF name for the given VF
-// If device is not VF then it will return its own ifname if exist else empty string
+// If device is not VF then it will return empty string
 func GetPfName(pciAddr string) (string, error) {
 	pfEswitchMode, err := GetPfEswitchMode(pciAddr)


I think we should check if pciAddr is not a VF and exit early WDYT ?

done can you have another look?

better to use !IsSriovVF() as you might get a PF which is capable of SR-IOV with no VFs.

in this case, if the PF is in switchdev you might hit L#81 below and return the PF netdev instead of ""

same might happen in getPfAddr() but its being prevented by lines 52-53 above (before your changes)

adrianchiris

one comment, otherwise LGTM

adrianchiris · 2022-02-28T12:45:21Z

pkg/utils/utils.go

 		}
 		return "", fmt.Errorf("error getting PF for PCI device %s %v", pciAddr, err)
 	}
 	return filepath.Base(pciinfo), nil
 }

 // GetPfName returns SRIOV PF name for the given VF
-// If device is not VF then it will return its own ifname if exist else empty string
+// If device is not VF then it will return empty string
 func GetPfName(pciAddr string) (string, error) {
 	pfEswitchMode, err := GetPfEswitchMode(pciAddr)


better to use !IsSriovVF() as you might get a PF which is capable of SR-IOV with no VFs.

in this case, if the PF is in switchdev you might hit L#81 below and return the PF netdev instead of ""

same might happen in getPfAddr() but its being prevented by lines 52-53 above (before your changes)

adrianchiris · 2022-02-28T12:46:24Z

pkg/utils/utils.go

 func GetPfAddr(pciAddr string) (string, error) {
+	if IsSriovPF(pciAddr) {


i dont think its needed, lines 56-60 below ensure its a VF.

also you might still get PF (sriov capable) with no sriov configured.
so to exit early you can !IsSriovVF()

imo you can either

remove this check

replace the check with !IsSriovVF() and remove the check in L#59

This commit remove the PF from pools if a Pfnames or rootDevices selector are implemented Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba · 2022-03-01T12:00:47Z

thanks for the comments @adrianchiris

can you please give it another look?

adrianchiris

LGTM, thanks @SchSeba !

zshi-redhat · 2022-03-01T15:36:03Z

/cc @bn222

bn222 · 2022-03-01T16:33:31Z

/lgtm

SchSeba requested review from zshi-redhat and adrianchiris February 22, 2022 16:06

SchSeba force-pushed the remove_pf_name_for_pf branch from 84d058a to ff9e7ce Compare February 23, 2022 13:00

SchSeba commented Feb 23, 2022

View reviewed changes

This was referenced Feb 23, 2022

add support for vfOnly in the selector #408

Closed

Exclude PFs even if they don't have any VFs configured #409

Closed

EmilienM mentioned this pull request Feb 23, 2022

[OSDOCS-3065] Enable OVS Hardware offload openshift/openshift-docs#41247

Merged

zshi-redhat reviewed Feb 24, 2022

View reviewed changes

pkg/utils/utils.go Show resolved Hide resolved

pkg/utils/utils.go Show resolved Hide resolved

update go modules

0d7af1b

Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the remove_pf_name_for_pf branch from 901fd44 to 18142d5 Compare February 24, 2022 09:46

EmilienM mentioned this pull request Feb 24, 2022

WIP - openstack/nfv: use SRIOV network operator for DPDK openshift/release#26507

Closed

adrianchiris reviewed Feb 24, 2022

View reviewed changes

SchSeba force-pushed the remove_pf_name_for_pf branch from 18142d5 to 8035fd9 Compare February 28, 2022 09:25

adrianchiris reviewed Feb 28, 2022

View reviewed changes

Remove PF from pools with Pfnames and rootDevices

6d8ad3e

This commit remove the PF from pools if a Pfnames or rootDevices selector are implemented Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the remove_pf_name_for_pf branch from 8035fd9 to 6d8ad3e Compare March 1, 2022 11:59

adrianchiris approved these changes Mar 1, 2022

View reviewed changes

zshi-redhat approved these changes Mar 2, 2022

View reviewed changes

zshi-redhat merged commit a51564f into k8snetworkplumbingwg:master Mar 2, 2022

adrianchiris mentioned this pull request Mar 2, 2022

Devlink Error message #413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PF from pools with Pfnames and rootDevices #410

Remove PF from pools with Pfnames and rootDevices #410

SchSeba commented Feb 22, 2022

SchSeba Feb 23, 2022

SchSeba commented Feb 23, 2022

SchSeba commented Feb 23, 2022

SchSeba commented Feb 23, 2022

zshi-redhat commented Feb 24, 2022

adrianchiris commented Feb 24, 2022

adrianchiris Feb 24, 2022

SchSeba Feb 28, 2022

adrianchiris Feb 28, 2022

adrianchiris left a comment

adrianchiris Feb 28, 2022

adrianchiris Feb 28, 2022

SchSeba commented Mar 1, 2022

adrianchiris left a comment

zshi-redhat commented Mar 1, 2022

bn222 commented Mar 1, 2022

		func GetPfAddr(pciAddr string) (string, error) {
		if IsSriovPF(pciAddr) {

Remove PF from pools with Pfnames and rootDevices #410

Remove PF from pools with Pfnames and rootDevices #410

Conversation

SchSeba commented Feb 22, 2022

SchSeba Feb 23, 2022

Choose a reason for hiding this comment

SchSeba commented Feb 23, 2022

SchSeba commented Feb 23, 2022

SchSeba commented Feb 23, 2022

zshi-redhat commented Feb 24, 2022

adrianchiris commented Feb 24, 2022

adrianchiris Feb 24, 2022

Choose a reason for hiding this comment

SchSeba Feb 28, 2022

Choose a reason for hiding this comment

adrianchiris Feb 28, 2022

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris Feb 28, 2022

Choose a reason for hiding this comment

adrianchiris Feb 28, 2022

Choose a reason for hiding this comment

SchSeba commented Mar 1, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

zshi-redhat commented Mar 1, 2022

bn222 commented Mar 1, 2022