Browse code

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 <albinker@gmail.com>

Albin Kerouanton authored on 2024/12/16 19:08:44
Showing 10 changed files
... ...
@@ -1333,43 +1333,6 @@ func (s *DockerDaemonSuite) TestDaemonRestartWithContainerRunning(t *testing.T)
1333 1333
 	}
1334 1334
 }
1335 1335
 
1336
-func (s *DockerDaemonSuite) TestDaemonRestartCleanupNetns(c *testing.T) {
1337
-	s.d.StartWithBusybox(testutil.GetContext(c), c)
1338
-	out, err := s.d.Cmd("run", "--name", "netns", "-d", "busybox", "top")
1339
-	if err != nil {
1340
-		c.Fatal(out, err)
1341
-	}
1342
-
1343
-	// Get sandbox key via inspect
1344
-	out, err = s.d.Cmd("inspect", "--format", "'{{.NetworkSettings.SandboxKey}}'", "netns")
1345
-	if err != nil {
1346
-		c.Fatalf("Error inspecting container: %s, %v", out, err)
1347
-	}
1348
-	fileName := strings.Trim(out, " \r\n'")
1349
-
1350
-	if out, err := s.d.Cmd("stop", "netns"); err != nil {
1351
-		c.Fatal(out, err)
1352
-	}
1353
-
1354
-	// Test if the file still exists
1355
-	icmd.RunCommand("stat", "-c", "%n", fileName).Assert(c, icmd.Expected{
1356
-		Out: fileName,
1357
-	})
1358
-
1359
-	// Remove the container and restart the daemon
1360
-	if out, err := s.d.Cmd("rm", "netns"); err != nil {
1361
-		c.Fatal(out, err)
1362
-	}
1363
-
1364
-	s.d.Restart(c)
1365
-
1366
-	// Test again and see now the netns file does not exist
1367
-	icmd.RunCommand("stat", "-c", "%n", fileName).Assert(c, icmd.Expected{
1368
-		Err:      "No such file or directory",
1369
-		ExitCode: 1,
1370
-	})
1371
-}
1372
-
1373 1336
 // tests regression detailed in #13964 where DOCKER_TLS_VERIFY env is ignored
1374 1337
 func (s *DockerDaemonSuite) TestDaemonTLSVerifyIssue13964(c *testing.T) {
1375 1338
 	host := "tcp://localhost:4271"
... ...
@@ -1105,7 +1105,6 @@ func (c *Controller) getIPAMDriver(name string) (ipamapi.Ipam, *ipamapi.Capabili
1105 1105
 func (c *Controller) Stop() {
1106 1106
 	c.store.Close()
1107 1107
 	c.stopExternalKeyListener()
1108
-	osl.GC()
1109 1108
 }
1110 1109
 
1111 1110
 // StartDiagnostic starts the network diagnostic server listening on port.
... ...
@@ -9,7 +9,6 @@ import (
9 9
 
10 10
 	"github.com/docker/docker/internal/testutils/netnsutils"
11 11
 	"github.com/docker/docker/libnetwork/ipams/defaultipam"
12
-	"github.com/docker/docker/libnetwork/osl"
13 12
 )
14 13
 
15 14
 func TestHostsEntries(t *testing.T) {
... ...
@@ -71,6 +70,4 @@ fe90::2	somehost.example.com somehost
71 71
 	if len(ctrlr.sandboxes) != 0 {
72 72
 		t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes))
73 73
 	}
74
-
75
-	osl.GC()
76 74
 }
... ...
@@ -1737,7 +1737,6 @@ func externalKeyTest(t *testing.T, reexec bool) {
1737 1737
 		if err := cnt.Delete(context.Background()); err != nil {
1738 1738
 			t.Fatal(err)
1739 1739
 		}
1740
-		osl.GC()
1741 1740
 	}()
1742 1741
 
1743 1742
 	// Join endpoint to sandbox before SetKey
... ...
@@ -12,7 +12,6 @@ import (
12 12
 	"strings"
13 13
 	"sync"
14 14
 	"syscall"
15
-	"time"
16 15
 
17 16
 	"github.com/containerd/log"
18 17
 	"github.com/docker/docker/internal/nlwrap"
... ...
@@ -39,13 +38,8 @@ func init() {
39 39
 }
40 40
 
41 41
 var (
42
-	once             sync.Once
43
-	garbagePathMap   = make(map[string]bool)
44
-	gpmLock          sync.Mutex
45
-	gpmWg            sync.WaitGroup
46
-	gpmCleanupPeriod = 60 * time.Second
47
-	gpmChan          = make(chan chan struct{})
48
-	netnsBasePath    = filepath.Join(defaultPrefix, "netns")
42
+	once          sync.Once
43
+	netnsBasePath = filepath.Join(defaultPrefix, "netns")
49 44
 )
50 45
 
51 46
 // SetBasePath sets the base url prefix for the ns path
... ...
@@ -62,78 +56,6 @@ func createBasePath() {
62 62
 	if err != nil {
63 63
 		panic("Could not create net namespace path directory")
64 64
 	}
65
-
66
-	// Start the garbage collection go routine
67
-	go removeUnusedPaths()
68
-}
69
-
70
-func removeUnusedPaths() {
71
-	gpmLock.Lock()
72
-	period := gpmCleanupPeriod
73
-	gpmLock.Unlock()
74
-
75
-	ticker := time.NewTicker(period)
76
-	for {
77
-		var (
78
-			gc   chan struct{}
79
-			gcOk bool
80
-		)
81
-
82
-		select {
83
-		case <-ticker.C:
84
-		case gc, gcOk = <-gpmChan:
85
-		}
86
-
87
-		gpmLock.Lock()
88
-		pathList := make([]string, 0, len(garbagePathMap))
89
-		for path := range garbagePathMap {
90
-			pathList = append(pathList, path)
91
-		}
92
-		garbagePathMap = make(map[string]bool)
93
-		gpmWg.Add(1)
94
-		gpmLock.Unlock()
95
-
96
-		for _, path := range pathList {
97
-			os.Remove(path)
98
-		}
99
-
100
-		gpmWg.Done()
101
-		if gcOk {
102
-			close(gc)
103
-		}
104
-	}
105
-}
106
-
107
-func addToGarbagePaths(path string) {
108
-	gpmLock.Lock()
109
-	garbagePathMap[path] = true
110
-	gpmLock.Unlock()
111
-}
112
-
113
-func removeFromGarbagePaths(path string) {
114
-	gpmLock.Lock()
115
-	delete(garbagePathMap, path)
116
-	gpmLock.Unlock()
117
-}
118
-
119
-// GC triggers garbage collection of namespace path right away
120
-// and waits for it.
121
-func GC() {
122
-	gpmLock.Lock()
123
-	if len(garbagePathMap) == 0 {
124
-		// No need for GC if map is empty
125
-		gpmLock.Unlock()
126
-		return
127
-	}
128
-	gpmLock.Unlock()
129
-
130
-	// if content exists in the garbage paths
131
-	// we can trigger GC to run, providing a
132
-	// channel to be notified on completion
133
-	waitGC := make(chan struct{})
134
-	gpmChan <- waitGC
135
-	// wait for GC completion
136
-	<-waitGC
137 65
 }
138 66
 
139 67
 // GenerateKey generates a sandbox key based on the passed
... ...
@@ -286,18 +208,10 @@ func unmountNamespaceFile(path string) {
286 286
 
287 287
 func createNamespaceFile(path string) error {
288 288
 	once.Do(createBasePath)
289
-	// Remove it from garbage collection list if present
290
-	removeFromGarbagePaths(path)
291 289
 
292 290
 	// If the path is there unmount it first
293 291
 	unmountNamespaceFile(path)
294 292
 
295
-	// wait for garbage collection to complete if it is in progress
296
-	// before trying to create the file.
297
-	//
298
-	// 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
299
-	gpmWg.Wait()
300
-
301 293
 	f, err := os.Create(path)
302 294
 	if err != nil {
303 295
 		return err
... ...
@@ -464,8 +378,10 @@ func (n *Namespace) Destroy() error {
464 464
 		return err
465 465
 	}
466 466
 
467
-	// Stash it into the garbage collection list
468
-	addToGarbagePaths(n.path)
467
+	// Remove the path where the netns was mounted
468
+	if err := os.Remove(n.path); err != nil {
469
+		log.G(context.TODO()).WithError(err).Error("error removing namespace file")
470
+	}
469 471
 	return nil
470 472
 }
471 473
 
... ...
@@ -6,11 +6,6 @@ type Namespace struct{}
6 6
 
7 7
 func (n *Namespace) Destroy() error { return nil }
8 8
 
9
-// GC triggers garbage collection of namespace path right away
10
-// and waits for it.
11
-func GC() {
12
-}
13
-
14 9
 // GetSandboxForExternalKey returns sandbox object for the supplied path
15 10
 func GetSandboxForExternalKey(path string, key string) (*Namespace, error) {
16 11
 	return nil, nil
... ...
@@ -19,7 +19,3 @@ func NewSandbox(key string, osCreate, isRestore bool) (*Namespace, error) {
19 19
 func GetSandboxForExternalKey(path string, key string) (*Namespace, error) {
20 20
 	return nil, nil
21 21
 }
22
-
23
-// GC triggers garbage collection of namespace path right away
24
-// and waits for it.
25
-func GC() {}
... ...
@@ -21,8 +21,3 @@ func NewSandbox(key string, osCreate, isRestore bool) (*Namespace, error) {
21 21
 func GetSandboxForExternalKey(path string, key string) (*Namespace, error) {
22 22
 	return nil, nil
23 23
 }
24
-
25
-// GC triggers garbage collection of namespace path right away
26
-// and waits for it.
27
-func GC() {
28
-}
... ...
@@ -11,7 +11,6 @@ import (
11 11
 	"strings"
12 12
 	"syscall"
13 13
 	"testing"
14
-	"time"
15 14
 
16 15
 	"github.com/docker/docker/internal/nlwrap"
17 16
 	"github.com/docker/docker/internal/testutils/netnsutils"
... ...
@@ -52,11 +51,6 @@ func newKey(t *testing.T) (string, error) {
52 52
 	}
53 53
 	_ = f.Close()
54 54
 
55
-	// Set the rpmCleanupPeriod to be low to make the test run quicker
56
-	gpmLock.Lock()
57
-	gpmCleanupPeriod = 2 * time.Second
58
-	gpmLock.Unlock()
59
-
60 55
 	return name, nil
61 56
 }
62 57
 
... ...
@@ -146,17 +140,9 @@ func verifySandbox(t *testing.T, ns *Namespace, ifaceSuffixes []string) {
146 146
 	}
147 147
 }
148 148
 
149
-func verifyCleanup(t *testing.T, ns *Namespace, wait bool) {
150
-	if wait {
151
-		time.Sleep(gpmCleanupPeriod * 2)
152
-	}
153
-
149
+func verifyCleanup(t *testing.T, ns *Namespace) {
154 150
 	if _, err := os.Stat(ns.Key()); err == nil {
155
-		if wait {
156
-			t.Fatalf("The sandbox path %s is not getting cleaned up even after twice the cleanup period", ns.Key())
157
-		} else {
158
-			t.Fatalf("The sandbox path %s is not cleaned up after running gc", ns.Key())
159
-		}
151
+		t.Fatalf("The sandbox path %s is not cleaned up after running gc", ns.Key())
160 152
 	}
161 153
 }
162 154
 
... ...
@@ -420,7 +406,7 @@ func TestSandboxCreate(t *testing.T) {
420 420
 	if err != nil {
421 421
 		t.Fatal(err)
422 422
 	}
423
-	verifyCleanup(t, s, true)
423
+	verifyCleanup(t, s)
424 424
 }
425 425
 
426 426
 func TestSandboxCreateTwice(t *testing.T) {
... ...
@@ -447,8 +433,7 @@ func TestSandboxCreateTwice(t *testing.T) {
447 447
 	if err != nil {
448 448
 		t.Fatal(err)
449 449
 	}
450
-	GC()
451
-	verifyCleanup(t, s, false)
450
+	verifyCleanup(t, s)
452 451
 }
453 452
 
454 453
 func TestSandboxGC(t *testing.T) {
... ...
@@ -467,8 +452,7 @@ func TestSandboxGC(t *testing.T) {
467 467
 		t.Fatal(err)
468 468
 	}
469 469
 
470
-	GC()
471
-	verifyCleanup(t, s, false)
470
+	verifyCleanup(t, s)
472 471
 }
473 472
 
474 473
 func TestAddRemoveInterface(t *testing.T) {
... ...
@@ -530,6 +514,5 @@ func TestAddRemoveInterface(t *testing.T) {
530 530
 		t.Fatal(err)
531 531
 	}
532 532
 
533
-	GC()
534
-	verifyCleanup(t, s, false)
533
+	verifyCleanup(t, s)
535 534
 }
... ...
@@ -15,7 +15,6 @@ import (
15 15
 	"github.com/docker/docker/libnetwork/ipamutils"
16 16
 	"github.com/docker/docker/libnetwork/netlabel"
17 17
 	"github.com/docker/docker/libnetwork/options"
18
-	"github.com/docker/docker/libnetwork/osl"
19 18
 	"gotest.tools/v3/assert"
20 19
 	is "gotest.tools/v3/assert/cmp"
21 20
 )
... ...
@@ -111,8 +110,6 @@ func TestSandboxAddEmpty(t *testing.T) {
111 111
 	if len(ctrlr.sandboxes) != 0 {
112 112
 		t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes))
113 113
 	}
114
-
115
-	osl.GC()
116 114
 }
117 115
 
118 116
 // // If different priorities are specified, internal option and ipv6 addresses mustn't influence endpoint order
... ...
@@ -205,8 +202,6 @@ func TestSandboxAddMultiPrio(t *testing.T) {
205 205
 	if len(ctrlr.sandboxes) != 0 {
206 206
 		t.Fatalf("controller sandboxes is not empty. len = %d", len(ctrlr.sandboxes))
207 207
 	}
208
-
209
-	osl.GC()
210 208
 }
211 209
 
212 210
 func TestSandboxAddSamePrio(t *testing.T) {
... ...
@@ -308,6 +303,4 @@ func TestSandboxAddSamePrio(t *testing.T) {
308 308
 	if len(ctrlr.sandboxes) != 0 {
309 309
 		t.Fatalf("controller containers is not empty. len = %d", len(ctrlr.sandboxes))
310 310
 	}
311
-
312
-	osl.GC()
313 311
 }