Grab the iptables lock before running iptables-restore. #1491

fasaxc · 2017-06-27T15:43:58Z

Description

To avoid conflicts with kube-proxy (and others), take the iptables lock before running iptables-restore.

See #1470

Todos

Unit tests (full coverage)
Integration tests (delete as appropriate) Done
Double check license requirements

Release Note

Felix now acquires the iptables lock while manipulating iptables.  This prevents conflicts 
with other applications, such as kube-proxy (as long as they also honor the lock).  
Upgrade note: to be effective if Felix is running in a container, this feature requires the 
directory containing the iptables lock file, `/run`, to be mounted into the container.

fasaxc · 2017-07-11T15:31:07Z

retest this please

nelljerram

Just a few nits.

nelljerram · 2017-07-13T12:42:54Z

config/param_types.go

+		err = p.parseFailed(raw, "invalid float")
+		return
+	}
+	result = time.Duration(millis * float64(time.Millisecond))


Is this cast needed? I'm curious to understand, if so.

Yes, it is. I want to allow the input to be a float, so you could say 1.2 (milliseconds). Go's casting rules are very strict so you can't do float * duration (which is really an int64 number of nanoseconds) without a cast. Then I need to cast back to time.Duration at the end.

Casting millis to time.Duration first then multiplying doesn't work because that would lose precision.

That makes sense - thanks.

nelljerram · 2017-07-13T12:43:36Z

intdataplane/int_dataplane.go

-	forceDataplaneRefresh bool
-	cleanupPending        bool
-	doneFirstApply        bool
+	// dataplaneNeedsSync is set if the dataplane is dirty in some way, I.e. we need to


lowercase "i.e."?

nelljerram · 2017-07-13T12:45:32Z

intdataplane/int_dataplane.go

+	iptablesNATOptions.ExtraCleanupRegexPattern = rules.HistoricInsertedNATRuleRegex
+
+	// Create the shared iptables lock.  This allows us to block other processes from
+	// manipulating iptables but it still allows us to do our work in parallel.


Not sure what the "... in parallel" clause means here.

nelljerram · 2017-07-13T13:31:03Z

iptables/lock.go

+	})
+	countLockTimeouts = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "felix_iptables_lock_timeouts",
+		Help: "Number of lock timeouts while waiting for hte iptables lock(s).",


nelljerram · 2017-07-13T13:44:58Z

iptables/lock.go

+			return nil, Err14LockTimeout
+		}
+		time.Sleep(probeInterval)
+		countLockRetries.Inc()


Feels like it would be useful for the 14 and 16 metrics to be distinct - perhaps with labels. In a failing system, it's probably going to be one of them in particular that is problematic.

nelljerram · 2017-07-13T13:53:46Z

config/param_types.go

+		err = p.parseFailed(raw, "invalid float")
+		return
+	}
+	result = time.Duration(millis * float64(time.Millisecond))


Is this cast needed? I'm curious to understand, if so.

nelljerram · 2017-07-13T13:55:29Z

intdataplane/int_dataplane.go

+	iptablesNATOptions.ExtraCleanupRegexPattern = rules.HistoricInsertedNATRuleRegex
+
+	// Create the shared iptables lock.  This allows us to block other processes from
+	// manipulating iptables but it still allows us to do our work in parallel.


I didn't understand the last clause on first reading this. Perhaps "but it still allows our own goroutines to modify iptables in parallel - which is OK because they are operating on different tables"?

nelljerram

Couple of queries about the new testing.

nelljerram · 2017-07-14T10:02:22Z

Makefile

@@ -324,6 +330,16 @@ ut combined.coverprofile: vendor/.up-to-date $(FELIX_GO_FILES)
 	@echo Running Go UTs.
 	$(DOCKER_GO_BUILD) ./utils/run-coverage

+bin/fv.test: vendor/.up-to-date $(FELIX_GO_FILES)
+	$(DOCKER_GO_BUILD) go test ./fv -c --tags fvtests -o bin/fv.test


When I added the k8sfv directory, I needed to exclude it from some of the infrastructure for running UTs:

./utils/run-coverage:11: grep -vE '/vendor/|\./proto/|.glide|/k8sfv/' | \ ./utils/run-coverage:24:ginkgo -cover -covermode=count -coverpkg=${go_dirs} -r -skipPackage k8sfv ./Makefile:352: $(DOCKER_GO_BUILD) ginkgo -r -skipPackage k8sfv $(GINKGO_OPTIONS) ./Makefile:357: $(DOCKER_GO_BUILD) ginkgo watch -r -skipPackage k8sfv $(GINKGO_OPTIONS)

Do you need to do that for fv as well?

I saw a neat trick in that Cloudflare article: using a build tag to identify which tests to build/run and I went for that approach. WDYT?

nelljerram · 2017-07-14T10:14:16Z

fv/iptables_lock_test.go

+	BeforeEach(func() {
+		containerName = fmt.Sprintf("felix-fv-%d-%d", os.Getpid(), containerIdx)
+		containerIdx++
+		felixDir, err := os.Getwd()


Do these tests need felixDir to be correct? I would expect Getwd() here to give <reporoot>/fv, not <reporoot> - and I'm not sure that is what you intend with the name felixDir.

Hmm, it works from the makefile due to the way we run it. Maybe I should be tolerant to that though, so that ginkgo watch does the right thing.

- Copy and adapt kube-proxy's iptables lock implementation. - Implement a shared iptables lock that allows for writes to different tables (which are orthogonal). - Add a test executable that grabs the lock for a specified length of time along with FV tests that test correct blocking of iptables -w. These are the Felix changes required for #1470.

caseydavenport · 2017-07-25T00:33:48Z

@fasaxc I think we've got duplicated release note information coming from this PR and projectcalico/calico#935. Can we remove the label from this one? (Or maybe we just leave it and say that's OK?)

fasaxc · 2017-07-25T08:23:27Z

Sure.

Grab the iptables lock before running iptables-restore.

fasaxc force-pushed the iptables-lock branch 2 times, most recently from c7990ce to 5a3e989 Compare June 28, 2017 09:55

fasaxc force-pushed the iptables-lock branch 2 times, most recently from 3dd2673 to 48a743d Compare July 11, 2017 14:35

fasaxc mentioned this pull request Jul 11, 2017

[WiP] Update docs and manifests for adding iptables lock support to Felix projectcalico/calico#902

Closed

3 tasks

fasaxc force-pushed the iptables-lock branch from 48a743d to c14aed9 Compare July 11, 2017 16:11

fasaxc requested a review from nelljerram July 11, 2017 16:28

fasaxc assigned nelljerram Jul 11, 2017

fasaxc added this to the Calico 2.4.0 milestone Jul 11, 2017

fasaxc added area/felix kind/enhancement priority/P2 labels Jul 11, 2017

caseydavenport added the release-note-required label Jul 13, 2017

nelljerram requested changes Jul 13, 2017

View reviewed changes

fasaxc force-pushed the iptables-lock branch from d2bafd5 to 62b7638 Compare July 13, 2017 15:49

nelljerram reviewed Jul 14, 2017

View reviewed changes

fasaxc force-pushed the iptables-lock branch from a4c038b to 82063bc Compare July 14, 2017 13:37

nelljerram approved these changes Jul 14, 2017

View reviewed changes

fasaxc merged commit ebfc5f5 into projectcalico:master Jul 14, 2017

fasaxc deleted the iptables-lock branch July 14, 2017 17:09

fasaxc removed the release-note-required label Jul 25, 2017

song-jiang pushed a commit to song-jiang/felix that referenced this pull request Aug 30, 2017

Merge pull request projectcalico#1491 from fasaxc/iptables-lock

cd583ea

Grab the iptables lock before running iptables-restore.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grab the iptables lock before running iptables-restore. #1491

Grab the iptables lock before running iptables-restore. #1491

fasaxc commented Jun 27, 2017 •

edited

Loading

fasaxc commented Jul 11, 2017

nelljerram left a comment

nelljerram Jul 13, 2017

fasaxc Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram Jul 13, 2017

nelljerram left a comment

nelljerram Jul 14, 2017

fasaxc Jul 14, 2017

nelljerram Jul 14, 2017

fasaxc Jul 14, 2017

caseydavenport commented Jul 25, 2017 •

edited

Loading

fasaxc commented Jul 25, 2017

Grab the iptables lock before running iptables-restore. #1491

Grab the iptables lock before running iptables-restore. #1491

Conversation

fasaxc commented Jun 27, 2017 • edited Loading

Description

Todos

Release Note

fasaxc commented Jul 11, 2017

nelljerram left a 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

nelljerram left a 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

caseydavenport commented Jul 25, 2017 • edited Loading

fasaxc commented Jul 25, 2017

fasaxc commented Jun 27, 2017 •

edited

Loading

caseydavenport commented Jul 25, 2017 •

edited

Loading