From 51814ae189ff53d9903826f6a9df98d1d6197e1f Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Fri, 29 Nov 2019 07:07:13 +0100 Subject: [PATCH] Be more agressive acquiring the iptables lock iptables has two options to modify the behaviour trying to acquire the lock. --wait -w [seconds] maximum wait to acquire xtables lock before give up --wait-interval -W [usecs] wait time to try to acquire xtables lock interval to wait for xtables lock default is 1 second Kubernetes uses -w 5 that means that wait 5 seconds to try to acquire the lock. If we are not able to acquire it, kube-proxy fails and retries in 30 seconds, that is an important penalty on sensitive applications. We can be a bit more aggresive and try to acquire the lock every 100 msec, that means that we have to fail 50 times to not being able to succeed. --- pkg/util/iptables/iptables.go | 13 ++++++++++- pkg/util/iptables/iptables_test.go | 35 ++++++++++++++++++++++++++++-- pkg/util/iptables/monitor_test.go | 8 +++---- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/pkg/util/iptables/iptables.go b/pkg/util/iptables/iptables.go index e9f519001ed..30c6c838161 100644 --- a/pkg/util/iptables/iptables.go +++ b/pkg/util/iptables/iptables.go @@ -163,6 +163,9 @@ var RandomFullyMinVersion = utilversion.MustParseGeneric("1.6.2") // WaitMinVersion a minimum iptables versions supporting the -w and -w flags var WaitMinVersion = utilversion.MustParseGeneric("1.4.20") +// WaitIntervalMinVersion a minimum iptables versions supporting the wait interval useconds +var WaitIntervalMinVersion = utilversion.MustParseGeneric("1.6.1") + // WaitSecondsMinVersion a minimum iptables versions supporting the wait seconds var WaitSecondsMinVersion = utilversion.MustParseGeneric("1.4.22") @@ -175,6 +178,12 @@ const WaitString = "-w" // WaitSecondsValue a constant for specifying the default wait seconds const WaitSecondsValue = "5" +// WaitIntervalString a constant for specifying the wait interval flag +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 const LockfilePath16x = "/run/xtables.lock" @@ -638,6 +647,8 @@ func getIPTablesVersion(exec utilexec.Interface, protocol Protocol) (*utilversio // Checks if iptables version has a "wait" flag func getIPTablesWaitFlag(version *utilversion.Version) []string { switch { + case version.AtLeast(WaitIntervalMinVersion): + return []string{WaitString, WaitSecondsValue, WaitIntervalString, WaitIntervalUsecondsValue} case version.AtLeast(WaitSecondsMinVersion): return []string{WaitString, WaitSecondsValue} case version.AtLeast(WaitMinVersion): @@ -650,7 +661,7 @@ func getIPTablesWaitFlag(version *utilversion.Version) []string { // Checks if iptables-restore has a "wait" flag func getIPTablesRestoreWaitFlag(version *utilversion.Version, exec utilexec.Interface, protocol Protocol) []string { if version.AtLeast(WaitRestoreMinVersion) { - return []string{WaitString, WaitSecondsValue} + return []string{WaitString, WaitSecondsValue, WaitIntervalString, WaitIntervalUsecondsValue} } // Older versions may have backported features; if iptables-restore supports diff --git a/pkg/util/iptables/iptables_test.go b/pkg/util/iptables/iptables_test.go index cda4cda94c1..7020275381c 100644 --- a/pkg/util/iptables/iptables_test.go +++ b/pkg/util/iptables/iptables_test.go @@ -620,7 +620,7 @@ func TestIPTablesWaitFlag(t *testing.T) { {"1.4.21", []string{WaitString}}, {"1.4.22", []string{WaitString, WaitSecondsValue}}, {"1.5.0", []string{WaitString, WaitSecondsValue}}, - {"2.0.0", []string{WaitString, WaitSecondsValue}}, + {"2.0.0", []string{WaitString, WaitSecondsValue, WaitIntervalString, WaitIntervalUsecondsValue}}, } for _, testCase := range testCases { @@ -729,6 +729,37 @@ func TestWaitFlagNew(t *testing.T) { } } +func TestWaitIntervalFlagNew(t *testing.T) { + fcmd := fakeexec.FakeCmd{ + CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ + // iptables version check + func() ([]byte, error) { return []byte("iptables v1.6.1"), nil }, + // iptables-restore version check + func() ([]byte, error) { return []byte{}, nil }, + // Success. + func() ([]byte, error) { return []byte{}, nil }, + }, + } + fexec := fakeexec.FakeExec{ + CommandScript: []fakeexec.FakeCommandAction{ + func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, + func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, + func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, + }, + } + runner := New(&fexec, ProtocolIpv4) + err := runner.DeleteChain(TableNAT, Chain("FOOBAR")) + if err != nil { + t.Errorf("expected success, got %v", err) + } + if fcmd.CombinedOutputCalls != 3 { + t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + } + if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString, WaitSecondsValue, WaitIntervalString, WaitIntervalUsecondsValue) { + t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2]) + } +} + func testSaveInto(t *testing.T, protocol Protocol) { version := " v1.9.22" iptablesCmd := iptablesCommand(protocol) @@ -963,7 +994,7 @@ func TestRestoreAllWait(t *testing.T) { } commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...) - if !commandSet.HasAll("iptables-restore", WaitString, WaitSecondsValue, "--counters", "--noflush") { + if !commandSet.HasAll("iptables-restore", WaitString, WaitSecondsValue, WaitIntervalString, WaitIntervalUsecondsValue, "--counters", "--noflush") { t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1]) } diff --git a/pkg/util/iptables/monitor_test.go b/pkg/util/iptables/monitor_test.go index 37e9ddcd3ad..06465987097 100644 --- a/pkg/util/iptables/monitor_test.go +++ b/pkg/util/iptables/monitor_test.go @@ -100,12 +100,12 @@ func (mfc *monitorFakeCmd) CombinedOutput() ([]byte, error) { return []byte("iptables v1.6.2"), nil } - if len(mfc.args) != 6 || mfc.args[0] != WaitString || mfc.args[1] != WaitSecondsValue || mfc.args[4] != "-t" { + if len(mfc.args) != 8 || mfc.args[0] != WaitString || mfc.args[1] != WaitSecondsValue || mfc.args[2] != WaitIntervalString || mfc.args[3] != WaitIntervalUsecondsValue || mfc.args[6] != "-t" { panic(fmt.Sprintf("bad args %#v", mfc.args)) } - op := operation(mfc.args[2]) - chainName := mfc.args[3] - tableName := mfc.args[5] + op := operation(mfc.args[4]) + chainName := mfc.args[5] + tableName := mfc.args[7] mfc.mfe.Lock() defer mfc.mfe.Unlock()