maps: switch maglev to cilium/ebpf package

[jibi]

No description provided.

[aditighag]

Nice! Fyi - you can drop the first commit - #15386 (comment).

[brb]

cc @christarazi (Chris has a WIP to make inner maps of a different size).

[jibi]

Nice! Fyi - you can drop the first commit - #15386 (comment).

Thanks for the headsup @aditighag 🙌
I actually do need v0.4.0 as I'm using the new Unpin() method (but I'll keep an eye on your PR and rebase once that's sorted)

[twpayne]

LGTM, just a couple of nits, feel free to ignore them.

[jibi]

@brb @christarazi while doing another round of tests I realized that whenever we need to access an existing maglev map from the cilium bpf lb maglev {list,get} commands we should just open the pinned map from its FD rather than using (m *ebpf.Map) OpenOrCreate(), which expects a map's spec and tries to create the map if it doesn't exist.

Therefore I added a new function to the ebpf package, OpenMap() (which mimics the OpenMap() function from the bpf package) and updated lbmap.MaybeInitMaglevMaps() to use that.

[christarazi]

LGTM re: the new changes. It should be compatible with #15019, which I'll rebase on top of this PR soon.

[jibi]

test-me-please

[christarazi]

@jibi I finally got around to rebasing my PR (#15019) on top of this and I ran into an issue.

The summary is that when you specify an InnerMap inside a MapSpec, then cilium/ebpf will create / open this inner map automatically. Whereas in the previous code, we created the inner map manually. In this PR, we are still creating the inner map manually, but we're also specifying the InnerMap field for the outer map, which causes cilium/ebpf to create a new inner map, eventually causing mismatching FDs because the original FD was just overwritten. This conflicting FD is the FD of the inner map stored inside the MaglevOuterVal. The consequence of this is that when you update the outer map, you get an "invalid argument" errno returned, obviously because the wrong FD was used.

I've worked around this manually by changing the NewMaglevInnerMap constructor to only return the MapSpec and not to create the inner map. Inside updateMaglevTable, I then create / open the map.

Here's the diff:

diff --git a/pkg/maps/lbmap/maglev.go b/pkg/maps/lbmap/maglev.go
index 286aa078c..8760b94bb 100644
--- a/pkg/maps/lbmap/maglev.go
+++ b/pkg/maps/lbmap/maglev.go
@@ -42,12 +42,9 @@ var (
 
 // InitMaglevMaps inits the ipv4 and/or ipv6 maglev outer and inner maps.
 func InitMaglevMaps(ipv4, ipv6 bool, tableSize uint32) error {
-	dummyInnerMap, dummyInnerMapSpec, err := NewMaglevInnerMap("cilium_lb_maglev_dummy", tableSize)
-	if err != nil {
-		return err
-	}
-	defer dummyInnerMap.Close()
+	dummyInnerMapSpec := NewMaglevInnerMap("cilium_lb_maglev_dummy", tableSize)
 
+	var err error
 	// Always try to delete old maps with the wrong M parameter, otherwise
 	// we may end up in a case where there are 2 maps (one for IPv4 and
 	// one for IPv6), one of which is not used, with 2 different table
@@ -173,12 +170,18 @@ func updateMaglevTable(ipv6 bool, revNATID uint16, backendIDs []uint16, tableSiz
 		innerMapName = MaglevInner6MapName
 	}
 
-	innerMap, _, err := NewMaglevInnerMap(innerMapName, uint32(tableSize))
+	innerMapSpec := NewMaglevInnerMap(innerMapName, uint32(tableSize))
+	m := ebpf.NewMap(innerMapSpec)
+	err := m.OpenOrCreate()
 	if err != nil {
 		return err
 	}
-	defer innerMap.Close()
+	defer m.Close()
 
+	innerMap := &maglevInnerMap{
+		Map:       m,
+		tableSize: uint32(tableSize),
+	}
 	if err := updateMaglevInnerMap(innerMap, backendIDs); err != nil {
 		return err
 	}
diff --git a/pkg/maps/lbmap/maglev_inner_map.go b/pkg/maps/lbmap/maglev_inner_map.go
index 5a9680a72..a497507ce 100644
--- a/pkg/maps/lbmap/maglev_inner_map.go
+++ b/pkg/maps/lbmap/maglev_inner_map.go
@@ -79,8 +79,8 @@ func (m maglevInnerVals) MarshalBinary() ([]byte, error) {
 }
 
 // NewMaglevInnerMap returns a new object representing a maglev inner map.
-func NewMaglevInnerMap(name string, tableSize uint32) (*maglevInnerMap, *ebpf.MapSpec, error) {
-	spec := &ebpf.MapSpec{
+func NewMaglevInnerMap(name string, tableSize uint32) *ebpf.MapSpec {
+	return &ebpf.MapSpec{
 		Name:       name,
 		Type:       ebpf.Array,
 		KeySize:    uint32(unsafe.Sizeof(MaglevInnerKey{})),
@@ -88,16 +88,6 @@ func NewMaglevInnerMap(name string, tableSize uint32) (*maglevInnerMap, *ebpf.Ma
 		MaxEntries: tableSizeToMaxEntries(tableSize),
 		Flags:      unix.BPF_F_INNER_MAP,
 	}
-
-	m := ebpf.NewMap(spec)
-	if err := m.OpenOrCreate(); err != nil {
-		return nil, nil, err
-	}
-
-	return &maglevInnerMap{
-		Map:       m,
-		tableSize: tableSize,
-	}, spec, nil
 }
 
 // MaglevInnerMapFromID returns a new object representing the maglev inner map

[christarazi]

As mentioned offline, it seems that my code is working without the patch provided above for some reason. My best guess is that my local kernel must have gotten into some bad / weird state with regards to the inner map, as that's what I was developing on 🤷.

[jibi]

test-me-please

[jibi]

retest-netnext

[jibi]

Last week I saw K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with vxlan Tests NodePort with Maglev Tests Maglev backend selection failing once (which might be related to the changes) but I haven't been able to reproduce it locally after quite a few test runs, so I think it's fine to merge the PR in the current state. I'll keep an eye on the CI in case the failure happens again

[brb]

🚀