Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

assets: Ensure BPFFS mount (bsc#1146991) #1101

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

vadorovsky
Copy link
Contributor

@vadorovsky vadorovsky commented May 12, 2020

Cilium DaemonSet has the host mount from /sys/fs/bpf directory and it
expects a mount of BPFFS type. Otherwise, it creates its own BPFFS
mount, but it's not persistent. To avoid any issues with lack of the
mount on the host, ensure it in kubelet service

Signed-off-by: Michal Rostecki mrostecki@suse.de

@vadorovsky vadorovsky force-pushed the fix-bpffs-mount branch 3 times, most recently from 282d11d to 2569cf0 Compare June 16, 2020 14:41
@vadorovsky vadorovsky requested a review from dirkmueller June 16, 2020 15:16
jenting
jenting previously approved these changes Jun 17, 2020
Copy link

@soumynathan soumynathan 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 to me.

@jenting jenting requested a review from evrardjp June 17, 2020 07:16
@@ -23,6 +23,7 @@ Description=kubelet: The Kubernetes Node Agent
Documentation=http://kubernetes.io/docs/

[Service]
ExecStartPre=/bin/bash -c "findmnt -t bpf --mountpoint /sys/fs/bpf || mount bpffs /sys/fs/bpf -t bpf"
Copy link
Member

Choose a reason for hiding this comment

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

NIT: this prints a message, we might want to do this as

/bin/bash -c "findmnt -t bpf --mountpoint /sys/fs/bpf > /dev/null || mount bpffs /sys/fs/bpf -t bpf"

@dirkmueller
Copy link
Member

So it looks like this is a missing feature in systemd. Starting with systemd 238, the bpffs is mounted by default.

did we ask the systemd maintainers whether they'd be interested in backporting the fixes? that would fix all CaaSP releases at once.

@vadorovsky
Copy link
Contributor Author

did we ask the systemd maintainers whether they'd be interested in backporting the fixes? that would fix all CaaSP releases at once.

We didn't.

Cilium DaemonSet has the host mount from /sys/fs/bpf directory and it
expects a mount of BPFFS type. Otherwise, it creates its own BPFFS
mount, but it's not persistent. To avoid any issues with lack of the
mount on the host, ensure it in kubelet service.

Fixes: SUSE#712

Signed-off-by: Michal Rostecki <mrostecki@suse.de>
@dirkmueller
Copy link
Member

I asked now about a backport for systemd/systemd@43b7f24

which would fix all caasp releases at the same time without having to get the caasp team to release updates on all caasp versions (which will take weeks to months)

@evrardjp
Copy link
Contributor

evrardjp commented Jun 17, 2020

I thought we were not allowed to backport systemd changes (or update systemd on stable)! That's amazing! Should we just drop this?

Copy link
Member

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

LGTM

@evrardjp evrardjp merged commit cc36786 into SUSE:master Jun 17, 2020
@dirkmueller
Copy link
Member

@evrardjp Its not clear whether we can do the systemd backport (systemd is maintained by the systemd maintainers at SUSE). they have any right to reject such a backport or delay it until the next SP.

however we need a solution in the caasp product nontheless. if it won't be done by systemd then there needs to be a caasp specific solution. it doesn't hurt to continue with this change though, a systemd fix will cause the code to become obsolete but it will not hurt anyone. once the systemd update is released (and sufficient time has passed to assume that customers have updated) the patch could simply be reverted.

@evrardjp
Copy link
Contributor

tl;dr: I agree. (In fact I agreed else I wouldn't have merged).

I am not sure this change will be easily backportable into older branches for fixing caasp4.
It doesn't hurt to merge it for CaaSP5 until we figure it out with systemd.

I encourage the network team to think about the fact this won't fix CaaSP4, as this PR's target is master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants