Consolidate network namespace handling by bleggett · Pull Request #29993 · cilium/cilium

bleggett · 2023-12-19T23:05:51Z

This is the first step towards fixing #27449 and #27450 - Removing dependence on ip netns from the internal netns lib, and consolidating netns creation/handling into one lib/spot, while not requiring mount pins for every netns creation, the way we currently do - most things (incl. tests) rarely need this.

This PR removes a few netns libs from the repo:

containernetworking/plugins/pkg/ns
vishvananda/netns from all but one spot (pkg/datapath/linux/devices_controller.go, where a sister lib wants a typecast)

in favor of an internal lib "github.com/cilium/cilium/pkg/netns" which avoids pinning netnses, and only has a dep on the unix package.

General goals of the github.com/cilium/cilium/pkg/netns package:

To be used in all Cilium code that needs to create new netnses.
Supports creating unpinned netnses owned by Cilium code.
Supports running closures in netnses previously pinned outside of Cilium code, by other processes (needed for CNI)
No support for pinning netnses owned by Cilium code, as we never actually need to do that.
Stick to stdlib sys/unix

Looking for:

Feedback on pkg/netns API surface/impl/general approach

Generally the intent is to make sure that code that creates netnses is solely responsible for closing them.

Code that is running out-of-process from the process that is the netns owner sometimes does need to run some closure in the netns context, but should not be burdened with netns lifecycle concerns, or be responsible for closing/cleanup, in virtually all cases. The API surface is designed to discourage the current overreliance on named-pin-based disposal/cleanup.

cc @ti-mo to make sure I interpreted his intent correctly.

ti-mo

Thanks for picking this up! I've renamed the PR to something more descriptive and left a few preliminary comments.

bleggett · 2024-01-09T23:35:26Z

Back from break, should be good for review.

Still working on test failures.

ti-mo · 2024-02-05T18:46:17Z

/test

bleggett · 2024-02-08T23:40:01Z

Resynced w/main - I'm good if you all are.

squeed

Looks basically good; I'd like to see it split in to a few more commits. One for the new API, one to move all test code, one for trivial changes, non-trivial changes in individual commits, and lastly one to remove old code. It would make reviewing a lot easier. Thanks!

maintainer-s-little-helper · 2024-02-19T23:25:39Z

Commits cad2332, 0d0b0b9, e979d13 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

bleggett · 2024-02-19T23:33:37Z

Looks basically good; I'd like to see it split in to a few more commits. One for the new API, one to move all test code, one for trivial changes, non-trivial changes in individual commits, and lastly one to remove old code. It would make reviewing a lot easier. Thanks!

@squeed I at least split into 3, that helps a bit:

Impl + tests
Non-test changes
Deleting old

(mergability check will fail but HTH, ty)

ti-mo · 2024-02-20T13:24:08Z

@bleggett Thanks for picking this back up, I fell sick a few weeks ago and didn't manage to continue.

I'm taking another look at this now, there's an issue with makeNetlinkFuncs() that's causing unit tests to fail and presumably also impacts the agent. The netlinkFuncs.*Subscribe members are called lazily, so makeNetlinkFuncs needs to take out a reference to the current netns and stuff it into each of the closures. This needs to be an actual NetNS reference so it doesn't get GC'd.

Also, the Build Commits workflow requires make build works for all commits individually, so I think the middle 2 commits need to be squashed together.

I'll push this up soon.

Edit: I've added the netns.Current() function to expose getCurrent() for use in makeNetlinkFuncs().

ti-mo · 2024-02-20T14:02:42Z

/test

ti-mo · 2024-02-20T15:33:47Z

The Build Commits workflow was still failing with the containernetworking removal split off as a separate commit, since it left two implementations of GetNetNSCookie in the implementation commit. I've squashed it back together and left the cilium-dbg cleanup code removal as a separate commit. That's the only thing that stood out as being separable from the other changes.

ti-mo · 2024-02-20T15:34:17Z

/test

ti-mo · 2024-02-20T15:35:56Z

/test

bleggett · 2024-02-20T17:38:01Z

@bleggett Thanks for picking this back up, I fell sick a few weeks ago and didn't manage to continue.

I'm taking another look at this now, there's an issue with makeNetlinkFuncs() that's causing unit tests to fail and presumably also impacts the agent. The netlinkFuncs.*Subscribe members are called lazily, so makeNetlinkFuncs needs to take out a reference to the current netns and stuff it into each of the closures. This needs to be an actual NetNS reference so it doesn't get GC'd.

Edit: I've added the netns.Current() function to expose getCurrent() for use in makeNetlinkFuncs().

Ah yep, this was an overzealous attempt to fully drop the final explicit "github.com/vishvananda/netns" dep. I'd initially left it like it is currently (explicitly taking the current and wrapping it in a github.com/vishvananda/netns.NetNS handle, as I wasn't sure it would take the right netns), then removed it and didn't see any local failures. I'm fine putting it back like this, it's how it was originally anyway.

ti-mo · 2024-02-21T13:36:44Z

@bleggett Already done, take a look at the current pkg/datapath/linux/devices_controller.go.

Looks like CI is still failing even after fixing makeNetlinkFuncs in ci-runtime. Do you have any cycles to investigate the remaining failures?

bleggett · 2024-02-21T15:53:39Z

@bleggett Already done, take a look at the current pkg/datapath/linux/devices_controller.go.

Looks like CI is still failing even after fixing makeNetlinkFuncs in ci-runtime. Do you have any cycles to investigate the remaining failures?

👍 Feel free to beat me to it if you want, otherwise I'll get to it in a day or two.

tommyp1ckles

ipam changes lgtm

bleggett · 2024-02-27T23:44:26Z

Can I get someone to /test?

I can't repro any unit test failures vis a vis main but need to check the conformance suites.

youngnick · 2024-02-27T23:58:03Z

/test

bleggett · 2024-02-28T16:26:31Z

@ti-mo et al - tests seem good to me locally, and the conformance tests are passing now.

I note that one each of the gateway API and ginkgo subtests are failing, I do not think that's related to this, but am happy to be corrected.

This should be good to merge if everyone is happy.

ti-mo · 2024-02-29T12:07:22Z

@bleggett Thanks for taking a look! What did you end up changing to address the failures?

Rebasing onto main to pick up #31017 for https://github.com/cilium/cilium/actions/runs/8072988990/job/22055814369.

This allows surfacing unexpected errors, but ignores interfaces that are already absent. Signed-off-by: Timo Beckers <timo@isovalent.com>

This was already redundant when running Cilium inside a container, since the nsfs instance inside the container is bound to the container's lifecycle. Running Cilium outside of a container is currently rather involved and not officially supported. Remove the netns cleanup code. When the container exits, cilium-health and its enclosing namespace also disappears. Follow-up commits will remove the code in package netns being called here. Signed-off-by: Timo Beckers <timo@isovalent.com>

The previous netns package had a few problems. It shelled out to iproute2, and it depended on both containernetworking/plugins/pkg/ns and vishvananda/netns, which lead to some idiosyncratic API. This commit addresses these issues and takes care of some much-needed API cleanup: - Create a new netns with New() - Open an existing pinned netns with OpenPinned() - Execute code within the netns with ns.Do() - Close with Close() Pinning network namespaces is not supported, as there is currently little reason for doing so. In case the requirement pops up again later, it can always be added. All tests now use anonymous (non-pinned) network namespaces, and the netns created for cilium-health also no longer leaves an entry in /var/run/netns. Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io> Co-authored-by: Timo Beckers <timo@isovalent.com>

ti-mo · 2024-02-29T12:07:49Z

/test

sayboras

LGTM from vendor team

bleggett · 2024-02-29T19:17:58Z

@bleggett Thanks for taking a look! What did you end up changing to address the failures?

Rebasing onto main to pick up #31017 for https://github.com/cilium/cilium/actions/runs/8072988990/job/22055814369.

Absolutely nothing, just resynced to main 😅

bleggett · 2024-02-29T19:18:37Z

TY all - apologies this took a bit of time 😅

bleggett requested review from a team as code owners December 19, 2023 23:05

bleggett requested review from joamaki and pippolo84 December 19, 2023 23:05

maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 19, 2023

bleggett requested review from derailed, lmb and tommyp1ckles December 19, 2023 23:05

github-actions Bot added the kind/community-contribution This was a contribution made by a community member. label Dec 19, 2023

ti-mo marked this pull request as draft December 20, 2023 12:32

ti-mo self-requested a review December 20, 2023 12:33

ti-mo changed the title ~~[WIP] Initial step towards fixing #27449~~ Consolidate network namespace handling Dec 20, 2023

ti-mo reviewed Dec 20, 2023

View reviewed changes

ti-mo added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Dec 20, 2023

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 20, 2023

bleggett force-pushed the bleggett/consolidate-netns-27449 branch 3 times, most recently from b0b40c3 to ea1d9eb Compare January 9, 2024 23:29

bleggett marked this pull request as ready for review January 9, 2024 23:33

bleggett requested a review from a team as a code owner January 9, 2024 23:33

bleggett requested review from sayboras and ti-mo January 9, 2024 23:33

bleggett force-pushed the bleggett/consolidate-netns-27449 branch from 64309b4 to a6c76f2 Compare January 10, 2024 22:15

squeed reviewed Feb 13, 2024

View reviewed changes

Comment thread cilium-dbg/cmd/post_uninstall_cleanup.go

squeed reviewed Feb 13, 2024

View reviewed changes

Comment thread cilium-health/launch/endpoint.go Outdated

squeed reviewed Feb 13, 2024

View reviewed changes

Comment thread plugins/cilium-cni/cmd/cmd.go

squeed previously requested changes Feb 13, 2024

View reviewed changes

tommyp1ckles reviewed Feb 16, 2024

View reviewed changes

Comment thread cilium-health/launch/endpoint.go

Comment thread cilium-health/launch/endpoint.go

tommyp1ckles approved these changes Feb 23, 2024

View reviewed changes

ti-mo and others added 3 commits February 29, 2024 13:07

link: ignore missing interfaces in DeleteByName

c79aaa8

This allows surfacing unexpected errors, but ignores interfaces that are already absent. Signed-off-by: Timo Beckers <timo@isovalent.com>

sayboras approved these changes Feb 29, 2024

View reviewed changes

marseel mentioned this pull request Mar 11, 2024

Memory usage regression on main branch #31324

Closed

Conversation

bleggett commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bleggett commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo commented Feb 5, 2024

Uh oh!

bleggett commented Feb 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maintainer-s-little-helper Bot commented Feb 19, 2024

Uh oh!

bleggett commented Feb 19, 2024

Uh oh!

ti-mo commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo commented Feb 20, 2024

Uh oh!

ti-mo commented Feb 20, 2024

Uh oh!

ti-mo commented Feb 20, 2024

Uh oh!

ti-mo commented Feb 20, 2024

Uh oh!

bleggett commented Feb 20, 2024

Uh oh!

ti-mo commented Feb 21, 2024

Uh oh!

bleggett commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tommyp1ckles left a comment

Choose a reason for hiding this comment

Uh oh!

bleggett commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youngnick commented Feb 27, 2024

Uh oh!

bleggett commented Feb 28, 2024

Uh oh!

ti-mo commented Feb 29, 2024

Uh oh!

ti-mo commented Feb 29, 2024

Uh oh!

sayboras left a comment

Choose a reason for hiding this comment

Uh oh!

bleggett commented Feb 29, 2024

Uh oh!

bleggett commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bleggett commented Dec 19, 2023 •

edited

Loading

bleggett commented Jan 9, 2024 •

edited

Loading

ti-mo commented Feb 20, 2024 •

edited

Loading

bleggett commented Feb 21, 2024 •

edited

Loading

bleggett commented Feb 27, 2024 •

edited

Loading