Skip to content

datapath: Switch to upstream bpftool, remove additional arguments #10353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 28, 2020
Merged

datapath: Switch to upstream bpftool, remove additional arguments #10353

merged 3 commits into from
Feb 28, 2020

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Feb 26, 2020 •
edited
Loading

Upstream bpftool does not run probes which emit dmesg messages by
default anymore. Additional arguments for filtering out probes are not
needed anymore.

Link: https://lore.kernel.org/bpf/20200226165941.6379-1-mrostecki@opensuse.org/T/

Signed-off-by: Michal Rostecki mrostecki@opensuse.org

Switch to upstream bpftool

This change is Reviewable

@vadorovsky vadorovsky requested a review from a team February 26, 2020 22:57
@vadorovsky vadorovsky requested a review from a team as a code owner February 26, 2020 22:57
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@vadorovsky
Copy link
Member Author

@borkmann is going to submit a change in packer-ci-build repository which will switch bpftool to bpf-next tree this week.

But anyway, accepting this PR now should be fine. Even if Vagrant images are shipping the old bpftool now, this pull request should still work, although it will emit the dmesg warnings. But it's master, so it should be fine. And after the new VM image will ship the newest bpftool, dmesg warnings will be not emitted anymore.

/cc @qmonnet @joestringer

@vadorovsky vadorovsky added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed dont-merge/needs-release-note labels Feb 26, 2020
@vadorovsky
Copy link
Member Author

test-me-please

@vadorovsky vadorovsky added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Feb 26, 2020
joestringer
joestringer approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Any reservations on backporting into 1.7.1 so we have an official bpftool version in the main images for the latest release?

@vadorovsky
Copy link
Member Author

vadorovsky commented Feb 26, 2020 •
edited
Loading

LGTM. Any reservations on backporting into 1.7.1 so we have an official bpftool version in the main images for the latest release?

I think we need new Vagrant image with upstream bpftool before we backport it to any stable release. So we need to wait for @borkmann's PR in packer-ci-build.

@coveralls
Copy link

coveralls commented Feb 26, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.003%) to 45.551% when pulling 71ca02e on mrostecki:upstream-bpftool into 6d583fd on cilium:master.

qmonnet
qmonnet requested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit on bpftool invocation, looks good otherwise! Thanks.

aanm
aanm requested changes Feb 27, 2020
View reviewed changes
@vadorovsky
datapath: Switch to upstream bpftool, remove additional arguments
7399b9a 
Upstream bpftool does not run probes which emit dmesg messages by
default anymore. Additional arguments for filtering out probes are not
needed anymore.

Link: https://lore.kernel.org/bpf/20200226165941.6379-1-mrostecki@opensuse.org/T/

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
vagrant: Switch to upstream bpftool
f85dddc 
Forked sources of the Linux kernel are not needed anymore, upstream
bpftool does not run probes which emit dmesg messages by default.

Link: https://lore.kernel.org/bpf/20200226165941.6379-1-mrostecki@opensuse.org/T/

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
Copy link
Member Author

@aanm Sorry for that! Also the dir name was wrong. Now it should work.

@vadorovsky
Copy link
Member Author

test-me-please

aanm
aanm requested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrostecki please add the following changes to the docker: Switch to upstream bpftool commit

diff --git a/Dockerfile b/Dockerfile
index 7699103b7..1c724d074 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -37,7 +37,7 @@ RUN make LOCKDEBUG=$LOCKDEBUG PKG_BUILD=1 V=$V LIBNETWORK_PLUGIN=$LIBNETWORK_PLU
 # built while allowing the new versions to make changes that are not
 # backwards compatible.
 #
-FROM quay.io/cilium/cilium-runtime:2020-02-26
+FROM quay.io/cilium/cilium-runtime:2020-02-27
 LABEL maintainer="maintainer@cilium.io"
 COPY --from=builder /tmp/install /
 COPY --from=cilium-envoy / /

Once this PR is merged and PR #10354 is also merged, we need to open a PR against 1.7 for which the cilium-runtime image tag will be 2020-02-27-v1.7

@vadorovsky vadorovsky requested a review from a team as a code owner February 27, 2020 16:46
@vadorovsky
Copy link
Member Author

test-me-please

@vadorovsky
Copy link
Member Author

@aanm

Once this PR is merged and PR #10354 is also merged, we need to open a PR against 1.7 for which the cilium-runtime image tag will be 2020-02-27-v1.7

We also need a change in packer-ci-build before doing that, but again, on sig-datapath meeting we decided that Daniel will do that alongside with the other changes he's preparing.

@aanm
Copy link
Member

aanm commented Feb 27, 2020

@aanm

Once this PR is merged and PR #10354 is also merged, we need to open a PR against 1.7 for which the cilium-runtime image tag will be 2020-02-27-v1.7

We also need a change in packer-ci-build before doing that, but again, on sig-datapath meeting we decided that Daniel will do that alongside with the other changes he's preparing.

@mrostecki in that case, shouldn't we wait for the VM to be made available? Otherwise the cilium build will fail for old VMs, no?

@aanm
Copy link
Member

aanm commented Feb 27, 2020

@mrostecki

17:54:01  Step 2/20 : FROM quay.io/cilium/cilium-builder:2020-02-27 as builder
17:54:02  manifest for quay.io/cilium/cilium-builder:2020-02-27 not found: manifest unknown: manifest unknown

the builder is not required, it should be the runtime image only.

jrajahalme
jrajahalme approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the builder vs. runtime issue @aanm pointed out.

@vadorovsky
docker: Switch to upstream bpftool
71ca02e 
Forked sources of the Linux kernel are not needed anymore, upstream
bpftool does not run probes which emit dmesg messages by default.

OCI images of Cilium used to clone the mainline Linus' tree of the
kernel sources, but to get upstream changes faster, this change uses the
bpf-next tree.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
Copy link
Member Author

vadorovsky commented Feb 28, 2020 •
edited
Loading

@aanm @jrajahalme Sorry for that, now it should work.

@aanm

@mrostecki in that case, shouldn't we wait for the VM to be made available? Otherwise the cilium build will fail for old VMs, no?

Well, the thing is that this pull request will work on old VMs (in terms of not being completely broken or making the CI red 😉 ), but will generate dmesg messages, which I guess is not a big deal on master branch (although it would be a big deal on stable branches).

That's because this PR goes back to the default set of arguments for bpftool and the upstream change is about not running bpf_write_user and bpf_trace_printk helper probes by default (if someone doesn't mind dmesg warnings and wants to run them, the additional argument full needs to be provided).

But on the other hand, I don't mind waiting for the change in packer-ci-build. I'm fine doing it either way. Maybe it's better to wait for the new VM to not confuse people and start backporting right after this PR is merged.

@vadorovsky
Copy link
Member Author

test-me-please

@aanm aanm merged commit a6d703e into cilium:master Feb 28, 2020
@vadorovsky vadorovsky deleted the upstream-bpftool branch February 28, 2020 13:24
@brb brb mentioned this pull request Mar 2, 2020
Merged
@vadorovsky vadorovsky mentioned this pull request Mar 3, 2020
Closed
@vadorovsky vadorovsky mentioned this pull request Mar 3, 2020
Merged
@jrajahalme jrajahalme mentioned this pull request Mar 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@qmonnet qmonnet qmonnet approved these changes

@ianvernon ianvernon Awaiting requested review from ianvernon

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vadorovsky @coveralls @aanm @joestringer @jrajahalme @qmonnet