Add pci address lock for allocated devices #220

SchSeba · 2022-08-29T12:34:53Z

This commit add a type of lock to allocated pci address

Fixes: #219

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

coveralls · 2022-08-29T12:37:04Z

Pull Request Test Coverage Report for Build 3037867348

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

19 of 102 (18.63%) changed or added relevant lines in 4 files are covered.
20 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.5%) to 32.811%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/utils.go	0	1	0.0%
pkg/config/config.go	9	12	75.0%
pkg/utils/pci_allocator.go	10	41	24.39%
pkg/utils/mocks/pci_allocator_mock.go	0	48	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/config/config.go	20	48.84%

Totals
Change from base Build 3015757156:	-1.5%
Covered Lines:	356
Relevant Lines:	1085

💛 - Coveralls

cmd/sriov/main.go

orelmisan · 2022-08-30T08:25:31Z

Hi @SchSeba, what will happen in case the ADD command fails because of the new lock?
Will there be one less interface in the Pod?

SchSeba · 2022-08-30T09:40:30Z

Hi @orelmisan nope. the pod will stay in ContainerCreating and multus will retry

example:

testpod-2jnht   Successfully assigned sriov-cni-test-2/testpod-2jnht to sriov-worker2
3m54s       Warning   DNSConfigForming         pod/testpod-2jnht   Search Line limits were exceeded, some search paths have been omitted, the applied search line is: sriov-cni-test-2.svc.cluster.local svc.cluster.local cluster.local eng.lab.tlv.redhat.com lab.tlv.redhat.com tlv.redhat.com
4m11s       Normal    AddedInterface           pod/testpod-2jnht   Add eth0 [10.244.1.45/24] from kindnet
4m11s       Warning   FailedCreatePodSandBox   pod/testpod-2jnht   Failed to create pod sandbox: rpc error: code = Unknown desc = failed to setup network for sandbox "f10739216389f1cb0b1dced00febb5612c27e2117e2312ffbce2b9ceb832d60d": [sriov-cni-test-2/testpod-2jnht:sriov-network]: error adding container to network "sriov-network": error the deivce is already allocated for pci address 0000:04:0a.4
3m57s       Normal    AddedInterface           pod/testpod-2jnht   Add eth0 [10.244.1.46/24] from kindnet
3m57s       Normal    AddedInterface           pod/testpod-2jnht   Add net1 [] from sriov-cni-test-2/sriov-network
3m57s       Normal    Pulling                  pod/testpod-2jnht   Pulling image "fedora"
3m55s       Normal    Pulled                   pod/testpod-2jnht   Successfully pulled image "fedora" in 1.767141498s
3m55s       Normal    Created                  pod/testpod-2jnht   Created container test
3m55s       Normal    Started                  pod/testpod-2jnht   Started container test
3m53s       Normal    Killing                  pod/testpod-2jnht   Stopping container test

orelmisan · 2022-08-30T10:13:06Z

Thank you for the answer @SchSeba.

orelmisan · 2022-08-30T12:09:12Z

cmd/sriov/main.go

+
+	// Check if the device is already allocated.
+	// This is to prevent issues where kubelet request to delete a pod and in the same time a new pod using the same
+	// vf is started. we can have an issue where the cmdDel of the old pod us called AFTER the cmdAdd of the new one


Suggested change

// vf is started. we can have an issue where the cmdDel of the old pod us called AFTER the cmdAdd of the new one

// vf is started. we can have an issue where the cmdDel of the old pod is called AFTER the cmdAdd of the new one

orelmisan · 2022-08-30T12:39:45Z

pkg/utils/utils.go

+
+	path := filepath.Join(allocatedPCIPath, pciAddress)
+
+	err := os.WriteFile(path, []byte(ns), 0700)


I suggest to grant this file only read and write permissions.

Suggested change

err := os.WriteFile(path, []byte(ns), 0700)

err := os.WriteFile(path, []byte(ns), 0600)

ormergi

@SchSeba does it make sense to check if a VF is allocated at cmdCheck?

SchSeba · 2022-08-30T13:03:36Z

Hi @ormergi I am not sure if that function is even implemented today via multus

cmd/sriov/main.go

SchSeba · 2022-08-30T13:07:42Z

pkg/config/config.go

+		return n, err
+	}
+
+	if isAllocated {


I moved the verification from the main to here so we produce a better error when a PCI address is still assign.
instead of this error

Failed to create pod sandbox: rpc error: code = Unknown desc = failed to create pod network sandbox k8s_client_seba_65fe3641-ebd8-4f8b-9b17-785f07b3ad3b_0(0140bb9890307e2f3f8804a06dd0f70a0970f7b8ee1254227eb5c0043ddf2f77): error adding pod seba_client to CNI network "multus-cni-network": plugin type="multus" name="multus-cni-network" failed (add): [seba/client/65fe3641-ebd8-4f8b-9b17-785f07b3ad3b:sriov-network-2]: error adding container to network "sriov-network-2": SRIOV-CNI failed to load netconf: LoadConf(): the VF 0000:d8:0b.0 does not have a interface name or a dpdk driver

we will return error that the PCI is still allocated

ormergi · 2022-08-30T14:33:49Z

Hi @ormergi I am not sure if that function is even implemented today via multus

@SchSeba The code looks like it does, check this out https://github.com/k8snetworkplumbingwg/multus-cni/blob/v3.7/cmd/main.go#L56
It may save a few iterations of the plugin when a VF is already allocated, isn't it?

EDIT: if it a lot of work or out of this PR scope, we can do it on a follow-up PR if it makes sense.

SchSeba · 2022-08-30T16:29:09Z

@ormergi I think it's I can open an issue so we track the request to implement the check function from the CNI API definition

orelmisan · 2022-08-31T08:01:30Z

Hi @SchSeba Does the new locking mechanism supports the following scenarios:

A graceful reboot of the node.
A non graceful reboot of the node (for example from a power outage).

adrianchiris · 2022-08-31T08:17:24Z

Hi @SchSeba Does the new locking mechanism supports the following scenarios:

There is also, reboot of node then force delete of pod from k8s API, in this case, when node comes up i dont think cni will be called

SchSeba · 2022-08-31T17:01:19Z

Hi @orelmisan @adrianchiris I test the 3 cases

1 . A graceful reboot of the node. - With a drain then pod moves to another node or to pending after the node is back the new pod start again

A non graceful reboot of the node (for example from a power outage). - tested 2 cases here
2.1 node is back before the eviction policy the pod moves to container creating again and after the sriov config daemon and device plugin are ready the pod start (as it's a new pod so a different netns)
2.2 node is back after eviction policy so a new pod is created and waiting on pending (again a different netns is used for the new pod)
There is also, reboot of node then force delete of pod from k8s API, in this case, when node comes up i dont think cni will be called - I test this one also when the node is back there is no pod to start and applying again the pod will be able to reuse the PCI address as it us a different netns

orelmisan · 2022-09-01T06:05:44Z

Thank you for your answer @SchSeba.
Will there be leftover files that could grow in number, and thus in size over time?

SchSeba · 2022-09-01T09:10:49Z

they may be files overtime but I don't think that can impact a system if you ask me.

orelmisan · 2022-09-01T09:24:53Z

they may be files overtime but I don't think that can impact a system if you ask me.

Will it work if these files will be written to /tmp so they will be automatically deleted on reboot?

SchSeba · 2022-09-04T11:39:53Z

we can do it. but I am scared that some auto clean in the node if there is storage pressure, will remove the files and then we can again have the same issue

orelmisan

Thank you for the changes @SchSeba

zeeke · 2022-09-05T13:54:52Z

pkg/utils/utils.go

+
+	dat, err := os.ReadFile(path)
+	if err != nil {
+		return false, "", fmt.Errorf("failed to read for pci address file for %s: %v", pciAddress, err)


Please, add the whole path to the error. It makes it easier to debug in case of errors.

Suggested change

return false, "", fmt.Errorf("failed to read for pci address file for %s: %v", pciAddress, err)

return false, "", fmt.Errorf("failed to read PCI address lock file %s: %w", path, err)

zeeke · 2022-09-05T13:56:03Z

pkg/utils/utils.go

+func CleanAllocatedPCI(pciAddress, dataDir string) error {
+	path := filepath.Join(dataDir, "pci", pciAddress)
+	if err := os.Remove(path); err != nil {
+		return fmt.Errorf("error removing PCI address lock file %s: %q", path, err)


nit: why using %q for printing errors?

Suggested change

return fmt.Errorf("error removing PCI address lock file %s: %q", path, err)

return fmt.Errorf("error removing PCI address lock file %s: %w", path, err)

it was a copy paste from CleanCachedNetConf I agree we should switch to %w but all the code use %v so I will use V and in the future we can open a PR to switch all of them together

Eoghan1232

overall, lgtm.

ormergi

@SchSeba Thanks for fixing it!

adrianchiris · 2022-09-07T17:28:54Z

pkg/config/config.go

@@ -36,6 +37,38 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
 		return nil, fmt.Errorf("LoadConf(): VF pci addr is required")
 	}

+	// Get the vf pci address
+	vfPciAddress, err := utils.GetPciAddress(n.Master, n.VFID)


n.DeviceID already holds the VF PCI address no ? why do we need this here ?

adrianchiris · 2022-09-07T17:29:57Z

pkg/types/types.go

@@ -40,6 +40,7 @@ type NetConf struct {
 	VlanQoS       *int   `json:"vlanQoS"`
 	DeviceID      string `json:"deviceID"` // PCI address of a VF in valid sysfs format
 	VFID          int
+	VFPciAddress  string


DeviceID is the VF PCI address AFAIK. did i miss anything ?

adrianchiris · 2022-09-07T17:40:50Z

pkg/utils/utils.go

 	}
 	return nil
 }
+
+func SaveAllocatedPCI(pciAddress, ns, dataDir string) error {


Shall we wrap these three methods in an interface ?

i know cache is not implemented this way, but i do not think we need to take the same approach here.

it would be great if this would be its own package with interface definition, impl and UT.
or if you prefer, part of util package but a separate file e.g pci_allocator.go WDYT ?

the dataDir (currently config.DefaultCNIDir) should be provided when creating the interface implementation (i.e provided in the "New" function)

WDYT ?

later on we can move the caching methods to their own interface as well.

adrianchiris · 2022-09-07T17:44:57Z

pkg/utils/utils.go

 	}
 	return nil
 }
+


can you add docstring to the methods below ?

adrianchiris · 2022-09-07T17:45:33Z

pkg/utils/utils.go

+	return nil
+}
+
+func IsAllocated(pciAddress, dataDir string) (bool, string, error) {


when you add docstring here, please add some info about the returned string.

adrianchiris · 2022-09-07T17:49:42Z

cmd/sriov/main.go

@@ -207,10 +212,20 @@ func cmdDel(args *skel.CmdArgs) error {
 		}
 	}

+	vfPciAddress, err := utils.GetPciAddress(netConf.Master, netConf.VFID)


netConf.DeviceID is not saved ?

adrianchiris · 2022-09-07T17:56:09Z

pkg/config/config.go

+	// This is to prevent issues where kubelet request to delete a pod and in the same time a new pod using the same
+	// vf is started. we can have an issue where the cmdDel of the old pod is called AFTER the cmdAdd of the new one
+	// This will block the new pod creation until the cmdDel is done.
+	isAllocated, allocatedNs, err := utils.IsAllocated(vfPciAddress, DefaultCNIDir)


we could roll the logic below into IsAllocated and simplify this method.

a VF is allocated if file exists and the namespace stored within it exists else it is not allocated.
if it is not allocated then IsAllocated can delete the stale file (as best effort operation) WDYT ?

adrianchiris · 2022-09-07T18:04:52Z

also it would be great if you could add more info in commit message about the issue and the changes introduced

and even better ( : - D ) if you can split PR to 2 commits (adding the locking mechanism and the other, integrating it in CNI code) with info on each would be even better :)

the latter is not a must ofc, depends on you.

SchSeba · 2022-09-12T13:57:45Z

@adrianchiris Thanks for the comments!

I make the changes let me know how it looks now :)

adrianchiris

so minor nits.

it would be great to add some UT of the added functionality.

adrianchiris · 2022-09-15T07:15:12Z

pkg/utils/pci_allocator.go

+	dataDir string
+}
+
+func NewPCIAllocator(dataDir string) PCIAllocation {


m(iiinor)nit: docstring

this method should IMO return *PCIAllocator

the caller should store it in a PCIAllocation interface

pkg/utils/pci_allocator.go

pkg/config/config.go

This is needed to lock the allocation of the same PCI address until the cmdDel is called or the kernel remove the network namespace. Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba · 2022-11-14T16:49:16Z

Hi @adrianchiris thanks for the great comments!

when you have time please have a look and let me know if you have any other comments

…i addresses Fixes: k8snetworkplumbingwg#219 Signed-off-by: Sebastian Sch <sebassch@gmail.com>

adrianchiris · 2022-11-17T13:08:50Z

Thanks for addressing my comments ! LGTM

Signed-off-by: Or Mergi <ormergi@redhat.com>

Place a delay between tests to assure resources (VF/s) are fully released before reused again on a new VMI. (results show that waiting for VMI/s to disappear is not enough) Ref: k8snetworkplumbingwg/sriov-cni#219 This workaround should be temporary until the fix [1] can be consumed. [1] k8snetworkplumbingwg/sriov-cni#220 Signed-off-by: Edward Haas <edwardh@redhat.com>

Following the flakes we have all over SR-IOV lanes due to [1] and [2], bump sriov-cni to v2.7.0 in order to consume the fix [3]. [1] kubevirt/kubevirt#6776 [2] k8snetworkplumbingwg/sriov-cni#219 [3] k8snetworkplumbingwg/sriov-cni#220 Signed-off-by: Or Mergi <ormergi@redhat.com>

Following the flakes we have all over SR-IOV lanes due to [1] and [2], bump sriov-cni to v2.7.0 in order to consume the fix [3]. [1] kubevirt/kubevirt#6776 [2] k8snetworkplumbingwg/sriov-cni#219 [3] k8snetworkplumbingwg/sriov-cni#220 Signed-off-by: Or Mergi <ormergi@redhat.com> Signed-off-by: Or Mergi <ormergi@redhat.com>

* Create allocation interface and implementation. This is needed to lock the allocation of the same PCI address until the cmdDel is called or the kernel remove the network namespace. Signed-off-by: Sebastian Sch <sebassch@gmail.com> * Use the allocator interface to prevent allocation of still in used pci addresses Fixes: k8snetworkplumbingwg#219 Signed-off-by: Sebastian Sch <sebassch@gmail.com> Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the add_pciaddress_lock branch 2 times, most recently from d9b7567 to de043e3 Compare August 29, 2022 13:53

adrianchiris reviewed Aug 29, 2022

View reviewed changes

cmd/sriov/main.go Show resolved Hide resolved

SchSeba force-pushed the add_pciaddress_lock branch from de043e3 to 60b047a Compare August 30, 2022 11:42

orelmisan reviewed Aug 30, 2022

View reviewed changes

ormergi reviewed Aug 30, 2022

View reviewed changes

SchSeba mentioned this pull request Aug 30, 2022

cmdDel fails releasing the device when kubelet deletes pause container #126

Open

SchSeba force-pushed the add_pciaddress_lock branch from 60b047a to eb95f56 Compare August 30, 2022 13:00

SchSeba force-pushed the add_pciaddress_lock branch from eb95f56 to e27e0ad Compare August 30, 2022 13:04

SchSeba commented Aug 30, 2022

View reviewed changes

SchSeba force-pushed the add_pciaddress_lock branch from e27e0ad to c0728f0 Compare August 30, 2022 13:08

orelmisan approved these changes Sep 4, 2022

View reviewed changes

zeeke reviewed Sep 5, 2022

View reviewed changes

Eoghan1232 approved these changes Sep 7, 2022

View reviewed changes

SchSeba force-pushed the add_pciaddress_lock branch from c0728f0 to a0e4c88 Compare September 7, 2022 10:09

ormergi approved these changes Sep 7, 2022

View reviewed changes

adrianchiris reviewed Sep 7, 2022

View reviewed changes

SchSeba force-pushed the add_pciaddress_lock branch from a0e4c88 to a3b8961 Compare September 12, 2022 13:56

adrianchiris reviewed Sep 15, 2022

View reviewed changes

adrianchiris mentioned this pull request Nov 7, 2022

Request: publish a new release #230

Closed

SchSeba force-pushed the add_pciaddress_lock branch from a3b8961 to 802b9ad Compare November 14, 2022 16:46

Create allocation interface and implementation.

5198a39

This is needed to lock the allocation of the same PCI address until the cmdDel is called or the kernel remove the network namespace. Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the add_pciaddress_lock branch from 802b9ad to 1ec4511 Compare November 14, 2022 16:48

SchSeba force-pushed the add_pciaddress_lock branch from 1ec4511 to 19e50d9 Compare November 14, 2022 16:51

Use the allocator interface to prevent allocation of still in used pc…

1ae1fea

…i addresses Fixes: k8snetworkplumbingwg#219 Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the add_pciaddress_lock branch from 19e50d9 to 1ae1fea Compare November 14, 2022 16:55

adrianchiris approved these changes Nov 17, 2022

View reviewed changes

adrianchiris merged commit 2aea1bb into k8snetworkplumbingwg:master Nov 17, 2022

ormergi added a commit to ormergi/kubevirtci that referenced this pull request Nov 21, 2022

test sriov-cni with k8snetworkplumbingwg/sriov-cni#220

73b39fe

Signed-off-by: Or Mergi <ormergi@redhat.com>

EdDev mentioned this pull request Nov 22, 2022

tests, sriov: Place a delay between tests kubevirt/kubevirt#8826

Closed

ormergi mentioned this pull request Nov 29, 2022

SR-IOV providers: Bump SR-IOV CNI to v2.7.0 kubevirt/kubevirtci#913

Merged

cgoncalves mentioned this pull request Dec 7, 2022

OCPBUGS-701: Add pci address lock for allocated devices openshift/sriov-cni#52

Merged

	// vf is started. we can have an issue where the cmdDel of the old pod us called AFTER the cmdAdd of the new one
	// vf is started. we can have an issue where the cmdDel of the old pod is called AFTER the cmdAdd of the new one


		path := filepath.Join(allocatedPCIPath, pciAddress)

		err := os.WriteFile(path, []byte(ns), 0700)

	err := os.WriteFile(path, []byte(ns), 0700)
	err := os.WriteFile(path, []byte(ns), 0600)

	return false, "", fmt.Errorf("failed to read for pci address file for %s: %v", pciAddress, err)
	return false, "", fmt.Errorf("failed to read PCI address lock file %s: %w", path, err)

	return fmt.Errorf("error removing PCI address lock file %s: %q", path, err)
	return fmt.Errorf("error removing PCI address lock file %s: %w", path, err)

Add pci address lock for allocated devices #220

Add pci address lock for allocated devices #220

Conversation

SchSeba commented Aug 29, 2022

coveralls commented Aug 29, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3037867348

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

orelmisan commented Aug 30, 2022 • edited Loading

SchSeba commented Aug 30, 2022

orelmisan commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi left a comment

Choose a reason for hiding this comment

SchSeba commented Aug 30, 2022

Choose a reason for hiding this comment

ormergi commented Aug 30, 2022 • edited Loading

SchSeba commented Aug 30, 2022

orelmisan commented Aug 31, 2022

adrianchiris commented Aug 31, 2022

SchSeba commented Aug 31, 2022

orelmisan commented Sep 1, 2022

SchSeba commented Sep 1, 2022

orelmisan commented Sep 1, 2022

SchSeba commented Sep 4, 2022

orelmisan left a comment

Choose a reason for hiding this comment

zeeke Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eoghan1232 left a comment

Choose a reason for hiding this comment

ormergi left a 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

adrianchiris commented Sep 7, 2022

SchSeba commented Sep 12, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Nov 14, 2022

adrianchiris commented Nov 17, 2022

coveralls commented Aug 29, 2022 •

edited

Loading

orelmisan commented Aug 30, 2022 •

edited

Loading

ormergi commented Aug 30, 2022 •

edited

Loading

zeeke Sep 5, 2022 •

edited

Loading