proxy/iptables: don't call iptables-restore when we don't need to #36006

dcbw · 2016-11-01T21:09:48Z

Diff old and new rules and don't bother calling iptables-restore when nothing has actually changed.

iptables-restore is a very heavy operation and depending on the kernel,
the CPU, and the number of rules to restore, can take a very long time
(~500ms or more).

i7-5600U @ 2.6GHz: 700ms for 1000 services (2+4 cores+threads, kernel 4.6.6)
i7-4790 @ 3.6GHz: 270ms for 1000 services (4+8 cores+threads, kernel 4.6.7)

Other parts of kubernetes (eg kubenet) might be running iptables-restore
too, leading to some pileup as each iptables-restore waits for others to
complete.

Related: #26637
Related: #33693
Related: #35334

This change is

@thockin @timothysc @danwinship @eparis

dcbw · 2016-11-01T22:13:53Z

@k8s-bot unit test this #35898

dcbw · 2016-11-01T22:24:20Z

Rebase to get revert that fixes #35898

dcbw · 2016-11-02T14:24:18Z

@k8s-bot node e2e test this issue #35983

danwinship · 2016-11-01T21:46:50Z

pkg/proxy/iptables/proxier_test.go

 			if destIP != "" {
-				return strings.Contains(r[iptablestest.Destination], destIP)
+				_, ok := r.Options[iptablestest.Destination]


This change (and the destPort change below) seems wrong; it only tests that r has some destination, not that it's the right one

danwinship · 2016-11-01T21:51:55Z

pkg/util/iptables/testing/fake.go

 // no-op implementation of iptables Interface
 type FakeIPTables struct {
-	Lines []byte
+	RestoreLines []byte
+	SaveLines    []byte


SaveLines seems to be unused

Moved to the commit that actually uses it.

danwinship · 2016-11-01T21:54:02Z

pkg/util/iptables/iptables.go

+}
+
+func parseRule(line string, filterChains []Chain, filterChainPrefixes []string) (*Rule, Chain, error) {
+	knownOptions := []knownOption{


should be a global (and const?)

Arrays can't be 'const' in Go, something about being able to take the address of an array element which would be illegal for a const. It also shouldn't really be global since it's used in only one function, so I moved the struct declaration into the function too.

But that means it's going to be constructing the array every time you call the function. If you made it global it would only have to do it once.

danwinship · 2016-11-01T21:59:28Z

pkg/util/iptables/iptables.go

+				return nil, "", fmt.Errorf("invalid iptables rule (not enough arguments): %v", line)
+			}
+			if len(items[i]) == 0 {
+				return nil, "", fmt.Errorf("invalid iptables rule (empty argument): %v", line)


actually it just means there were multiple spaces rather than just 1... which doesn't seem like it should necessarily be invalid. (also, you don't check for empty argument if it's quoted)

Fixed, multiple spaces are now handled initially by a helper that splits the line, and empty quoted arguments are passed through as-is (sans quoting of course).

danwinship · 2016-11-01T22:00:20Z

pkg/util/iptables/iptables.go

+						option.Arg += " "
+					}
+					option.Arg += items[j]
+					if strings.HasSuffix(option.Arg, "\"") {


maybe use HasPrefix for the initial quote above for consistency?

danwinship · 2016-11-02T14:10:12Z

pkg/util/iptables/iptables.go

+		} else if strings.HasPrefix(line, "-A ") {
+			rule, chain, err := parseRule(line, filterChains, filterChainPrefixes)
+			if err != nil {
+				return make(map[Chain][]Rule), err


return nil, err? (likewise elsewhere)

Fixed for the error cases.

danwinship · 2016-11-02T14:17:20Z

pkg/proxy/iptables/proxier.go

@@ -235,7 +236,7 @@ func NewProxier(ipt utiliptables.Interface, sysctl utilsysctl.Interface, exec ut
 		return nil, fmt.Errorf("invalid iptables-masquerade-bit %v not in [0, 31]", masqueradeBit)
 	}
 	masqueradeValue := 1 << uint(masqueradeBit)
-	masqueradeMark := fmt.Sprintf("%#08x/%#08x", masqueradeValue, masqueradeValue)
+	masqueradeMark := fmt.Sprintf("%#04x/%#04x", masqueradeValue, masqueradeValue)


possibly in the wrong commit?

This is not right. At least on my machines, it is actually best-fit:

-A FOO -j MARK --set-xmark 0x1/0x1 -A FOO -j MARK --set-xmark 0x10/0x10 -A FOO -j MARK --set-xmark 0x100/0x100 -A FOO -j MARK --set-xmark 0x1000/0x1000 -A FOO -j MARK --set-xmark 0x10000/0x10000 -A FOO -j MARK --set-xmark 0x100000/0x100000 -A FOO -j MARK --set-xmark 0x1000000/0x1000000 -A FOO -j MARK --set-xmark 0x10000000/0x10000000

danwinship · 2016-11-02T14:20:16Z

pkg/proxy/iptables/proxier.go

+	newChains, err := parseKubeProxyTableAddRules(table, newLines)
+	if err != nil {
+		return false, err
+	}


There's a difference between oldLines failing to parse and newLines failing to parse: oldLines can fail to parse if the administrator mucks around with iptables behind our back, or if the format of iptables-save changes. But newLines can only fail to parse in the event of a programmer error.

True, fixed.

danwinship · 2016-11-02T14:24:59Z

pkg/proxy/iptables/proxier.go

-		// Revert new local ports.
-		revertPorts(replacementPortsMap, proxier.portsMap)
-		return
+		glog.Warningf("failed to parse iptables filter rules: %v", err)


In some failure cases (eg, iptables-save output format changes), every single call to sameIptablesRuleset() will fail... we might want to not log every time?

It'll now only warn if parsing the new rules failed, which should be entirely under our control.

danwinship · 2016-11-02T14:26:05Z

pkg/proxy/iptables/proxier_test.go

+		stickyMaxAgeSeconds: 180,                     // TODO: paramaterize this in the API.
+		clusterIP:           ip,
+		protocol:            protocol,
+		port:                port,
 		onlyNodeLocalEndpoints: onlyNodeLocalEndpoints,
 	}


not sure what happened there but this isn't gofmt'ed correctly

actually it is; that's what gofmt -s -w spit out for this file. Maybe Go 1.7 now considers variable length?

timothysc · 2016-11-03T21:35:16Z

re-assigning, b/c daniel is out.

dcbw · 2016-11-09T20:20:08Z

@danwinship PTAL, thanks!

dcbw · 2016-11-09T20:51:29Z

integration test flake is #36473

dcbw · 2016-11-09T21:25:50Z

@k8s-bot test this #36473

danwinship · 2016-11-10T17:59:54Z

pkg/util/iptables/iptables.go

+// Split a line into space-separated arguments, handling simple quoting
+func splitLine(line string) ([]string, error) {
+	items := strings.Split(line, " ")
+	args := make([]string, 0)


consider make([]string, 0, len(items)) as an optimization

danwinship · 2016-11-10T18:03:36Z

pkg/util/iptables/iptables.go

+}
+
+func parseRule(line string, allowChains []Chain, allowChainPrefixes []string) (*Rule, Chain, error) {
+	optionAttrs := map[string]optionAttrs{


as mentioned above, this has to be re-built every time you call parseRule(), while if it was global it would only have to be built once

also maybe sort them into four groups by the optionAttr values, with a comment above each group? (// arg required, can't negate, etc)

danwinship · 2016-11-10T18:07:43Z

pkg/util/iptables/iptables.go

+
+		attr, ok := optionAttrs[opt]
+		if !ok {
+			return nil, "", fmt.Errorf("Unknown iptables rule option %v", opt)


hm, I think there are some more places where you said %v when you meant %q (across multiple commits)

danwinship · 2016-11-10T18:14:56Z

pkg/util/iptables/iptables.go

+			// negation only valid at beginning of options
+			if items[i] == "!" {
+				negated = true
+				continue


It would simplify the loop if you just did an i++ here like you do for options with arguments. (eg, then both opt and negated would be local to the loop body)

I had it that way before, but then we have to also check if i == len(items) a lot. I'll try it again this way and see.

danwinship · 2016-11-10T18:22:41Z

pkg/util/iptables/iptables_test.go

+		args, err := splitLine(tc.line)
+		if err != nil {
+			if tc.err != "" {
+				if !strings.HasPrefix(fmt.Sprintf("%v", err), tc.err) {


err.Error(). Although none of the test cases sets tc.err anyway

Done, and added some error testcases.

danwinship · 2016-11-10T18:26:00Z

pkg/util/iptables/iptables_test.go

+		result, err := ParseTableAddRules(tc.table, tc.filterChains, tc.filterChainPrefixes, tc.data)
+		if err != nil {
+			if tc.err != "" {
+				if !strings.HasPrefix(fmt.Sprintf("%v", err), tc.err) {


err.Error()

danwinship · 2016-11-10T18:58:55Z

pkg/proxy/iptables/proxier.go

-		// Revert new local ports.
-		revertPorts(replacementPortsMap, proxier.portsMap)
-		return
+		glog.Warningf("failed to parse iptables filter rules: %v", err)


With just a warning, I fear that this is just going to get broken. Someone will add a new rule that the parsing code doesn't know how to parse, no one will notice the warning message, and we'll ship a release where all of this parsing and comparing ends up just being an expensive no-op. OTOH, the code is complicated enough that I'm not comfortable saying we should panic if parsing fails...

Maybe utiliptables.EnsureRule() should return an error if it finds itself about to add a rule that it wouldn't later be able to parse? (Though in that case we'd want to validate module names too, to ensure that there aren't going to be any new "fixups for default module options" needed.)

The proxy testcases should pick up changes to the ruleset though, so it should fail pretty early if syncProxyRules() gets changed. And it's at least designed to fail safe, as in it shouldn't change existing behavior if things don't work, it'll just always sync and log a message.

Ideally instead of adding textual rules we'd have a more structured iptables module that would use the vishvananda/netlink to read/write stuff and thus be able to more directly compare instead of screen-scraping, but that's a ton of work...

If you really want it I can try the EnsureRule thing and add hostports, userspace proxy, and other testcases too.

Oh, it didn't occur to me that the parsing code didn't deal with hostports, etc, already...

Maybe just add warning comments to all the relevant bits of the iptables proxy then where people might change things in ways that would cause breakage

@danwinship the proxy code is the only one so far that uses ParseTableRules(), and while the others could use this too, they don't get called anywhere near as often so they aren't a hotpath like the proxy stuff is. I'll definitely add comments.

dcbw · 2016-11-10T22:38:49Z

@danwinship PTAL, thanks

dims · 2016-11-13T00:26:04Z

@thockin @wojtek-t @danwinship @saad-ali : seems a bit big change at the tail end of v1.5. right?

saad-ali · 2016-11-14T01:08:00Z

@thockin @wojtek-t @danwinship @saad-ali : seems a bit big change at the tail end of v1.5. right?

Agreed, we are well into the code freeze for 1.5. Is this a bug fix? @kubernetes/sig-network for more context. Removing 1.5 milestone until we have verfication.

We'll need this in the iptables proxy code to figure out when its pointless to call iptables-restore since things haven't changed.

iptables-restore is a very heavy operation and depending on the kernel, the CPU, and the number of rules to restore, can take a very long time (~500ms or more). i7-5600U @ 2.6GHz: 700ms for 1000 services (2+4 cores+threads, kernel 4.6.6) i7-4790 @ 3.6GHz: 270ms for 1000 services (4+8 cores+threads, kernel 4.6.7) Other parts of kubernetes (eg kubenet) might be running iptables-restore too, leading to some pileup as each iptables-restore waits for others to complete. Related: kubernetes#26637 Related: kubernetes#33693 Related: kubernetes#35334

Go assumes int types for numerics that aren't explicitly typed, leading to: "constant 2147483648 overflows int"

k8s-ci-robot · 2016-12-01T19:18:49Z

Jenkins GCI GCE e2e failed for commit 12fa2af. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

dcbw · 2016-12-01T19:45:45Z

@k8s-bot gci gce e2e test this issue #37361

jeremyeder · 2016-12-14T12:50:26Z

To give you a sense of what we're seeing wrt iptables save/restore churn, here is a graph showing CPU utilization across a variety of system roles in a cluster. Avg in the 2+ core range for iptables rule validation.

This cluster has 1000 nodes and 14,000 pods. At scales far lower than this, you can find iptables consuming a core or so on every node in the cluster at any given time. For RH'rs reading, if you'd like access please ping me internally.

rantoniuk · 2016-12-14T12:57:16Z

@jeremyeder out of curiosity, how many services do you have in the cluster?

jeremyeder · 2016-12-14T13:03:21Z

Approximately 600 services.

widgetpl · 2016-12-16T07:23:09Z

@jeremyeder do you have any network issues like packet drops etc. couse we have faced such issues when we hit to around 500 services.

jeremyeder · 2016-12-16T20:45:22Z

@widgetpl Just confirmed, no drops or errors on any of the interfaces.

k8s-github-robot · 2017-01-12T07:19:30Z

@dcbw PR needs rebase

k8s-github-robot · 2017-01-24T00:13:14Z

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS

We suggest the following people:
cc @brendandburns
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-02-17T11:09:34Z

This PR hasn't been active in 62 days. It will be closed in 27 days (Mar 16, 2017).

cc @danwinship @dcbw @dchen1107 @smarterclayton @thockin @wojtek-t

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

dcbw · 2017-02-17T16:50:26Z

Going to close this one too, as the core issue may have already been addressed by other changes like #41022 and #38996

googlebot added the cla: yes label Nov 1, 2016

k8s-github-robot assigned lavalamp Nov 1, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Nov 1, 2016

dcbw force-pushed the proxy-rules branch from bdd5c98 to a46a313 Compare November 1, 2016 22:23

danwinship reviewed Nov 2, 2016

View reviewed changes

timothysc added area/kube-proxy priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 2, 2016

timothysc added this to the v1.5 milestone Nov 2, 2016

timothysc assigned danwinship, thockin and wojtek-t and unassigned lavalamp Nov 3, 2016

timothysc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 3, 2016

timothysc added the sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. label Nov 4, 2016

dcbw force-pushed the proxy-rules branch from a46a313 to 0d1d925 Compare November 9, 2016 20:20

danwinship suggested changes Nov 10, 2016

View reviewed changes

dcbw force-pushed the proxy-rules branch from 0d1d925 to 47e7de6 Compare November 10, 2016 22:38

saad-ali removed this from the v1.5 milestone Nov 14, 2016

timothysc mentioned this pull request Dec 1, 2016

[kube-proxy] Fix for non-blocking updates during min-sync-period #37726

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2016

dcbw added 4 commits December 1, 2016 12:06

util/iptables: add simple iptables rule parser and testcases

512def2

We'll need this in the iptables proxy code to figure out when its pointless to call iptables-restore since things haven't changed.

proxy/iptables: ensure consistent order of iptables rules

1429a72

proxy/iptables: fix 31 bit masquerade marks

12fa2af

Go assumes int types for numerics that aren't explicitly typed, leading to: "constant 2147483648 overflows int"

dcbw force-pushed the proxy-rules branch from 3e62cfa to 12fa2af Compare December 1, 2016 18:11

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2016

dcbw mentioned this pull request Dec 20, 2016

proxy/iptables: don't sync proxy rules if services map didn't change #38996

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2017

k8s-github-robot assigned smarterclayton and unassigned danwinship, thockin and wojtek-t Jan 30, 2017

apelisse assigned danwinship, thockin and wojtek-t Jan 31, 2017

k8s-github-robot assigned dchen1107 Jan 31, 2017

dcbw closed this Feb 17, 2017

proxy/iptables: don't call iptables-restore when we don't need to #36006

proxy/iptables: don't call iptables-restore when we don't need to #36006

Conversation

dcbw commented Nov 1, 2016 • edited Loading

dcbw commented Nov 1, 2016 • edited Loading

dcbw commented Nov 1, 2016

dcbw commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Nov 3, 2016

dcbw commented Nov 9, 2016

dcbw commented Nov 9, 2016

dcbw commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Nov 10, 2016

dims commented Nov 13, 2016

saad-ali commented Nov 14, 2016

k8s-ci-robot commented Dec 1, 2016

dcbw commented Dec 1, 2016

jeremyeder commented Dec 14, 2016

rantoniuk commented Dec 14, 2016

jeremyeder commented Dec 14, 2016

widgetpl commented Dec 16, 2016

jeremyeder commented Dec 16, 2016

k8s-github-robot commented Jan 12, 2017

k8s-github-robot commented Jan 24, 2017

k8s-github-robot commented Feb 17, 2017

dcbw commented Feb 17, 2017

dcbw commented Nov 1, 2016 •

edited

Loading

dcbw commented Nov 1, 2016 •

edited

Loading