Merge pull request #94553 from knight42/fix/TestRestoreAllWaitOldIptablesRestore

Deflake TestRestoreAllWaitOldIptablesRestore

pkg/util/iptables/iptables.go

@@ -186,9 +186,12 @@ const WaitIntervalString = "-W"
 // WaitIntervalUsecondsValue a constant for specifying the default wait interval useconds
 const WaitIntervalUsecondsValue = "100000"
 
-// LockfilePath16x is the iptables lock file acquired by any process that's making any change in the iptable rule
+// LockfilePath16x is the iptables 1.6.x lock file acquired by any process that's making any change in the iptable rule
 const LockfilePath16x = "/run/xtables.lock"
 
+// LockfilePath14x is the iptables 1.4.x lock file acquired by any process that's making any change in the iptable rule
+const LockfilePath14x = "@xtables"
+
 // runner implements Interface in terms of exec("iptables").
 type runner struct {
 	mu              sync.Mutex
@@ -198,20 +201,24 @@ type runner struct {
 	hasRandomFully  bool
 	waitFlag        []string
 	restoreWaitFlag []string
-	lockfilePath    string
+	lockfilePath14x string
+	lockfilePath16x string
 }
 
 // newInternal returns a new Interface which will exec iptables, and allows the
 // caller to change the iptables-restore lockfile path
-func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath string) Interface {
+func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath14x, lockfilePath16x string) Interface {
 	version, err := getIPTablesVersion(exec, protocol)
 	if err != nil {
 		klog.Warningf("Error checking iptables version, assuming version at least %s: %v", MinCheckVersion, err)
 		version = MinCheckVersion
 	}
 
-	if lockfilePath == "" {
-		lockfilePath = LockfilePath16x
+	if lockfilePath16x == "" {
+		lockfilePath16x = LockfilePath16x
+	}
+	if lockfilePath14x == "" {
+		lockfilePath14x = LockfilePath14x
 	}
 
 	runner := &runner{
@@ -221,14 +228,15 @@ func newInternal(exec utilexec.Interface, protocol Protocol, lockfilePath string
 		hasRandomFully:  version.AtLeast(RandomFullyMinVersion),
 		waitFlag:        getIPTablesWaitFlag(version),
 		restoreWaitFlag: getIPTablesRestoreWaitFlag(version, exec, protocol),
-		lockfilePath:    lockfilePath,
+		lockfilePath14x: lockfilePath14x,
+		lockfilePath16x: lockfilePath16x,
 	}
 	return runner
 }
 
 // New returns a new Interface which will exec iptables.
 func New(exec utilexec.Interface, protocol Protocol) Interface {
-	return newInternal(exec, protocol, "")
+	return newInternal(exec, protocol, "", "")
 }
 
 // EnsureChain is part of Interface.
@@ -390,7 +398,7 @@ func (runner *runner) restoreInternal(args []string, data []byte, flush FlushFla
 	// from stepping on each other.  iptables-restore 1.6.2 will have
 	// a --wait option like iptables itself, but that's not widely deployed.
 	if len(runner.restoreWaitFlag) == 0 {
-		locker, err := grabIptablesLocks(runner.lockfilePath)
+		locker, err := grabIptablesLocks(runner.lockfilePath14x, runner.lockfilePath16x)
 		if err != nil {
 			return err
 		}

pkg/util/iptables/iptables_linux.go

@@ -49,7 +49,7 @@ func (l *locker) Close() error {
 	return utilerrors.NewAggregate(errList)
 }
 
-func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) {
+func grabIptablesLocks(lockfilePath14x, lockfilePath16x string) (iptablesLocker, error) {
 	var err error
 	var success bool
 
@@ -66,9 +66,9 @@ func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) {
 	// can't assume which lock method it'll use.
 
 	// Roughly duplicate iptables 1.6.x xtables_lock() function.
-	l.lock16, err = os.OpenFile(lockfilePath, os.O_CREATE, 0600)
+	l.lock16, err = os.OpenFile(lockfilePath16x, os.O_CREATE, 0600)
 	if err != nil {
-		return nil, fmt.Errorf("failed to open iptables lock %s: %v", lockfilePath, err)
+		return nil, fmt.Errorf("failed to open iptables lock %s: %v", lockfilePath16x, err)
 	}
 
 	if err := wait.PollImmediate(200*time.Millisecond, 2*time.Second, func() (bool, error) {
@@ -82,7 +82,7 @@ func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) {
 
 	// Roughly duplicate iptables 1.4.x xtables_lock() function.
 	if err := wait.PollImmediate(200*time.Millisecond, 2*time.Second, func() (bool, error) {
-		l.lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: "@xtables", Net: "unix"})
+		l.lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockfilePath14x, Net: "unix"})
 		if err != nil {
 			return false, nil
 		}

pkg/util/iptables/iptables_test.go

@@ -35,7 +35,11 @@ import (
 	fakeexec "k8s.io/utils/exec/testing"
 )
 
-const TestLockfilePath = "xtables.lock"
+func getLockPaths() (string, string) {
+	lock14x := fmt.Sprintf("@xtables-%d", time.Now().Nanosecond())
+	lock16x := fmt.Sprintf("xtables-%d.lock", time.Now().Nanosecond())
+	return lock14x, lock16x
+}
 
 func testIPTablesVersionCmds(t *testing.T, protocol Protocol) {
 	version := " v1.4.22"
@@ -934,8 +938,8 @@ func TestRestoreAll(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err != nil {
@@ -975,8 +979,8 @@ func TestRestoreAllWait(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err != nil {
@@ -1020,8 +1024,8 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err != nil {
@@ -1065,24 +1069,23 @@ func TestRestoreAllGrabNewLock(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	// Grab the /run lock and ensure the RestoreAll fails
-	runLock, err := os.OpenFile(TestLockfilePath, os.O_CREATE, 0600)
+	runLock, err := os.OpenFile(lockPath16x, os.O_CREATE, 0600)
 	if err != nil {
-		t.Fatalf("expected to open %s, got %v", TestLockfilePath, err)
+		t.Fatalf("expected to open %s, got %v", lockPath16x, err)
 	}
 	defer runLock.Close()
 
 	if err := grabIptablesFileLock(runLock); err != nil {
-		t.Errorf("expected to lock %s, got %v", TestLockfilePath, err)
+		t.Errorf("expected to lock %s, got %v", lockPath16x, err)
 	}
 
 	err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err == nil {
-		t.Errorf("expected failure, got success instead")
+		t.Fatal("expected failure, got success instead")
 	}
 	if !strings.Contains(err.Error(), "failed to acquire new iptables lock: timed out waiting for the condition") {
 		t.Errorf("expected timeout error, got %v", err)
@@ -1107,22 +1110,21 @@ func TestRestoreAllGrabOldLock(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	var runLock *net.UnixListener
 	// Grab the abstract @xtables socket, will retry if the socket exists
 	err := wait.PollImmediate(time.Second, wait.ForeverTestTimeout, func() (done bool, err error) {
-		runLock, err = net.ListenUnix("unix", &net.UnixAddr{Name: "@xtables", Net: "unix"})
+		runLock, err = net.ListenUnix("unix", &net.UnixAddr{Name: lockPath14x, Net: "unix"})
 		if err != nil {
-			t.Logf("Failed to lock @xtables: %v, will retry.", err)
+			t.Logf("Failed to lock %s: %v, will retry.", lockPath14x, err)
 			return false, nil
 		}
 		return true, nil
 	})
 	if err != nil {
-		t.Fatal("Timed out locking @xtables")
+		t.Fatalf("Timed out locking %s", lockPath14x)
 	}
 	if runLock == nil {
 		t.Fatal("Unexpected nil runLock")
@@ -1132,7 +1134,7 @@ func TestRestoreAllGrabOldLock(t *testing.T) {
 
 	err = runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err == nil {
-		t.Errorf("expected failure, got success instead")
+		t.Fatal("expected failure, got success instead")
 	}
 	if !strings.Contains(err.Error(), "failed to acquire old iptables lock: timed out waiting for the condition") {
 		t.Errorf("expected timeout error, got %v", err)
@@ -1160,8 +1162,8 @@ func TestRestoreAllWaitBackportedIptablesRestore(t *testing.T) {
 			func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
 		},
 	}
-	runner := newInternal(&fexec, ProtocolIPv4, TestLockfilePath)
-	defer os.Remove(TestLockfilePath)
+	lockPath14x, lockPath16x := getLockPaths()
+	runner := newInternal(&fexec, ProtocolIPv4, lockPath14x, lockPath16x)
 
 	err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
 	if err != nil {

pkg/util/iptables/iptables_unsupported.go

@@ -23,7 +23,7 @@ import (
 	"os"
 )
 
-func grabIptablesLocks(lockfilePath string) (iptablesLocker, error) {
+func grabIptablesLocks(lock14filePath, lock16filePath string) (iptablesLocker, error) {
 	return nil, fmt.Errorf("iptables unsupported on this platform")
 }