Use temp network namespace for rename #325

adilGhaffarDev · 2025-03-24T07:47:58Z

This PR is changing how SetupVF sets up a VF in Pod netns.
Previous Approach:

Set link down
Set temp name on the link
Remove alt name
Change netns (move link to pod netns)
Rename link in netns to Pod interface name
… do other operations on the link in netns (Enable IPv4 ARP, Set MAC address, Enable Optimistic DAD etc)
Bring IF up in Pod netns

New approach:

Create new tempNS
Move link to tempNS
Rename the link in tempNS to pod interface name
Remove alt name from the link in tempNS
Move link in tempNS to pod netns
… do other operations on the link in netns (Enable IPv4 ARP, Set MAC address, Enable Optimistic DAD etc)
Bring IF up in Pod netns

Previously we were setting temp name of the link now we move the link to tempNS.
Why we are doing this?
Check this issue:
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1599
Rapid renaming of link causes race condition explained in the issue please check.
To solve the issue they used tempNS instead of renaming link: containernetworking/plugins#1073
We are trying to do the same in this PR.

Fixes #321

pkg/sriov/sriov.go

SchSeba · 2025-04-15T08:42:29Z

Hi @adilGhaffarDev can you please fix the unit-tests please

adilGhaffarDev · 2025-04-15T10:26:46Z

Hi @adilGhaffarDev can you please fix the unit-tests please

I am working on them, I want to run the tests locally but it gives lot of permission related errors. example error:

  [FAILED] Unexpected error:
      <*errors.errorString | 0xc00030c730>: 
      failed to create the sriov lock directory("/tmp/ginkgo2916429793/pci/vf_lock"): mkdir /tmp/ginkgo2916429793/pci/vf_lock: permission denied
      {
          s: "failed to create the sriov lock directory(\"/tmp/ginkgo2916429793/pci/vf_lock\"): mkdir /tmp/ginkgo2916429793/pci/vf_lock: permission denied",
      }
  occurred
  In [It] at: /home/metal3ci/sriov-cni/pkg/config/config_test.go:38

Is it possible to run these tests locally? in VM?

adilGhaffarDev · 2025-04-16T09:18:40Z

Hi @adilGhaffarDev can you please fix the unit-tests please

I am working on them, I want to run the tests locally but it gives lot of permission related errors. example error:
  [FAILED] Unexpected error:
      <*errors.errorString | 0xc00030c730>: 
      failed to create the sriov lock directory("/tmp/ginkgo2916429793/pci/vf_lock"): mkdir /tmp/ginkgo2916429793/pci/vf_lock: permission denied
      {
          s: "failed to create the sriov lock directory(\"/tmp/ginkgo2916429793/pci/vf_lock\"): mkdir /tmp/ginkgo2916429793/pci/vf_lock: permission denied",
      }
  occurred
  In [It] at: /home/metal3ci/sriov-cni/pkg/config/config_test.go:38
Is it possible to run these tests locally? in VM?

Nevermind it just needed sudo.

adilGhaffarDev · 2025-04-16T09:43:32Z

Hi @adilGhaffarDev can you please fix the unit-tests please

All tests are now passing. Please take a look at the test case "should save all the netConf struct to the cache file without dns" in pkg/utils/utils_test.go. It was previously failing, although I hadn’t modified any related code. I’ve made some adjustments to address the failure, but I’m not entirely sure why it was failing in the first place.

adilGhaffarDev · 2025-04-16T09:44:07Z

@adrianchiris @SchSeba please check

SchSeba · 2025-04-23T13:32:23Z

Hi @adilGhaffarDev,
I can see the build is still failing please take a look :)

coveralls · 2025-04-24T08:17:23Z

Pull Request Test Coverage Report for Build 19429187032

Details

43 of 86 (50.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.8%) to 62.135%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sriov/sriov.go	43	86	50.0%

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

Totals
Change from base Build 19405086786:	-0.8%
Covered Lines:	978
Relevant Lines:	1574

💛 - Coveralls

adilGhaffarDev · 2025-04-28T06:38:00Z

I can see the build is still failing please take a look :)

Now all issues are fixed 😄

adrianchiris

partial review. i still need to take a look at ReleaseVF

pkg/sriov/sriov.go

adrianchiris · 2025-04-29T11:40:55Z

pkg/sriov/sriov.go

-		_ = s.utils.EnableArpAndNdiscNotify(podifName)
+			"tempNSObj", tempNSObj,
+			"netns.Fd()", int(netns.Fd()))
+		if err := s.nLink.LinkSetNsFd(tempNSObj, int(netns.Fd())); err != nil {


same as above we should use: err = ....

pkg/sriov/sriov.go

zeeke · 2025-09-22T10:43:34Z

hi @adilGhaffarDev , I need a little clarification on the problem this PR is trying to solve, as I got no answer in #321 and the commit description does not describe it.

Even with that information, we have to simplify the SetupVF function: with a 5 defer statement insisting on the same err variable, it is pretty hard to determine every possible combination of error.

adilGhaffarDev · 2025-09-25T12:33:46Z

hi @adilGhaffarDev , I need a little clarification on the problem this PR is trying to solve, as I got no answer in #321 and the commit description does not describe it.

Even with that information, we have to simplify the SetupVF function: with a 5 defer statement insisting on the same err variable, it is pretty hard to determine every possible combination of error.

I have added in the description of PR what this PR is doing and why it is needed.
I understand that defers are making it bit confusing but those are safeguards so nothing goes wrong. I am open to suggestions in improving the defer situation. I will also check myself.

zeeke · 2025-10-02T09:52:50Z

my considerations are:
a. this PR removed the step LinkSetDown. I'm not sure why it was in place, and I'm ok with it if no one recalls why it was in place
b. Is the race condition we are trying to solve here about udev rules (like the PR description states)? or is it about having multiple CNI calls insisting on the same temp_XXX name?
c. can we avoid nested netns.Do(...) calls? They make the defer call chain very hard to understand. Can we call them sequentially?

adilGhaffarDev · 2025-10-13T16:31:54Z

c. can we avoid nested netns.Do(...) calls? They make the defer call chain very hard to understand. Can we call them sequentially?

I have removed nested netns.Do() calls. Please check, now it's much simpler. Defers are removed, and now the code is easier to understand.

adilGhaffarDev · 2025-10-20T06:16:17Z

@SchSeba @zeeke @adrianchiris . take a look when you get time.
Simplified the code and got rid of confusing defers.

zeeke

Thanks for simplifying the code. Left some comments

pkg/sriov/sriov.go

adilGhaffarDev · 2025-10-23T09:42:26Z

@zeeke comments are addressed.

adilGhaffarDev · 2025-10-27T07:41:00Z

@SchSeba @adrianchiris looking for reviews.

zeeke

only one comment from my side.
thanks for all your rework

pkg/sriov/sriov.go

zeeke

LGTM

Thanks for having addressed all my comments

adilGhaffarDev · 2025-10-29T15:00:25Z

@SchSeba @adrianchiris can we merge this?

SchSeba

I think we are getting closer I like this solution more it's easier to read and follow I would say :p

pkg/sriov/sriov.go

adilGhaffarDev · 2025-11-12T09:27:59Z

@SchSeba comments are addressed.

SchSeba · 2025-11-16T11:42:26Z

pkg/sriov/sriov.go

+		}
+
+		// Try to move interface back to initns
+		if nsLinkObj, e := s.nLink.LinkByName(linkName); e == nil {


if this line works https://github.com/k8snetworkplumbingwg/sriov-cni/pull/325/files#diff-3f93d9d758dfaab2615b76451d0a014c280e2aa12c0c97f60b2a5c8b2d68e589R202 then here we will not find the interface right?

no it should be able to find because on line 203 we doing:
linkSetNameError := s.nLink.LinkSetName(nsLinkObj, linkName)
so at 211 we should be able to find the interface.
I am changing it bit to make it more clear.

Signed-off-by: Muhammad Adil Ghaffar <[email protected]>

github-advanced-security bot found potential problems Mar 24, 2025

View reviewed changes

pkg/sriov/sriov.go Fixed Show fixed Hide fixed

adilGhaffarDev force-pushed the fixes-321/adil branch 3 times, most recently from 0b97f8d to 0ffa338 Compare March 27, 2025 10:54

adilGhaffarDev force-pushed the fixes-321/adil branch 3 times, most recently from fdf11d7 to 14b8542 Compare April 15, 2025 08:04

adilGhaffarDev force-pushed the fixes-321/adil branch 5 times, most recently from e89ae1f to a66752c Compare April 15, 2025 10:00

adilGhaffarDev force-pushed the fixes-321/adil branch from a66752c to 89bf141 Compare April 16, 2025 09:18

adilGhaffarDev force-pushed the fixes-321/adil branch 2 times, most recently from e213f83 to dd28ba8 Compare April 16, 2025 09:39

adilGhaffarDev force-pushed the fixes-321/adil branch from dd28ba8 to e0b5e0a Compare April 22, 2025 11:53

adilGhaffarDev changed the title ~~use temp network namespace for rename~~ Use temp network namespace for rename Apr 23, 2025

adilGhaffarDev force-pushed the fixes-321/adil branch 2 times, most recently from 82d6540 to d765377 Compare April 23, 2025 11:50

adilGhaffarDev force-pushed the fixes-321/adil branch 2 times, most recently from 7587548 to 5c28d18 Compare April 24, 2025 08:13

adrianchiris reviewed Apr 29, 2025

View reviewed changes

adilGhaffarDev force-pushed the fixes-321/adil branch 3 times, most recently from fcc708a to 7eaa7c0 Compare October 13, 2025 16:31

zeeke reviewed Oct 20, 2025

View reviewed changes

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

adilGhaffarDev force-pushed the fixes-321/adil branch 3 times, most recently from e6bb840 to b3a97c7 Compare October 23, 2025 09:35

zeeke reviewed Oct 27, 2025

View reviewed changes

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

adilGhaffarDev force-pushed the fixes-321/adil branch 2 times, most recently from 628c3bd to 2cc273e Compare October 27, 2025 09:08

zeeke approved these changes Oct 27, 2025

View reviewed changes

SchSeba reviewed Nov 11, 2025

View reviewed changes

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Show resolved Hide resolved

pkg/sriov/sriov.go Show resolved Hide resolved

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

adilGhaffarDev force-pushed the fixes-321/adil branch 4 times, most recently from ddc32b3 to 719aae2 Compare November 12, 2025 09:26

adilGhaffarDev requested review from SchSeba and zeeke November 12, 2025 09:27

SchSeba reviewed Nov 16, 2025

View reviewed changes

Use temp network namespace for rename

c7cbde8

Signed-off-by: Muhammad Adil Ghaffar <[email protected]>

adilGhaffarDev force-pushed the fixes-321/adil branch from 719aae2 to c7cbde8 Compare November 17, 2025 12:12

Use temp network namespace for rename #325

Are you sure you want to change the base?

Use temp network namespace for rename #325

Uh oh!

Conversation

adilGhaffarDev commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SchSeba commented Apr 15, 2025

Uh oh!

adilGhaffarDev commented Apr 15, 2025

Uh oh!

adilGhaffarDev commented Apr 16, 2025

Uh oh!

adilGhaffarDev commented Apr 16, 2025

Uh oh!

adilGhaffarDev commented Apr 16, 2025

Uh oh!

SchSeba commented Apr 23, 2025

Uh oh!

coveralls commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19429187032

Details

💛 - Coveralls

Uh oh!

adilGhaffarDev commented Apr 28, 2025

Uh oh!

adrianchiris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrianchiris Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeeke commented Sep 22, 2025

Uh oh!

adilGhaffarDev commented Sep 25, 2025

Uh oh!

zeeke commented Oct 2, 2025

Uh oh!

adilGhaffarDev commented Oct 13, 2025

Uh oh!

adilGhaffarDev commented Oct 20, 2025

Uh oh!

zeeke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adilGhaffarDev commented Oct 23, 2025

Uh oh!

adilGhaffarDev commented Oct 27, 2025

Uh oh!

zeeke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zeeke left a comment

Choose a reason for hiding this comment

Uh oh!

adilGhaffarDev commented Oct 29, 2025

Uh oh!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adilGhaffarDev commented Mar 24, 2025 •

edited

Loading

coveralls commented Apr 24, 2025 •

edited

Loading