daemon: Implement route-based device detection #17219

joamaki · 2021-08-23T15:14:44Z

This reimplements the device detection logic to use the route
information rather than detecting devices based on k8s nodeIP and
the device with the default route.

The devices are discovered by finding all devices mentioned in global
unicast routes and filtering out cilium-managed devices by prefix.

Motivation for this change is to handle cases like #15960 more reliably
and not have the user specify devices manually. This also prepares for
later work to detect devices at runtime to allow e.g. use of KPR with ENI,
which adds devices dynamically.

The reason for introducing DeviceManager is to later add support for
dynamically reconfiguring the datapath for devices added at runtime.
Also the detected devices are now also used for host firewall and
bandwidth manager so makes sense for this logic to be moved out.

Fixes: #15960

Detect devices from global unicast routes in addition to only
looking for the device with the Kubernetes Node IP and the one with
default route.  This expands the set of devices used for kube-proxy
replacement, host firewall and bandwidth manager and should reduce
the need to specify devices manually.

joamaki · 2021-08-23T15:17:44Z

daemon/cmd/devices.go

+
+// updateDevicesFromRoutes processes a batch of routes and updates the set of
+// devices. Returns true if devices changed.
+func (dm *DeviceManager) updateDevicesFromRoutes(routes []netlink.Route) bool {


A follow-up PR will reuse this logic to detect devices at runtime and reload the datapath.

joamaki · 2021-08-23T15:20:32Z

daemon/cmd/devices.go

+	return nil
+}
+
+func (dm *DeviceManager) GetDevices() []string {


In a follow-up with runtime detection of devices the plan is to stop using option.Config.Devices throughout and rather call into device manager to ask for the list of devices as it may mutate over time. Hence I already have the state for the devices and this function here.

See #17187 for the work in progress for the runtime detection and reasoning behind the structure of this file.

joamaki · 2021-08-23T15:21:50Z

daemon/cmd/daemon.go

@@ -112,6 +112,8 @@ type Daemon struct {
 	monitorAgent *monitoragent.Agent
 	ciliumHealth *health.CiliumHealth

+	deviceManager *DeviceManager


A more suitable place for DeviceManager might be in pkg/datapath, but I'd like to leave it here for now.

daemon/cmd/devices.go

joamaki · 2021-08-23T15:37:42Z

test-me-please

daemon/cmd/devices.go

joamaki · 2021-08-30T07:56:17Z

test-me-please

daemon/cmd/devices.go

joamaki · 2021-09-01T16:28:02Z

test-me-please

joamaki · 2021-09-03T07:21:45Z

test-runtime

joamaki · 2021-09-03T08:56:34Z

test-gke

joamaki · 2021-09-06T09:52:26Z

test-runtime

daemon/cmd/devices_test.go

joamaki · 2021-09-21T12:45:53Z

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDemosTest Tests Star Wars Demo

Failure Output

FAIL: DNS entry is not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

joamaki · 2021-09-22T14:05:11Z

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sBookInfoDemoTest Bookinfo Demo Tests bookinfo demo

Failure Output

FAIL: DNS entry is not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

joamaki · 2021-09-23T11:39:58Z

test-1.16-netnext

joamaki · 2021-09-27T14:12:25Z

@pchaigno PTAL

pchaigno

Could you break commit daemon: Address review comments on device detection and merge it in the respective commits it fixes/improves? Having it in a separate commit hinders reviewing and doesn't make sense once merged.

Ah I was just trying to make reviewing easier. I'll squash.

I didn't mean you should squash everything; that doesn't make reviews any easier ;-)

If you're looking for guidance on how to split commits in pull requests, maybe a good starting point is https://kernelnewbies.org/PatchPhilosophy#How_to_break_up_changes. For this specific pull request, a good example is the code you moved from daemon/cmd/kube_proxy_replacement{,_test}.go to a new file; that move should be in its own commit, before the rest. It would help reviewers because we wouldn't have to re-review existing code that was just moved around.

daemon/cmd/daemon.go

daemon/cmd/devices.go

This reimplements the device detection logic to use the route information rather than detecting devices based on k8s nodeIP and the device with the default route. The devices are discovered by finding all devices mentioned in global unicast routes and filtering out cilium-managed devices by prefix. The reason for introducing DeviceManager is to later add support for dynamically reconfiguring the datapath for devices added at runtime. Also the detected devices are now also used for host firewall and bandwidth manager so makes sense for this logic to be moved out. Fixes: cilium#15960 Signed-off-by: Jussi Maki <jussi@isovalent.com>

Since IPsec is not yet using the detected devices, disable detection if only IPsec is enabled. This fixes a failure in IPsec L7 tests. Root cause still unknown why detecting the devices causes issues. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2021-09-30T09:35:10Z

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running

Failure Output

FAIL: DNS entry is not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

joamaki · 2021-10-01T09:23:08Z

test-1.16-netnext

pchaigno · 2021-10-01T13:36:32Z

I think the two tests failing in k8s-1.16-kernel-netnext are new, so likely a case of #15474. You can confirm by checking you don't have the related changes (the changes those tests test) in your branch. Happy to help on Slack.

If that's confirmed, then I think we're good to go. The remaining cilium/agent review is for changes under daemon/ but I'm familiar with the specific code you touched and I approved already.

joamaki · 2021-10-01T15:05:26Z

I think the two tests failing in k8s-1.16-kernel-netnext are new, so likely a case of #15474. You can confirm by checking you don't have the related changes (the changes those tests test) in your branch. Happy to help on Slack.

If that's confirmed, then I think we're good to go. The remaining cilium/agent review is for changes under daemon/ but I'm familiar with the specific code you touched and I approved already.

cilium % git diff upstream/master -- test/k8sT/Services.go|grep "Checks conn"
-       }, "Checks connectivity when skipping socket lb in pod ns", func() {

Yep, it's a new test.

pchaigno · 2021-10-01T15:21:22Z

Yep, it's a new test.

Yep, but are the changes added with that test included in your branch?

joamaki requested review from brb and borkmann August 23, 2021 15:14

maintainer-s-little-helper bot assigned borkmann and brb Aug 23, 2021

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 23, 2021

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from 780dbc0 to 19a42da Compare August 23, 2021 15:16

joamaki commented Aug 23, 2021

View reviewed changes

joamaki mentioned this pull request Aug 23, 2021

daemon: New route-based device detection and reconfiguration #17187

Closed

4 tasks

joamaki commented Aug 23, 2021

View reviewed changes

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

borkmann approved these changes Aug 23, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned borkmann Aug 23, 2021

borkmann reviewed Aug 23, 2021

View reviewed changes

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

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from 19a42da to b6bfcd9 Compare August 30, 2021 07:47

joamaki added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 30, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 30, 2021

joamaki force-pushed the pr/joamaki/route-based-device-detection branch 2 times, most recently from 6bd1d94 to c75e3f1 Compare August 30, 2021 14:02

joamaki commented Aug 30, 2021

View reviewed changes

daemon/cmd/devices.go Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from c75e3f1 to cd0c249 Compare September 1, 2021 15:23

joestringer mentioned this pull request Sep 1, 2021

CI: Smoke test: Timed out waiting for the condition on pods #12279

Closed

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from cd0c249 to ee8436a Compare September 2, 2021 16:59

joamaki force-pushed the pr/joamaki/route-based-device-detection branch 2 times, most recently from 231790e to 23bc0f4 Compare September 6, 2021 09:02

joamaki requested a review from a team as a code owner September 21, 2021 10:24

joamaki requested a review from qmonnet September 21, 2021 10:24

maintainer-s-little-helper bot assigned qmonnet Sep 21, 2021

qmonnet approved these changes Sep 21, 2021

View reviewed changes

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

maintainer-s-little-helper bot unassigned qmonnet Sep 21, 2021

joamaki force-pushed the pr/joamaki/route-based-device-detection branch 2 times, most recently from e235b4b to 8dadc1a Compare September 21, 2021 11:26

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from 8dadc1a to 4bb3944 Compare September 22, 2021 09:02

maintainer-s-little-helper bot requested review from brb and pchaigno September 23, 2021 14:48

brb approved these changes Sep 27, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned brb Sep 27, 2021

pchaigno requested changes Sep 29, 2021

View reviewed changes

joamaki added 2 commits September 30, 2021 10:48

joamaki force-pushed the pr/joamaki/route-based-device-detection branch from 4bb3944 to 78faa05 Compare September 30, 2021 08:52

pchaigno approved these changes Sep 30, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned pchaigno Sep 30, 2021

joamaki added the area/daemon Impacts operation of the Cilium daemon. label Oct 1, 2021

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 1, 2021

borkmann merged commit 74fbca2 into cilium:master Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Implement route-based device detection #17219

daemon: Implement route-based device detection #17219

joamaki commented Aug 23, 2021 •

edited

joamaki Aug 23, 2021

joamaki Aug 23, 2021

joamaki Aug 23, 2021 •

edited

joamaki commented Aug 23, 2021

joamaki commented Aug 30, 2021

joamaki commented Sep 1, 2021

joamaki commented Sep 3, 2021

joamaki commented Sep 3, 2021

joamaki commented Sep 6, 2021

joamaki commented Sep 21, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Sep 22, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Sep 23, 2021

joamaki commented Sep 27, 2021

pchaigno left a comment

joamaki commented Sep 30, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Oct 1, 2021

pchaigno commented Oct 1, 2021 •

edited

joamaki commented Oct 1, 2021

pchaigno commented Oct 1, 2021

daemon: Implement route-based device detection #17219

daemon: Implement route-based device detection #17219

Conversation

joamaki commented Aug 23, 2021 • edited

joamaki Aug 23, 2021

Choose a reason for hiding this comment

joamaki Aug 23, 2021

Choose a reason for hiding this comment

joamaki Aug 23, 2021 • edited

Choose a reason for hiding this comment

joamaki commented Aug 23, 2021

joamaki commented Aug 30, 2021

joamaki commented Sep 1, 2021

joamaki commented Sep 3, 2021

joamaki commented Sep 3, 2021

joamaki commented Sep 6, 2021

joamaki commented Sep 21, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Sep 22, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Sep 23, 2021

joamaki commented Sep 27, 2021

pchaigno left a comment

Choose a reason for hiding this comment

joamaki commented Sep 30, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented Oct 1, 2021

pchaigno commented Oct 1, 2021 • edited

joamaki commented Oct 1, 2021

pchaigno commented Oct 1, 2021

joamaki commented Aug 23, 2021 •

edited

joamaki Aug 23, 2021 •

edited

joamaki commented Sep 21, 2021 •

edited by maintainer-s-little-helper bot

joamaki commented Sep 22, 2021 •

edited by maintainer-s-little-helper bot

joamaki commented Sep 30, 2021 •

edited by maintainer-s-little-helper bot

pchaigno commented Oct 1, 2021 •

edited