Skip to content

libnet/osl: drop netns path GC#49099

Merged
vvoland merged 1 commit intomoby:masterfrom
akerouanton:libnet-netns-path-gc
Dec 16, 2024
Merged

libnet/osl: drop netns path GC#49099
vvoland merged 1 commit intomoby:masterfrom
akerouanton:libnet-netns-path-gc

Conversation

@akerouanton
Copy link
Member

@akerouanton akerouanton commented Dec 16, 2024

- What I did

Commit 3ec19ff introduced a GC goroutine to delete files where netns were mounted. It was primarly added to work around a race in kernel 3.18-4.0.1. Since no distros we support are using such old kernels, there's no need to keep this code around.

- How to verify it

Existing tests.

@akerouanton akerouanton added area/networking Networking kind/refactor PR's that refactor, or clean-up code labels Dec 16, 2024
@akerouanton akerouanton added this to the 28.0.0 milestone Dec 16, 2024
@akerouanton akerouanton self-assigned this Dec 16, 2024
@akerouanton akerouanton changed the title lbnet/osl: drop netns path GC libnet/osl: drop netns path GC Dec 16, 2024
Commit 3ec19ff introduced a GC goroutine to delete files where netns
were mounted. It was primarly added to work around a race in kernel
3.18-4.0.1. Since no distros we support are using such old kernels,
there's no need to keep this code around.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
gpmChan = make(chan chan struct{})
netnsBasePath = filepath.Join(defaultPrefix, "netns")
once sync.Once
netnsBasePath = filepath.Join(defaultPrefix, "netns")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR probably, but I was curious what this once was used for (we should probably rename it to describe what it's for); but looking at that once, the logic seems ... iffy .. it's used to call createBasePath (which is "ok"), but createBasePath depends on basepath(), which returns netnsBasePath, which can be set through an exported function SetBasePath();

  • the basepath() function for sure looks redundant (but that's a nit)
  • SetBasePath() has no synchronisation
  • It also means that SetBasePath can be called after createBasePath ran, and it would not run again (if netnsBasePath was modified

// SetBasePath sets the base url prefix for the ns path
func SetBasePath(path string) {
netnsBasePath = filepath.Join(path, "netns")
}
func basePath() string {
return netnsBasePath
}
func createBasePath() {
err := os.MkdirAll(basePath(), 0o755)
if err != nil {
panic("Could not create net namespace path directory")
}
// Start the garbage collection go routine
go removeUnusedPaths()
}

@thaJeztah
Copy link
Member

I added a link to the original PR in libnetwork (moby/libnetwork#223), which also mentions the fix in the kernel, which went in to kernel 4.1; torvalds/linux@8f502d5

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vvoland vvoland merged commit daacd6c into moby:master Dec 16, 2024
@akerouanton akerouanton deleted the libnet-netns-path-gc branch December 16, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/networking Networking kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants