From f053beb34c1a5cedfa63cdc55ee95b765fb53a8c Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Mon, 16 Dec 2024 11:08:44 +0100 Subject: [PATCH] libnet/osl: drop netns path GC Commit 3ec19ff62b 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 --- integration-cli/docker_cli_daemon_test.go | 37 --------- libnetwork/controller.go | 1 - libnetwork/endpoint_unix_test.go | 3 - libnetwork/libnetwork_linux_test.go | 1 - libnetwork/osl/namespace_linux.go | 96 ++--------------------- libnetwork/osl/namespace_unsupported.go | 5 -- libnetwork/osl/namespace_windows.go | 4 - libnetwork/osl/sandbox_freebsd.go | 5 -- libnetwork/osl/sandbox_linux_test.go | 29 ++----- libnetwork/sandbox_unix_test.go | 7 -- 10 files changed, 12 insertions(+), 176 deletions(-) diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go index 2c97c02e4e..dcd1d9f538 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/integration-cli/docker_cli_daemon_test.go @@ -1333,43 +1333,6 @@ func (s *DockerDaemonSuite) TestDaemonRestartWithContainerRunning(t *testing.T) } } -func (s *DockerDaemonSuite) TestDaemonRestartCleanupNetns(c *testing.T) { - s.d.StartWithBusybox(testutil.GetContext(c), c) - out, err := s.d.Cmd("run", "--name", "netns", "-d", "busybox", "top") - if err != nil { - c.Fatal(out, err) - } - - // Get sandbox key via inspect - out, err = s.d.Cmd("inspect", "--format", "'{{.NetworkSettings.SandboxKey}}'", "netns") - if err != nil { - c.Fatalf("Error inspecting container: %s, %v", out, err) - } - fileName := strings.Trim(out, " \r\n'") - - if out, err := s.d.Cmd("stop", "netns"); err != nil { - c.Fatal(out, err) - } - - // Test if the file still exists - icmd.RunCommand("stat", "-c", "%n", fileName).Assert(c, icmd.Expected{ - Out: fileName, - }) - - // Remove the container and restart the daemon - if out, err := s.d.Cmd("rm", "netns"); err != nil { - c.Fatal(out, err) - } - - s.d.Restart(c) - - // Test again and see now the netns file does not exist - icmd.RunCommand("stat", "-c", "%n", fileName).Assert(c, icmd.Expected{ - Err: "No such file or directory", - ExitCode: 1, - }) -} - // tests regression detailed in #13964 where DOCKER_TLS_VERIFY env is ignored func (s *DockerDaemonSuite) TestDaemonTLSVerifyIssue13964(c *testing.T) { host := "tcp://localhost:4271" diff --git a/libnetwork/controller.go b/libnetwork/controller.go index 108865a842..74377be152 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -1105,7 +1105,6 @@ func (c *Controller) getIPAMDriver(name string) (ipamapi.Ipam, *ipamapi.Capabili func (c *Controller) Stop() { c.store.Close() c.stopExternalKeyListener() - osl.GC() } // StartDiagnostic starts the network diagnostic server listening on port. diff --git a/libnetwork/endpoint_unix_test.go b/libnetwork/endpoint_unix_test.go index a5549e3bb6..feabf93f64 100644 --- a/libnetwork/endpoint_unix_test.go +++ b/libnetwork/endpoint_unix_test.go @@ -9,7 +9,6 @@ import ( "github.com/docker/docker/internal/testutils/netnsutils" "github.com/docker/docker/libnetwork/ipams/defaultipam" - "github.com/docker/docker/libnetwork/osl" ) func TestHostsEntries(t *testing.T) { @@ -71,6 +70,4 @@ fe90::2 somehost.example.com somehost if len(ctrlr.sandboxes) != 0 { t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes)) } - - osl.GC() } diff --git a/libnetwork/libnetwork_linux_test.go b/libnetwork/libnetwork_linux_test.go index 5ba9197337..8c1dca6635 100644 --- a/libnetwork/libnetwork_linux_test.go +++ b/libnetwork/libnetwork_linux_test.go @@ -1737,7 +1737,6 @@ func externalKeyTest(t *testing.T, reexec bool) { if err := cnt.Delete(context.Background()); err != nil { t.Fatal(err) } - osl.GC() }() // Join endpoint to sandbox before SetKey diff --git a/libnetwork/osl/namespace_linux.go b/libnetwork/osl/namespace_linux.go index efbed92af1..6a5f72d7f6 100644 --- a/libnetwork/osl/namespace_linux.go +++ b/libnetwork/osl/namespace_linux.go @@ -12,7 +12,6 @@ import ( "strings" "sync" "syscall" - "time" "github.com/containerd/log" "github.com/docker/docker/internal/nlwrap" @@ -39,13 +38,8 @@ func init() { } var ( - once sync.Once - garbagePathMap = make(map[string]bool) - gpmLock sync.Mutex - gpmWg sync.WaitGroup - gpmCleanupPeriod = 60 * time.Second - gpmChan = make(chan chan struct{}) - netnsBasePath = filepath.Join(defaultPrefix, "netns") + once sync.Once + netnsBasePath = filepath.Join(defaultPrefix, "netns") ) // SetBasePath sets the base url prefix for the ns path @@ -62,78 +56,6 @@ func createBasePath() { if err != nil { panic("Could not create net namespace path directory") } - - // Start the garbage collection go routine - go removeUnusedPaths() -} - -func removeUnusedPaths() { - gpmLock.Lock() - period := gpmCleanupPeriod - gpmLock.Unlock() - - ticker := time.NewTicker(period) - for { - var ( - gc chan struct{} - gcOk bool - ) - - select { - case <-ticker.C: - case gc, gcOk = <-gpmChan: - } - - gpmLock.Lock() - pathList := make([]string, 0, len(garbagePathMap)) - for path := range garbagePathMap { - pathList = append(pathList, path) - } - garbagePathMap = make(map[string]bool) - gpmWg.Add(1) - gpmLock.Unlock() - - for _, path := range pathList { - os.Remove(path) - } - - gpmWg.Done() - if gcOk { - close(gc) - } - } -} - -func addToGarbagePaths(path string) { - gpmLock.Lock() - garbagePathMap[path] = true - gpmLock.Unlock() -} - -func removeFromGarbagePaths(path string) { - gpmLock.Lock() - delete(garbagePathMap, path) - gpmLock.Unlock() -} - -// GC triggers garbage collection of namespace path right away -// and waits for it. -func GC() { - gpmLock.Lock() - if len(garbagePathMap) == 0 { - // No need for GC if map is empty - gpmLock.Unlock() - return - } - gpmLock.Unlock() - - // if content exists in the garbage paths - // we can trigger GC to run, providing a - // channel to be notified on completion - waitGC := make(chan struct{}) - gpmChan <- waitGC - // wait for GC completion - <-waitGC } // GenerateKey generates a sandbox key based on the passed @@ -286,18 +208,10 @@ func unmountNamespaceFile(path string) { func createNamespaceFile(path string) error { once.Do(createBasePath) - // Remove it from garbage collection list if present - removeFromGarbagePaths(path) // If the path is there unmount it first unmountNamespaceFile(path) - // wait for garbage collection to complete if it is in progress - // before trying to create the file. - // - // TODO(aker): This garbage-collection was for a kernel bug in kernels 3.18-4.0.1: is this still needed on current kernels (and on kernel 3.10)? see https://github.com/moby/moby/pull/46315/commits/c0a6beba8e61d4019e1806d5241ba22007072ca2#r1331327103 - gpmWg.Wait() - f, err := os.Create(path) if err != nil { return err @@ -464,8 +378,10 @@ func (n *Namespace) Destroy() error { return err } - // Stash it into the garbage collection list - addToGarbagePaths(n.path) + // Remove the path where the netns was mounted + if err := os.Remove(n.path); err != nil { + log.G(context.TODO()).WithError(err).Error("error removing namespace file") + } return nil } diff --git a/libnetwork/osl/namespace_unsupported.go b/libnetwork/osl/namespace_unsupported.go index 622eea48d9..278449dfdb 100644 --- a/libnetwork/osl/namespace_unsupported.go +++ b/libnetwork/osl/namespace_unsupported.go @@ -6,11 +6,6 @@ type Namespace struct{} func (n *Namespace) Destroy() error { return nil } -// GC triggers garbage collection of namespace path right away -// and waits for it. -func GC() { -} - // GetSandboxForExternalKey returns sandbox object for the supplied path func GetSandboxForExternalKey(path string, key string) (*Namespace, error) { return nil, nil diff --git a/libnetwork/osl/namespace_windows.go b/libnetwork/osl/namespace_windows.go index 12c64ae551..fdd7d25498 100644 --- a/libnetwork/osl/namespace_windows.go +++ b/libnetwork/osl/namespace_windows.go @@ -19,7 +19,3 @@ func NewSandbox(key string, osCreate, isRestore bool) (*Namespace, error) { func GetSandboxForExternalKey(path string, key string) (*Namespace, error) { return nil, nil } - -// GC triggers garbage collection of namespace path right away -// and waits for it. -func GC() {} diff --git a/libnetwork/osl/sandbox_freebsd.go b/libnetwork/osl/sandbox_freebsd.go index 4a95fa8136..4d1df8ab94 100644 --- a/libnetwork/osl/sandbox_freebsd.go +++ b/libnetwork/osl/sandbox_freebsd.go @@ -21,8 +21,3 @@ func NewSandbox(key string, osCreate, isRestore bool) (*Namespace, error) { func GetSandboxForExternalKey(path string, key string) (*Namespace, error) { return nil, nil } - -// GC triggers garbage collection of namespace path right away -// and waits for it. -func GC() { -} diff --git a/libnetwork/osl/sandbox_linux_test.go b/libnetwork/osl/sandbox_linux_test.go index b998512a41..7037a07701 100644 --- a/libnetwork/osl/sandbox_linux_test.go +++ b/libnetwork/osl/sandbox_linux_test.go @@ -11,7 +11,6 @@ import ( "strings" "syscall" "testing" - "time" "github.com/docker/docker/internal/nlwrap" "github.com/docker/docker/internal/testutils/netnsutils" @@ -52,11 +51,6 @@ func newKey(t *testing.T) (string, error) { } _ = f.Close() - // Set the rpmCleanupPeriod to be low to make the test run quicker - gpmLock.Lock() - gpmCleanupPeriod = 2 * time.Second - gpmLock.Unlock() - return name, nil } @@ -146,17 +140,9 @@ func verifySandbox(t *testing.T, ns *Namespace, ifaceSuffixes []string) { } } -func verifyCleanup(t *testing.T, ns *Namespace, wait bool) { - if wait { - time.Sleep(gpmCleanupPeriod * 2) - } - +func verifyCleanup(t *testing.T, ns *Namespace) { if _, err := os.Stat(ns.Key()); err == nil { - if wait { - t.Fatalf("The sandbox path %s is not getting cleaned up even after twice the cleanup period", ns.Key()) - } else { - t.Fatalf("The sandbox path %s is not cleaned up after running gc", ns.Key()) - } + t.Fatalf("The sandbox path %s is not cleaned up after running gc", ns.Key()) } } @@ -420,7 +406,7 @@ func TestSandboxCreate(t *testing.T) { if err != nil { t.Fatal(err) } - verifyCleanup(t, s, true) + verifyCleanup(t, s) } func TestSandboxCreateTwice(t *testing.T) { @@ -447,8 +433,7 @@ func TestSandboxCreateTwice(t *testing.T) { if err != nil { t.Fatal(err) } - GC() - verifyCleanup(t, s, false) + verifyCleanup(t, s) } func TestSandboxGC(t *testing.T) { @@ -467,8 +452,7 @@ func TestSandboxGC(t *testing.T) { t.Fatal(err) } - GC() - verifyCleanup(t, s, false) + verifyCleanup(t, s) } func TestAddRemoveInterface(t *testing.T) { @@ -530,6 +514,5 @@ func TestAddRemoveInterface(t *testing.T) { t.Fatal(err) } - GC() - verifyCleanup(t, s, false) + verifyCleanup(t, s) } diff --git a/libnetwork/sandbox_unix_test.go b/libnetwork/sandbox_unix_test.go index 530306e86b..144b144af2 100644 --- a/libnetwork/sandbox_unix_test.go +++ b/libnetwork/sandbox_unix_test.go @@ -15,7 +15,6 @@ import ( "github.com/docker/docker/libnetwork/ipamutils" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/options" - "github.com/docker/docker/libnetwork/osl" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -111,8 +110,6 @@ func TestSandboxAddEmpty(t *testing.T) { if len(ctrlr.sandboxes) != 0 { t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes)) } - - osl.GC() } // // If different priorities are specified, internal option and ipv6 addresses mustn't influence endpoint order @@ -205,8 +202,6 @@ func TestSandboxAddMultiPrio(t *testing.T) { if len(ctrlr.sandboxes) != 0 { t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes)) } - - osl.GC() } func TestSandboxAddSamePrio(t *testing.T) { @@ -308,6 +303,4 @@ func TestSandboxAddSamePrio(t *testing.T) { if len(ctrlr.sandboxes) != 0 { t.Fatalf("controller containers is not empty. len = %d", len(ctrlr.sandboxes)) } - - osl.GC() }