bpf: Migrate `cilium-migrate-map` from C to Go #16917

nathanjsweet · 2021-07-16T23:30:42Z

Currently, initializing bpf programs and migrating maps is done by a C program implementing an ELF parser. This patch introduces a Go version of the program where cilium/ebpf does the heavy lifting instead. The feature is now also exposed as a Go API, making it callable from other parts of the agent. This version also parses and verifies the ELF's BTF section.

A CLI wrapper is temporarily included to make it callable from init.sh while it is translated to Go.

Best reviewed per commit.

bpf: Migrate map migration logic from C to Go

pkg/bpf/migrate/migrate.go

brb · 2021-07-21T12:18:22Z

@nathanjsweet MBOI #16561.

ti-mo

Hey, thanks for taking this on. I think most of the code around renaming map pins can be removed, as the lib has Map.Pin() that was introduced specifically for this renaming use case.

I don't think we should pass in $RETCODE, let's handle any conditionals in the script itself.

Also, we should take #16561 into account, but can be done in a follow-up. Will further the discussion on the issue itself.

cilium/cmd/bpf_map_migrate.go

pkg/bpf/migrate/migrate.go

bpf/init.sh

cilium/cmd/bpf_map_migrate.go

pkg/bpf/migrate/migrate.go

bpf/init.sh

ti-mo · 2021-09-29T21:13:31Z

/test

ti-mo · 2021-10-06T20:09:00Z

/test

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes with L7 policy Tests NodePort with L7 Policy

Failure Output

FAIL: Request from k8s1 to service http://[fd03::611d]:10080 failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

ti-mo · 2021-10-11T11:47:03Z

/test

tklauser

Thanks @ti-mo for walking me through the change. LGTM with one minor comment which can be done in a follow-up PR if needed.

tklauser · 2021-10-11T14:21:23Z

bpf/cilium-map-migrate.c

-#include "elf/libelf.h"
-#include "elf/gelf.h"
-
-#include "bpf/ctx/nobpf.h"


It looks like cilium-map-migrate.c is the only user of the headers in bpf/include/elf and the bpf/ctx/nobpf.h. So I think we can safely remove these as part of this PR as well.

Also, we no longer need to link binaries against libelf so we can remove -lelf from bpf/Makefile here:

cilium/bpf/Makefile

Lines 306 to 307 in 6a722d3

@# Due to gcc bug, -lelf needs to be at the end.

$(QUIET) ${HOST_CC} -Wall -O2 -Wno-format-truncation -I include/ $@.c -lelf -o $@

Thanks, that's great news! 😁 Will tackle in a follow-up. 👍

ti-mo · 2021-10-11T19:33:05Z

/test

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

Click to show.

Test Name

RuntimePrivilegedUnitTests Run Tests

Failure Output

FAIL: Failed to run privileged unit tests

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

Job 'Cilium-PR-K8s-1.19-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

[empty]

Failure Output

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.19-kernel-5.4 so I can create a new GitHub issue to track it.

ti-mo · 2021-10-12T13:54:59Z

/test

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig MonitorAggregation Checks that monitor aggregation restricts notifications

Failure Output

FAIL: unable to retrieve all nodes with 'kubectl get nodes -o json | jq '.items | length'': Exitcode: -1

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

Upgrade the lib to make use of new pinning features. Signed-off-by: Timo Beckers <timo@isovalent.com>

Document the ELF parsing logic to make it easier to grok. Signed-off-by: Timo Beckers <timo@isovalent.com>

This patch uses io.WriteSeeker.Seek() to navigate to each symbol's offset instead of alternating between streaming input from the original ELF and inscribing replacement values. The new approach is slightly more straightforward as no global offset tracking needs to take place. Signed-off-by: Timo Beckers <timo@isovalent.com>

The way we currently perform string substitutions in ELFs no longer holds up with the presence of a BTF blob. The .BTF section has its own string table for representing type metadata, and cilium/ebpf uses this table to perform lookups for building its internal representations of the BTF type graph. This behaviour should not be altered, as it makes sure all ELF symbols are correctly represented in the BTF blob. This patch extends our current approach to also rewrite pieces of the .BTF section, which will be required for as long as we keep using a mix of iproute2 and cilium/ebpf for reading ELFs. Signed-off-by: Timo Beckers <timo@isovalent.com>

Currently initializing bpf programs and migrating maps is done by a C program implementing an ELF parser. This patch introduces a Go version of the program where cilium/ebpf does the heavy lifting instead. The feature is now also exposed as a Go API, making it callable from other parts of the agent. This version also parses and verifies the ELF's BTF section. A CLI wrapper is temporarily included to make it callable from init.sh while it is translated to Go. Signed-off-by: Nate Sweet <nathanjsweet@pm.me> Co-authored-by: Timo Beckers <timo@isovalent.com>

This uses the newly-ported Go logic to execute bpffs map migrations in replaceQdisc and replaceDatapath. Also simplified the control flow a bit to avoid accidentally shadowing err. The revert behaviour has been made more explicit. Signed-off-by: Timo Beckers <timo@isovalent.com>

This has been replaced with a Go-based implementation, so no longer needs to be part of the build/packaging process. The CILIUM_BPF_MNT env has been removed from shell invocations pertaining to bpffs since it was only referenced in cilium-map-migrate. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo · 2021-10-13T08:51:00Z

/test

ti-mo · 2021-10-13T11:51:44Z

/mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9

ti-mo · 2021-10-13T13:29:55Z

Opened #17590 and #17594 for flakes found while working on this PR.

pchaigno

Nice stuff! The pull request is really easy to review thanks to all the comments :-)

I have a couple nits below, which I'll send as a separate PR given I'm a bit late to the merge party.

pkg/bpf/bpffs_migrate.go

pkg/datapath/loader/netlink.go

@tklauser

As flagged by @tklauser in cilium#16917 (comment), these headers are no longer used, and we no longer need to link against libelf. Signed-off-by: Timo Beckers <timo@isovalent.com>

@tklauser

As flagged by @tklauser in #16917 (comment), these headers are no longer used, and we no longer need to link against libelf. Signed-off-by: Timo Beckers <timo@isovalent.com>

nathanjsweet added the release-note/misc This PR makes changes that have no direct user impact. label Jul 16, 2021

nathanjsweet force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from ad37ac7 to 21ffe74 Compare July 19, 2021 22:32

ti-mo reviewed Jul 20, 2021

View reviewed changes

pkg/bpf/migrate/migrate.go Outdated Show resolved Hide resolved

nathanjsweet force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch 3 times, most recently from b99b8e5 to c8fe204 Compare July 23, 2021 20:44

nathanjsweet force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch 2 times, most recently from 2e5dcd4 to f9a7dc6 Compare August 12, 2021 18:19

ti-mo reviewed Aug 19, 2021

View reviewed changes

pchaigno added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. sig/loader Impacts the loading of BPF programs into the kernel. labels Sep 1, 2021

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from f9a7dc6 to 6f0932f Compare September 23, 2021 15:48

nathanjsweet force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from 6f0932f to d06baee Compare September 24, 2021 01:44

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch 3 times, most recently from a154b15 to 80b2b10 Compare September 29, 2021 20:36

ti-mo removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 29, 2021

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch 4 times, most recently from 4b2a1ab to eb711d6 Compare October 6, 2021 19:53

ti-mo marked this pull request as ready for review October 6, 2021 21:23

ti-mo requested a review from a team October 6, 2021 21:23

ti-mo requested review from a team as code owners October 6, 2021 21:23

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from 17e9cd1 to c8be0b5 Compare October 11, 2021 11:52

tklauser approved these changes Oct 11, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned tklauser Oct 11, 2021

tklauser mentioned this pull request Oct 11, 2021

Update some dependencies to release versions #17497

Merged

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from c8be0b5 to f17159d Compare October 12, 2021 12:33

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from f17159d to 6f160eb Compare October 12, 2021 19:58

ti-mo and others added 7 commits October 13, 2021 10:49

vendor: upgrade cilium/ebpf to v0.7.0

3516aa7

Upgrade the lib to make use of new pinning features. Signed-off-by: Timo Beckers <timo@isovalent.com>

elf: use errors.Is() in symbol extraction, simplify logic

facc9d0

Document the ELF parsing logic to make it easier to grok. Signed-off-by: Timo Beckers <timo@isovalent.com>

ti-mo force-pushed the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch from 6f160eb to 66cd0e0 Compare October 13, 2021 08:50

borkmann approved these changes Oct 14, 2021

View reviewed changes

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

borkmann merged commit 4db3166 into master Oct 14, 2021

borkmann deleted the pr/nathanjsweet/migrate-cilium-migrate-map-from-c-to-go branch October 14, 2021 08:42

pchaigno reviewed Oct 14, 2021

View reviewed changes

pkg/bpf/bpffs_migrate.go Show resolved Hide resolved

pkg/bpf/bpffs_migrate.go Show resolved Hide resolved

pkg/bpf/bpffs_migrate.go Show resolved Hide resolved

pkg/datapath/loader/netlink.go Show resolved Hide resolved

pchaigno mentioned this pull request Oct 14, 2021

treewide: Use formatted logrus logs when possible #17611

Merged

ti-mo mentioned this pull request Oct 14, 2021

bpf: remove libelf dependency and unused nobpf.h #17612

Merged

tklauser mentioned this pull request Nov 15, 2021

bpf: remove accidentally committed cilium-map-migrate binary #17860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: Migrate `cilium-migrate-map` from C to Go #16917

bpf: Migrate `cilium-migrate-map` from C to Go #16917

nathanjsweet commented Jul 16, 2021 •

edited by ti-mo

brb commented Jul 21, 2021

ti-mo left a comment

ti-mo commented Sep 29, 2021

ti-mo commented Oct 6, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented Oct 11, 2021

tklauser left a comment

tklauser Oct 11, 2021

ti-mo Oct 11, 2021

ti-mo commented Oct 11, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ti-mo commented Oct 12, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented Oct 13, 2021

ti-mo commented Oct 13, 2021

ti-mo commented Oct 13, 2021

pchaigno left a comment

	@# Due to gcc bug, -lelf needs to be at the end.
	$(QUIET) ${HOST_CC} -Wall -O2 -Wno-format-truncation -I include/ $@.c -lelf -o $@

bpf: Migrate cilium-migrate-map from C to Go #16917

bpf: Migrate cilium-migrate-map from C to Go #16917

Conversation

nathanjsweet commented Jul 16, 2021 • edited by ti-mo

brb commented Jul 21, 2021

ti-mo left a comment

Choose a reason for hiding this comment

ti-mo commented Sep 29, 2021

ti-mo commented Oct 6, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented Oct 11, 2021

tklauser left a comment

Choose a reason for hiding this comment

tklauser Oct 11, 2021

Choose a reason for hiding this comment

ti-mo Oct 11, 2021

Choose a reason for hiding this comment

ti-mo commented Oct 11, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ti-mo commented Oct 12, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

ti-mo commented Oct 13, 2021

ti-mo commented Oct 13, 2021

ti-mo commented Oct 13, 2021

pchaigno left a comment

Choose a reason for hiding this comment

bpf: Migrate `cilium-migrate-map` from C to Go #16917

bpf: Migrate `cilium-migrate-map` from C to Go #16917

nathanjsweet commented Jul 16, 2021 •

edited by ti-mo

ti-mo commented Oct 6, 2021 •

edited by maintainer-s-little-helper bot

ti-mo commented Oct 11, 2021 •

edited by maintainer-s-little-helper bot

ti-mo commented Oct 12, 2021 •

edited by maintainer-s-little-helper bot