Reset the VF if failure occurs before the netconf is cached #222

adilGhaffarDev · 2022-08-30T07:49:26Z

This PR fixes following issue:
#212

SchSeba · 2022-08-30T16:03:26Z

SchSeba · 2022-08-30T16:12:43Z

cmd/sriov/main.go

@@ -94,6 +94,8 @@ func cmdAdd(args *skel.CmdArgs) error {
 				})
 				if err == nil {
 					_ = sm.ReleaseVF(netConf, args.IfName, args.ContainerID, netns)
+					// Reset the VF if failure occurs before the netconf is cached
+					_ = sm.ResetVFConfig(netConf)


this is not the right place.

we need to run the reset not only for non dpdk mode.
I will like to see a defer function going before

if err := sm.ApplyVFConfig(netConf); err != nil {

but for this we need to split that function to the point where we field the origin info.
can you take this part to another function?

pfLink, err := s.nLink.LinkByName(conf.Master) if err != nil { return fmt.Errorf("failed to lookup master %q: %v", conf.Master, err) } // Save current the VF state before modifying it vfState := getVfInfo(pfLink, conf.VFID) if vfState == nil { return fmt.Errorf("failed to find vf %d", conf.VFID) } conf.OrigVfState.FillFromVfInfo(vfState)

and then you can have something like

sm := sriov.NewSriovManager() (new filed origin function) (defer here with the reset function inside) if err := sm.ApplyVFConfig(netConf); err != nil { return fmt.Errorf("SRIOV-CNI failed to configure VF %q", err) }

also another change is we need a global err that we can use so if there is an error down in the stack the LIFO method of the defer function will call the release in the right order

ipam release

release vf

resetVFconfig

I have added the requested changes kindly check.

coveralls · 2022-09-07T09:28:04Z

Pull Request Test Coverage Report for Build 3412540026

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

7 of 11 (63.64%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+2.1%) to 36.428%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sriov/sriov.go	7	11	63.64%

Files with Coverage Reduction	New Missed Lines	%
pkg/sriov/sriov.go	1	35.74%

Totals
Change from base Build 3405549672:	2.1%
Covered Lines:	361
Relevant Lines:	991

💛 - Coveralls

adrianchiris

Hey there , thx for contributing !

a few comments.

once reworked it would be great if you squash to a single commit and add a descriptive commit message.

this brings #115 one step closer to completion :)

adrianchiris · 2022-09-07T17:04:47Z

cmd/sriov/main.go

+			if err == nil {
+				_ = sm.ReleaseVF(netConf, args.IfName, args.ContainerID, netns)
+				// Reset the VF if failure occurs before the netconf is cached
+				_ = sm.ResetVFConfig(netConf)


we should always do this IMO, regardless of the fact that netdev is present in container namespace.
currently if ApplyVFConfig fails it will not call ResetVFConfig here.

so lets call it after L91

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

pkg/sriov/sriov.go

@@ -126,7 +126,8 @@ type Manager interface {
 	SetupVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) (string, error)
 	ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) error
 	ResetVFConfig(conf *sriovtypes.NetConf) error
-	ApplyVFConfig(conf *sriovtypes.NetConf) error
+	ApplyVFConfig(conf *sriovtypes.NetConf, pfLink netlink.Link) error


why did you change this interface method ? just to avoid getting netlink handle twice ?
I think it should be kept as is for the sake of simplicity as before

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

pkg/sriov/sriov.go

@@ -126,7 +126,8 @@ type Manager interface {
 	SetupVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) (string, error)
 	ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) error
 	ResetVFConfig(conf *sriovtypes.NetConf) error
-	ApplyVFConfig(conf *sriovtypes.NetConf) error
+	ApplyVFConfig(conf *sriovtypes.NetConf, pfLink netlink.Link) error
+	FillOriginalVfInfo(conf *sriovtypes.NetConf) (netlink.Link, error)


the name of the method and what it returns is a bit confusing.
Id prefer that it will just do one thing and fill the original VF info.

adrianchiris · 2022-09-07T17:13:51Z

pkg/sriov/sriov.go

+	vfState := getVfInfo(pfLink, conf.VFID)
+	if vfState == nil {
+		return pfLink, fmt.Errorf("failed to find vf %d", conf.VFID)
+		//return fmt.Errorf("failed to find vf %d", conf.VFID)


nit: remove line

adrianchiris · 2022-09-07T17:14:15Z

pkg/sriov/sriov.go

@@ -374,6 +364,22 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 	return nil
 }

+// Fills original VF state


nit add golang docs string

adrianchiris · 2022-09-07T17:15:24Z

cmd/sriov/main.go

-	if err := sm.ApplyVFConfig(netConf); err != nil {
+	pfLink, err := sm.FillOriginalVfInfo(netConf)
+	if err != nil {
+		return fmt.Errorf("failed to lookup master %q: %v", netConf.Master, err)


error string should be: "failed to get original vf information: %v"

the needed info is already part of err

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

pkg/sriov/sriov.go

@@ -374,6 +364,22 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 	return nil
 }

+// Fills original VF state
+func (s *sriovManager) FillOriginalVfInfo(conf *sriovtypes.NetConf) (netlink.Link, error) {


can you add unit tests for this new method ?

I have made the changes that you have asked, kindly check it.

wizhaoredhat · 2022-09-20T18:00:04Z

cmd/sriov/main.go

+		return fmt.Errorf("failed to get original vf information: %v", err)
+	}
+	defer func() {
+		if err != nil {


There is a call here in this function:

if len(newResult.IPs) == 0 { return errors.New("IPAM plugin returned missing IP config") }

Where err wouldn't be != nil. Should we also handle that as well?

Sorry, I might be misunderstanding,if len(newResult.IPs) == 0 we are returning the error and it will not be equal to nil, defer function will release ipam and vf, and resetVFConfig. Is it wrong flow? Can you elaborate bit more what is the issue here?

The defer function checks that "if err != nil" and in that specific case err will equal nil since it is a length check. So should we actually do something like this?

if len(newResult.IPs) == 0 { err = ... return err }

Sorry I missed that. Now its fixed.

looks good now, thanks!

adilGhaffarDev · 2022-09-21T12:36:07Z

I would like to know how can I run these PR tests myself if I want to? I have seen /retest in one of the PR, wasn't able find any other. Thanks.

adilGhaffarDev · 2022-11-02T10:41:11Z

@SchSeba kindly review and trigger tests here.

adilGhaffarDev · 2022-11-07T17:22:11Z

/test-all

cyclinder · 2022-11-11T03:00:28Z

/cc @adrianchiris @SchSeba

PTAL :)

cgoncalves · 2022-11-16T10:56:39Z

LGTM

adrianchiris

LGTM!

SchSeba · 2022-11-20T14:12:52Z

This will not work as expected from here

https://github.com/k8snetworkplumbingwg/sriov-cni/pull/222/files#diff-30bddd0b0a3414968a5fa92d19e50ad3168e273728fd52ba867afbc442defe62R115

r, err := ipam.ExecAdd(netConf.IPAM.Type, args.StdinData)

you can't allocate the new err variable or it will not get caught by the defer function.

adilGhaffarDev · 2022-11-21T09:01:54Z

@SchSeba Fixed, there was another place with similar allocation that is also fixed. kindly check.

SchSeba

Thanks @adilGhaffarDev it looks good now

/lgtm

SchSeba reviewed Aug 30, 2022

View reviewed changes

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from fbc2673 to f5e9e56 Compare September 6, 2022 12:54

adrianchiris reviewed Sep 7, 2022

View reviewed changes

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from f5e9e56 to a9aa458 Compare September 12, 2022 09:56

adilGhaffarDev requested review from adrianchiris and SchSeba and removed request for adrianchiris and SchSeba September 12, 2022 13:17

wizhaoredhat reviewed Sep 20, 2022

View reviewed changes

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from a9aa458 to 39f61af Compare September 22, 2022 07:16

adilGhaffarDev requested review from SchSeba and wizhaoredhat and removed request for adrianchiris and SchSeba September 23, 2022 08:10

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from 39f61af to 79d287f Compare October 13, 2022 07:55

adilGhaffarDev closed this Oct 13, 2022

adilGhaffarDev reopened this Oct 13, 2022

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from 79d287f to 1eb89e7 Compare October 24, 2022 14:47

adilGhaffarDev requested review from SchSeba and removed request for wizhaoredhat November 1, 2022 10:41

adrianchiris mentioned this pull request Nov 7, 2022

Request: publish a new release #230

Closed

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from 1eb89e7 to bcf7aaf Compare November 7, 2022 17:20

adrianchiris approved these changes Nov 17, 2022

View reviewed changes

Reset the VF if failure occurs before the netconf is cached

e92d077

adilGhaffarDev force-pushed the vf-stuck-issue/adil branch from bcf7aaf to e92d077 Compare November 21, 2022 09:00

SchSeba approved these changes Nov 22, 2022

View reviewed changes

SchSeba merged commit 7eb235b into k8snetworkplumbingwg:master Nov 22, 2022

This was referenced Dec 19, 2022

VF configs are not reset when SetupVF fails #212

Closed

SR-IOV CNI to properly restore the state of a VF #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset the VF if failure occurs before the netconf is cached #222

Reset the VF if failure occurs before the netconf is cached #222

adilGhaffarDev commented Aug 30, 2022 •

edited

Loading

SchSeba commented Aug 30, 2022

SchSeba Aug 30, 2022

adilGhaffarDev Sep 6, 2022

coveralls commented Sep 7, 2022 •

edited

Loading

adrianchiris left a comment

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adrianchiris Sep 7, 2022

adilGhaffarDev Sep 20, 2022

wizhaoredhat Sep 20, 2022 •

edited

Loading

adilGhaffarDev Sep 21, 2022

wizhaoredhat Sep 21, 2022

adilGhaffarDev Sep 22, 2022

wizhaoredhat Sep 23, 2022

adilGhaffarDev commented Sep 21, 2022

adilGhaffarDev commented Nov 2, 2022

adilGhaffarDev commented Nov 7, 2022

cyclinder commented Nov 11, 2022

cgoncalves commented Nov 16, 2022

adrianchiris left a comment

SchSeba commented Nov 20, 2022

adilGhaffarDev commented Nov 21, 2022 •

edited

Loading

SchSeba left a comment

Reset the VF if failure occurs before the netconf is cached #222

Reset the VF if failure occurs before the netconf is cached #222

Conversation

adilGhaffarDev commented Aug 30, 2022 • edited Loading

SchSeba commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 7, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3412540026

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

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

wizhaoredhat Sep 20, 2022 • edited Loading

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

adilGhaffarDev commented Sep 21, 2022

adilGhaffarDev commented Nov 2, 2022

adilGhaffarDev commented Nov 7, 2022

cyclinder commented Nov 11, 2022

cgoncalves commented Nov 16, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

SchSeba commented Nov 20, 2022

adilGhaffarDev commented Nov 21, 2022 • edited Loading

SchSeba left a comment

Choose a reason for hiding this comment

adilGhaffarDev commented Aug 30, 2022 •

edited

Loading

coveralls commented Sep 7, 2022 •

edited

Loading

wizhaoredhat Sep 20, 2022 •

edited

Loading

adilGhaffarDev commented Nov 21, 2022 •

edited

Loading