datapath: Devices table and controller #24677

joamaki · 2023-03-31T16:42:39Z

This PR adds the following new tables to pkg/datapath/tables:

// Device is a local network device along with addresses associated with it.
type Device struct {
	Index        int              // positive integer that starts at one, zero is never used
	MTU          int              // maximum transmission unit
	Name         string           // e.g., "en0", "lo0", "eth0.100"
	HardwareAddr net.HardwareAddr // IEEE MAC-48, EUI-48 and EUI-64 form
	Flags        net.Flags        // e.g. net.FlagUp, net.eFlagLoopback, net.FlagMulticast

	Selected    bool            // If true this device can be used by Cilium
	Addrs       []DeviceAddress // Addresses assigned to the device
	RawFlags    uint32          // Raw interface flags
	Type        string          // Device type, e.g. "veth" etc.
	MasterIndex int             // Index of the master device (e.g. bridge or bonding device)
}

var deviceTableSchema = &memdb.TableSchema{
	Name: "devices",
	Indexes: map[string]*memdb.IndexSchema{
		string(statedb.IDIndex): {
			Name:         string(statedb.IDIndex),
			AllowMissing: false,
			Unique:       true,
			Indexer:      &memdb.IntFieldIndex{Field: "Index"},
		},
		string(DeviceNameIndex): {
			Name: string(DeviceNameIndex),
			// Name can be temporarily missing if we create the device
			// from an address update.
			AllowMissing: true,
			Unique:       true,
			Indexer:      &memdb.StringFieldIndex{Field: "Name"},
		},
	},
}

type Route struct {
	Table     int
	LinkIndex int

	Scope uint8
	Dst   netip.Prefix
	Src   netip.Addr
	Gw    netip.Addr
}

var (
	routeTableSchema = &memdb.TableSchema{
		Name: "routes",
		Indexes: map[string]*memdb.IndexSchema{
			"id": {
				Name:         "id",
				AllowMissing: false,
				Unique:       true,
				Indexer: &memdb.CompoundIndex{
					Indexes: []memdb.Indexer{
						&memdb.IntFieldIndex{Field: "Table"},
						&memdb.IntFieldIndex{Field: "LinkIndex"},
						&statedb.NetIPPrefixFieldIndex{Field: "Dst"},
					},
				},
			},
			string(linkIndexIndex): {
				Name:         string(linkIndexIndex),
				AllowMissing: false,
				Unique:       false,
				Indexer:      &memdb.IntFieldIndex{Field: "LinkIndex"},
			}},
	}
)

The tables are populated by the DevicesController by subscribing to links, addresses and routes using netlink.
To avoid large changes to existing code, this PR keeps the existing DeviceManager API and validates the
new implementation using the old existing test suite.

Follow-up PRs will incrementally switch uses of option.Config.GetDevices() to querying and watching the devices table.

pkg/option/config.go

pkg/datapath/linux/devices.go

brb · 2023-04-20T14:31:18Z

pkg/datapath/state/device.go

+
+// NodePortAddr returns the NodePort frontend addresses chosen
+// from this device.
+func (dev *Device) NodePortAddr() NodePortAddr {


Do we actually need this function to be NodePort-specific? I guess in the future we will support multiple NodePort IP addrs per netdev, so maybe we could use a generic IPAddrs() method instead?

To elaborate more - I don't want to leak any LB details (in this case NodePort) into this relatively generic package.

Since this method is about picking the single nodeport frontend for the device I would keep it NodePort-specific. However, you're right it shouldn't be here. I'll move it to a more appropriate place. In the future this would go away completely once service load-balancing is refactored to support multiple frontends per device.

tommyp1ckles · 2023-05-29T23:09:29Z

pkg/datapath/linux/devices_controller.go

+		Namespace:    dc.NetNS,
+		ListExisting: false,
+	}
+	if err := netlink.AddrSubscribeWithOptions(addrUpdates, dc.ctx.Done(), opts); err != nil {


I believe in the past we've seen issues where the netlink can be unreliabe, should we try to reestablish the nl connection/sub if the channel gets closed?

Oh that's a bummer. Would require fair bit of additional complexity to resubscribe as it needs to relist from scratch in order to make sure no update has been missed.

Do you have any more info about the unreliability?

netlink man page has this to say:

Netlink is not a reliable protocol. It tries its best to deliver a message to its destination(s), but may drop messages when an out-of-memory condition or other error occurs. For reliable transfer the sender can request an acknowledgement from the receiver by setting the NLM_F_ACK flag. An acknowledgement is an NLMSG_ERROR packet with the error field set to 0. The application must generate acknowledgements for received messages itself. The kernel tries to send an NLMSG_ERROR message for every failed packet. A user process should follow this convention too. However, reliable transmissions from kernel to user are impossible in any case. The kernel can't send a netlink message if the socket buffer is full: the message will be dropped and the kernel and the user-space process will no longer have the same view of kernel state. It is up to the application to detect when this happens (via the ENOBUFS error returned by [recvmsg(2)](https://man7.org/linux/man-pages/man2/recvmsg.2.html)) and resynchronize.

The go netlink library doesn't do the ACKs so this is indeed is pretty unreliable and may miss updates.
Not sure in what scenarios the netlink socket would get closed, but wouldn't be surprised it can happen
in OOM situations.

I'll need to think a bit how we can make this reliable...

EDIT: Could maybe check sequence numbers of all updates and then restart everything from scratch if there's a missing update or any other error. Though maybe ENOBUFS would be sent to the ErrorCallback when that would happen.

Yeah, correct.

A timeout was added by a contributor to the nl library to solve issues where the socket would hang indefinitely (I don't think it was clear what the actual underlying issue was, potentially just a bug in netlink?).

Could we propose/add the feature ourselves?

The recvfrom hang has been fixed 1 year ago: vishvananda/netlink#793. Unfortunately the netlink library seems to not be maintained well anymore. I'll likely look into actively maintained alternative libraries that would also allow cleanly distinguishing between initial listing and updates.

aojea · 2023-06-14T08:52:15Z

/cc

lmb

Thanks for your changes, it reads easier now.

lmb · 2023-07-10T15:16:21Z

pkg/datapath/linux/devices_controller.go

+		}
+
+		for _, u := range updates {
+			if d == nil {


Is this necessary even though there is an if d == nil at the end of the loop?

Point is to stop processing further updates for this link. I could instead add a break label and do break updateLoop on line 438, but this seems nicer.

EDIT: Ah, but now I'm likely making unfounded assumption here about ifindex not being reused too quickly. Will figure out what I can actually assume and fix the code accordingly. Wonder what fun races this thing has with regards to addresses and routes. Likely the correct way to do this (which is not easy with this netlink library) is to have a single netlink socket over which we get address, link and route updates.

EDIT2: At least locally I'm not seeing any reuse of ifindex. Not quite sure how paranoid this code should be. Thoughts?

pkg/datapath/linux/devices_controller.go

lmb · 2023-07-10T15:18:38Z

pkg/datapath/linux/devices_controller.go

+
+// netlinkFuncs wraps the netlink subscribe functions into a simpler interface to facilitate
+// testing of the error handling paths.
+type netlinkFuncs struct {


Not saying you need to change this, but I'm curious: why a struct of func pointers instead of an interface?

Less boilerplate and makes it easy to override/wrap one of the methods to inject slightly different behavior in tests.

pkg/datapath/tables/route.go

pkg/datapath/linux/devices_controller.go

tommyp1ckles · 2023-07-17T16:17:34Z

pkg/datapath/linux/devices_controller.go

+		case <-ctx.Done():
+			stop()
+			return
+		case <-t.After(restartWaitDuration):


I'm guessing there's no need for a backoff here?

I removed the backoff in the end as it doesn't really bring much value, plus the logic for resetting the backoff would've been a bit involved (when would we even reset it?).

pkg/datapath/tables/route.go

brb · 2023-07-21T08:05:42Z

daemon/cmd/daemon.go

+	var devices []string
+	if d.deviceManager != nil {
+		if _, err := d.deviceManager.Detect(params.Clientset.IsEnabled()); err != nil {
+			if option.Config.AreDevicesRequired() {


It'd be call if in the future each Cell could individually set a bit saying that the device detection is required.

I would get rid of this AreDevicesRequired stuff completely in the future and have each feature decide how to function or not without any devices.

joamaki · 2023-07-24T08:35:22Z

/test

joamaki · 2023-07-26T11:22:08Z

/test

For consistency rename IPIndexer to IPFieldIndex. Add NetIPPrefixFieldIndex to allow indexing netip.Prefix. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Add state tables for storing network devices (along with addresses) and routes. Signed-off-by: Jussi Maki <jussi@isovalent.com>

DevicesController populates the devices and routes tables. To avoid large refactoring of the existing code around option.Config.GetDevices() this opts for an incremental approach: keep the DeviceManager API as-is and reuse its test to validate that the existing functionality does not break. The tests required some slight adaptation: the "DeviceManager" is re-created after each test case to recreate the initial devices table. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2023-08-01T08:12:29Z

/test

tommyp1ckles

ok looks good on my end, thanks for bearing with me 😄

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 Mar 31, 2023

joamaki mentioned this pull request Mar 31, 2023

statedb: An in-memory database #24523

Merged

joamaki commented Mar 31, 2023

View reviewed changes

pkg/option/config.go Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/statedb-devices branch 5 times, most recently from d827e6e to a724660 Compare April 17, 2023 12:22

joamaki commented Apr 18, 2023

View reviewed changes

pkg/datapath/linux/devices.go Show resolved Hide resolved

joamaki mentioned this pull request Apr 18, 2023

daemon: Perform early (partial) local node info initialization #24866

Merged

dylandreimerink approved these changes Apr 19, 2023

View reviewed changes

brb reviewed Apr 20, 2023

View reviewed changes

joamaki self-assigned this May 5, 2023

joamaki mentioned this pull request May 22, 2023

ipsec: Attach to new ENI interfaces as they are added #25565

Closed

tommyp1ckles reviewed May 29, 2023

View reviewed changes

joamaki mentioned this pull request May 30, 2023

ipsec: Reinitialize IPsec for new devices in ENI mode #25744

Merged

joamaki force-pushed the pr/joamaki/statedb-devices branch 2 times, most recently from 93bbfe8 to 55d4285 Compare June 8, 2023 10:33

joamaki force-pushed the pr/joamaki/statedb-devices branch from 55d4285 to f65e6e4 Compare June 14, 2023 09:49

dylandreimerink force-pushed the pr/joamaki/statedb-devices branch 2 times, most recently from ac666b4 to 0519596 Compare June 27, 2023 09:58

joamaki force-pushed the pr/joamaki/statedb-devices branch 3 times, most recently from 2320a59 to f7bc6c5 Compare July 3, 2023 13:52

joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Jul 3, 2023

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 Jul 3, 2023

joamaki force-pushed the pr/joamaki/statedb-devices branch from f7bc6c5 to 34ce5f2 Compare July 3, 2023 14:16

joamaki marked this pull request as ready for review July 4, 2023 08:16

joamaki requested a review from a team as a code owner July 4, 2023 08:16

joamaki requested a review from lmb July 7, 2023 13:56

joamaki mentioned this pull request Jul 10, 2023

egressgw: improve reconciliation for IP routes #26721

Merged

joamaki force-pushed the pr/joamaki/statedb-devices branch 2 times, most recently from 5615c3f to dcc72ca Compare July 10, 2023 12:31

lmb approved these changes Jul 10, 2023

View reviewed changes

tommyp1ckles reviewed Jul 17, 2023

View reviewed changes

brb removed their request for review July 21, 2023 08:04

brb reviewed Jul 21, 2023

View reviewed changes

joamaki force-pushed the pr/joamaki/statedb-devices branch from dcc72ca to 975a93b Compare July 24, 2023 07:51

joamaki force-pushed the pr/joamaki/statedb-devices branch from 975a93b to 35aa8a2 Compare July 26, 2023 08:57

joamaki force-pushed the pr/joamaki/statedb-devices branch from 35aa8a2 to 2889f20 Compare July 27, 2023 08:04

joamaki added 3 commits July 31, 2023 11:31

statedb: Rename IPIndexer to IPFieldIndex, add NetIPPrefixFieldIndex

43c2b27

For consistency rename IPIndexer to IPFieldIndex. Add NetIPPrefixFieldIndex to allow indexing netip.Prefix. Signed-off-by: Jussi Maki <jussi@isovalent.com>

datapath/tables: Add device and route state tables

dea8d34

Add state tables for storing network devices (along with addresses) and routes. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/statedb-devices branch from 2889f20 to 57014fe Compare July 31, 2023 08:32

joamaki requested a review from tommyp1ckles August 1, 2023 08:12

tommyp1ckles approved these changes Aug 2, 2023

View reviewed changes

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 Aug 2, 2023

joamaki merged commit 03ad61b into cilium:main Aug 2, 2023

aojea mentioned this pull request Aug 9, 2023

NodePort Services do not work with new Node addresses #24481

Closed

2 tasks

michaelasp mentioned this pull request Aug 24, 2023

datapath: fix NodePort to remote hostns backend with tunnel config #27323

Merged

sayboras mentioned this pull request Oct 6, 2023

v1.14 Backports 2023-10-06 #28442

Merged

9 tasks

bimmlerd mentioned this pull request Nov 14, 2023

Make sure runtime device detection works with the Bandwidth Manager #29107

Closed

datapath: Devices table and controller #24677

datapath: Devices table and controller #24677

Uh oh!

Conversation

joamaki commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joamaki Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Jun 14, 2023

Uh oh!

lmb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joamaki Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joamaki commented Jul 24, 2023

Uh oh!

joamaki commented Jul 26, 2023

Uh oh!

joamaki commented Aug 1, 2023

Uh oh!

tommyp1ckles left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joamaki commented Mar 31, 2023 •

edited

Loading

joamaki Jun 6, 2023 •

edited

Loading

joamaki Jul 24, 2023 •

edited

Loading