daemon: Enable device auto detection for host-fw when BPF NodePort is disabled #12050

brb · 2020-06-12T13:17:23Z

This PR:

Restructures initKubeProxyReplacementOptions(), and splits it into two helpers.
Enables device auto detection for host-fw.
Fixes direct routing iface in the DSR check.

Reviewable per commit.

brb · 2020-06-12T13:17:41Z

test-me-please

pchaigno · 2020-06-12T14:13:56Z

@brb Smoke test failed 😉 (and took 40min to report because of #11976).

coveralls · 2020-06-12T14:16:15Z

Coverage increased (+0.02%) to 37.061% when pulling f536867 on pr/brb/autodev-hostfw into f3b474a on master.

daemon/cmd/daemon_main.go

brb · 2020-06-12T15:53:20Z

test-me-please

daemon/cmd/daemon_main.go

brb · 2020-06-15T05:54:10Z

test-me-please

pchaigno · 2020-06-15T06:01:47Z

@brb Smoke test failed again :-)

brb · 2020-06-15T10:22:49Z

@brb Smoke test failed again :-)

It was failing due to missing operator image. I've rebased against the master, let's see whether it has been fixed.

Just for DRY. Also, make the log messages in initKubeProxyReplacementOptions() more consistent and less ambiguous. Signed-off-by: Martynas Pumputis <m@lambda.lt>

Move all the checks which do not depend on option.Config.{DirectRoutingDevice,Devices} above the device auto detection. Signed-off-by: Martynas Pumputis <m@lambda.lt>

We no longer consider that the first device among option.Config.Devices is going to be used for direct routing. Signed-off-by: Martynas Pumputis <m@lambda.lt>

The second helper function is supposed to call the device detection, and the third to finish the initialization. It's required, because the host-fw also requires the auto detection, and extending the former helper to consider host-fw is a bit brittle, as any premature return would prevent from the auto-detection for the host-fw. Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb · 2020-06-15T10:59:16Z

test-me-please

nebril · 2020-06-15T12:43:06Z

daemon/cmd/daemon_main.go

+
+// detectDevicesForNodePortAndHostFirewall tries to detect bpf_host devices
+// (if needed).
+func detectDevicesForNodePortAndHostFirewall(strict bool) {


This is probably out of scope of this PR, but do you think it would be possible for us to call all these init functions with config as an argument instead of using global one explicitely? This would make it possible to unit test them to make sure that we don't mess things up, or is there anything aside from config manipulation that makes it unfeasible?

Good idea! Added to my TODO list.

Agree, would be nice to get some test coverage on this entire part of the code (i.e. dependent option handling)

Enable the device auto-detection when BPF NodePort is disabled, but host-fw is enabled. Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb · 2020-06-15T13:02:47Z

test-me-please

tklauser

LGTM with two small nits. Feel free to ignore or defer to follow-up.

tklauser · 2020-06-15T15:22:54Z

daemon/cmd/daemon_main.go

+			len(option.Config.Devices) == 1
+}
+
+// disableNodePort disables BPF NodePort and friends who are dependent from


Nit:

Suggested change

// disableNodePort disables BPF NodePort and friends who are dependent from

// disableNodePort disables BPF NodePort and friends which are dependent on

tklauser · 2020-06-15T15:31:08Z

daemon/cmd/daemon_main.go

-						iface, iface)
-				}
+				disableNodePort()
+				log.Warn(fmt.Sprintf("%s. Disabling BPF NodePort.", err))


Consider using log.WithError(err) here?

Suggested change

log.Warn(fmt.Sprintf("%s. Disabling BPF NodePort.", err))

log.WithError(err).Warn("Disabling BPF NodePort.")

brb · 2020-06-15T15:36:51Z

LGTM with two small nits. Feel free to ignore or defer to follow-up.

@tklauser I've added those two to my TODO list (CI passed, don't want to re-run).

brb added pending-review area/daemon Impacts operation of the Cilium daemon. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/misc This PR makes changes that have no direct user impact. labels Jun 12, 2020

brb requested review from pchaigno and a team June 12, 2020 13:17

maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 12, 2020

brb removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 12, 2020

pchaigno added the needs-backport/1.8 label Jun 12, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 12, 2020

pchaigno requested changes Jun 12, 2020

View reviewed changes

daemon/cmd/daemon_main.go Show resolved Hide resolved

daemon/cmd/daemon_main.go Show resolved Hide resolved

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved

brb force-pushed the pr/brb/autodev-hostfw branch from c0ea924 to 772c580 Compare June 12, 2020 15:53

brb requested a review from pchaigno June 12, 2020 15:53

pchaigno reviewed Jun 12, 2020

View reviewed changes

daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved

pchaigno approved these changes Jun 12, 2020

View reviewed changes

brb force-pushed the pr/brb/autodev-hostfw branch from 772c580 to 37531ba Compare June 15, 2020 10:22

brb added 3 commits June 15, 2020 12:22

daemon: Move disabling BPF NodePort to separate helper

3c33eef

Just for DRY. Also, make the log messages in initKubeProxyReplacementOptions() more consistent and less ambiguous. Signed-off-by: Martynas Pumputis <m@lambda.lt>

daemon: Regroup initKubeProxyReplacementOptions checks

4ef5497

Move all the checks which do not depend on option.Config.{DirectRoutingDevice,Devices} above the device auto detection. Signed-off-by: Martynas Pumputis <m@lambda.lt>

daemon: Fix DR device into DSR checks

c0799e4

We no longer consider that the first device among option.Config.Devices is going to be used for direct routing. Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb force-pushed the pr/brb/autodev-hostfw branch 3 times, most recently from d44860e to 239f0c1 Compare June 15, 2020 10:56

nebril approved these changes Jun 15, 2020

View reviewed changes

brb added priority/release-blocker and removed priority/release-blocker labels Jun 15, 2020

daemon: Enable dev auto-detect for host-fw

f536867

Enable the device auto-detection when BPF NodePort is disabled, but host-fw is enabled. Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb force-pushed the pr/brb/autodev-hostfw branch from 239f0c1 to f536867 Compare June 15, 2020 13:02

brb requested a review from a team as a code owner June 15, 2020 13:02

tgraf removed the priority/release-blocker label Jun 15, 2020

brb requested a review from pchaigno June 15, 2020 13:18

pchaigno approved these changes Jun 15, 2020

View reviewed changes

tklauser approved these changes Jun 15, 2020

View reviewed changes

maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jun 15, 2020

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 16, 2020

tgraf merged commit e4bd54c into master Jun 16, 2020

1.8.0 automation moved this from In progress to Merged Jun 16, 2020

tgraf deleted the pr/brb/autodev-hostfw branch June 16, 2020 10:56

borkmann mentioned this pull request Jun 16, 2020

testing: final conversion 4.19 CI to kube-proxy free #12045

Merged

borkmann added backport-pending/1.8 and removed needs-backport/1.8 labels Jun 16, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 16, 2020

borkmann mentioned this pull request Jun 16, 2020

v1.8 backport 2020-06-16 #12103

Merged

borkmann added backport-done/1.8 and removed backport-pending/1.8 labels Jun 16, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Enable device auto detection for host-fw when BPF NodePort is disabled #12050

daemon: Enable device auto detection for host-fw when BPF NodePort is disabled #12050

brb commented Jun 12, 2020

brb commented Jun 12, 2020

pchaigno commented Jun 12, 2020

coveralls commented Jun 12, 2020 •

edited

brb commented Jun 12, 2020

brb commented Jun 15, 2020

pchaigno commented Jun 15, 2020

brb commented Jun 15, 2020

brb commented Jun 15, 2020

nebril Jun 15, 2020

brb Jun 15, 2020

tklauser Jun 15, 2020

brb commented Jun 15, 2020

tklauser left a comment

tklauser Jun 15, 2020

tklauser Jun 15, 2020

brb commented Jun 15, 2020

	// disableNodePort disables BPF NodePort and friends who are dependent from
	// disableNodePort disables BPF NodePort and friends which are dependent on

	log.Warn(fmt.Sprintf("%s. Disabling BPF NodePort.", err))
	log.WithError(err).Warn("Disabling BPF NodePort.")

daemon: Enable device auto detection for host-fw when BPF NodePort is disabled #12050

daemon: Enable device auto detection for host-fw when BPF NodePort is disabled #12050

Conversation

brb commented Jun 12, 2020

brb commented Jun 12, 2020

pchaigno commented Jun 12, 2020

coveralls commented Jun 12, 2020 • edited

brb commented Jun 12, 2020

brb commented Jun 15, 2020

pchaigno commented Jun 15, 2020

brb commented Jun 15, 2020

brb commented Jun 15, 2020

nebril Jun 15, 2020

Choose a reason for hiding this comment

brb Jun 15, 2020

Choose a reason for hiding this comment

tklauser Jun 15, 2020

Choose a reason for hiding this comment

brb commented Jun 15, 2020

tklauser left a comment

Choose a reason for hiding this comment

tklauser Jun 15, 2020

Choose a reason for hiding this comment

tklauser Jun 15, 2020

Choose a reason for hiding this comment

brb commented Jun 15, 2020

coveralls commented Jun 12, 2020 •

edited