Cilium fix bpffs check #8599

olemarkus · 2020-02-20T20:27:32Z

The check introduced in #7832 is unfortunately too naïve to detect if bpffs has been mounted leading to the unit file never being installed. This is quite disastrous for cilium.

This check in this PR should be more robust.

/kind bug

k8s-ci-robot · 2020-02-20T20:27:40Z

Hi @olemarkus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hakman · 2020-02-20T20:43:08Z

/ok-to-test

rifelpet · 2020-02-21T02:55:51Z

@olemarkus I see the original PR was backported all the way to 1.15. Should this be considered a blocker for 1.16.0 stable?

olemarkus · 2020-02-21T05:42:33Z

I don't understand why, but the 1.16 and 1.15 branch is not affected. The /sys/fs/bpf path doesn't exist at the time of the check on those branches even if I use the same ami. Another change added to kops after 1.16 branch was cut must be causing this.

hakman · 2020-02-21T05:48:40Z

Just a quick update, the Cilium periodic e2e is working again:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/ci-kubernetes-e2e-kops-aws-cni-cilium/1230702775738830848

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template

olemarkus · 2020-02-21T20:48:48Z

@rifelpet unfortunately, this does indeed break cilium back to 1.15.2. It should probably block 1.16.

hakman · 2020-02-21T21:23:30Z

@olemarkus the sorting in vendor/modules.txt seems off can you check that you are using go v1.13.x?
https://go-review.googlesource.com/c/go/+/174527/

olemarkus · 2020-02-21T21:52:50Z

I am using go1.13.4 and just running make gomod. When I run it again, there are no changes to the vendor/modules.txt file.

olemarkus · 2020-02-22T05:52:40Z

For some reason make gomod used another binary than expected. Things looks a lot more sane now.

hakman · 2020-02-22T07:07:36Z

Thanks for the update @olemarkus. I was just about to start checking why it worked differently on my end.

hakman · 2020-02-22T09:57:01Z

Me and @rifelpet did some research yesterday on this topic and will add some questions.

hakman · 2020-02-22T10:02:47Z

nodeup/pkg/model/network.go

-			if os.IsNotExist(err) {
-				alreadyMounted = false
-			} else {
-				return fmt.Errorf("error checking for /sys/fs/bpf: %v", err)
-			}


I think this is still valid. If there is an unexpected error when reading from /sys/fs/bp, the systemd mount would not help, right?

This is probably still valid. Most likely this would be due to the path being absent and if the unit tries to mount it will just fail. Result will be quite similar and I suppose equally easy to detect.

hakman · 2020-02-22T10:10:20Z

nodeup/pkg/model/network.go

 		} else {
-			alreadyMounted = true
+			alreadyMounted = int32(magic) == int32(fsdata.Type)


What happens if the fs type is not BPF? Would it be better to return with an error also?

The Cilium code describes the logic for mounting the BPF fs and there is also a mention of a fallback location /run/cilium/bpffs:
https://github.com/cilium/cilium/blob/cbd63be46569751ac45060e54f6787f1286b75cd/pkg/bpf/bpffs_linux.go#L213-L224

// checkOrMountDefaultLocations tries to check or mount the BPF filesystem in
// standard locations, which are:
// - /sys/fs/bpf
// - /run/cilium/bpffs
// There is a procedure of determining which directory is going to be used:
// 1. Checking whether BPFFS filesystem is mounted in /sys/fs/bpf.
// 2. If there is no mount, then mount BPFFS in /sys/fs/bpf and finish there.
// 3. If there is a BPFFS mount, finish there.
// 4. If there is a mount, but with the other filesystem, then it means that most
// probably Cilium is running inside container which has mounted /sys/fs/bpf
// from host, but host doesn't have proper BPFFS mount, so that mount is just
// the empty directory. In that case, mount BPFFS under /run/cilium/bpffs.

If BPF is not mounted on /sys/fs/bpf, the path will still exist, which is why the old check fails. So most likly the fs type will not be bpf before we install the unit.

/run/cilium/bpffs is a fallback if the host is not mounting bpf. This will cause network disruptions every time cilium pods restart though since the bpf mounts are lost.

OK, more clear now. Thanks for clarifying the fallback part.

Regarding the check, if the path exists, unix.Statfs would succeed and will return SYSFS_MAGIC=0x62656572 or something similar. If there is an error other than the "no such file or directory", maybe it's not a good thing to continue. Same goes for a magic number that is not SYSFS_MAGIC or BPF_FS_MAGIC.

Shouldn't you be able to mount bpffs on whatever path that exists?

I will add back the return on error shortly. unix.Statfs("/sys/fs/bpf", &fsdata) would return error on file not found too, shouldn't it? So the os.Stat check would be redundant.

Just checked and you are right, if the mount path doesn't exist, the unit won't help. Probably for any type of error BPF mounting will fail.

This part is mostly ok. There may be a rare case where the magic number is different from SYSFS_MAGIC=0x62656572 or BPF_FS_MAGIC= 0xCAFE4A11 and mounting will fail. I don't think it's worth adding it for now.

rifelpet · 2020-02-22T14:16:36Z

nodeup/pkg/model/network.go

-			} else {
-				return fmt.Errorf("error checking for /sys/fs/bpf: %v", err)
-			}
+			return fmt.Errorf("error checking for /sys/fs/bpf: %v", err)
 		} else {


nit: we don't need this else statement anymore.

rifelpet · 2020-02-22T14:17:36Z

nodeup/pkg/model/network.go

@@ -70,15 +70,15 @@ func (b *NetworkBuilder) Build(c *fi.ModelBuilderContext) error {
 	if networking.Cilium != nil {
 		// systemd v238 includes the bpffs mount by default; and gives an error "has a bad unit file setting" if we try to mount it again (see mount_point_is_api)
 		var alreadyMounted bool
-		_, err := os.Stat("/sys/fs/bpf")
+		// bpffs magic number
+		magic := uint32(0xCAFE4A11)


it might be worth adding this link as a comment: https://github.com/torvalds/linux/blob/v4.8/include/uapi/linux/magic.h#L80

Also would be nice to move the var and comment just before it's used.

rifelpet · 2020-02-22T19:06:54Z

/lgtm
/approve

We increased the frequency of the cilium periodic job to run hourly, we'll keep an eye on it to confirm things are still stable before we merge cherry-picks of this 👍

k8s-ci-robot · 2020-02-22T19:07:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rifelpet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

justinsb · 2020-02-22T20:19:54Z

I am a little confused ... https://k8s-testgrid.appspot.com/google-aws#kops-aws-cni-cilium seems to suggest this broke two days ago, but my original (insufficient) PR is from a month or two ago. Do we know what caused it to break? (Was it a newer version of cilium that makes the directory?)

olemarkus · 2020-02-22T20:38:31Z

Just bad timing. The e2e tests broke because of the typo fixed here #8591
The bpf mount bug this PR fixed is unrelated to the e2e tests.

justinsb · 2020-02-22T21:44:36Z

Ah - thanks @olemarkus (and thanks also for fixing!)

…9-origin-release-1.15 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

…9-origin-release-1.17 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

…9-origin-release-1.16 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

mazzy89 · 2020-02-23T14:20:50Z

@olemarkus I'm running k8s 1.15.9 with cilium from kops 1.15.2 but I'm not having this issue.Is it clear for you why this version is not affected?

olemarkus · 2020-02-23T14:52:25Z

It is affected too. If you see the cilium agent logs, you should see a warning about bpf not already mounted on the hair

mazzy89 · 2020-02-23T14:59:20Z

I see. I'll take a look at that. Which are the effects of this? I did not notice anything weird in the cluster. Everything is operative and healthy in term of networking.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 20, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 20, 2020

k8s-ci-robot requested review from joshbranham and robinpercy February 20, 2020 20:29

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 20, 2020

yurrriq reviewed Feb 21, 2020

View reviewed changes

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.12.yaml.template Outdated Show resolved Hide resolved

Properly detect that bpffs has been mounted

f2f29d9

rifelpet added the blocks-next label Feb 21, 2020

olemarkus force-pushed the cilium-fix-bpffs-check branch from bdf4a39 to 60a8f34 Compare February 21, 2020 21:05

olemarkus force-pushed the cilium-fix-bpffs-check branch from 60a8f34 to 5df745d Compare February 22, 2020 05:47

make gomod

d728ac8

olemarkus force-pushed the cilium-fix-bpffs-check branch from 5df745d to d728ac8 Compare February 22, 2020 05:49

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 22, 2020

hakman reviewed Feb 22, 2020

View reviewed changes

Return error if statfs fails

7c84e7d

rifelpet reviewed Feb 22, 2020

View reviewed changes

Various nits

bcafdca

k8s-ci-robot assigned rifelpet Feb 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2020

k8s-ci-robot merged commit 116ec23 into kubernetes:master Feb 22, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 22, 2020

k8s-ci-robot added a commit that referenced this pull request Feb 22, 2020

Merge pull request #8612 from olemarkus/automated-cherry-pick-of-#859…

848004b

…9-origin-release-1.15 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

k8s-ci-robot added a commit that referenced this pull request Feb 22, 2020

Merge pull request #8613 from olemarkus/automated-cherry-pick-of-#859…

c3847b9

…9-origin-release-1.17 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

k8s-ci-robot added a commit that referenced this pull request Feb 22, 2020

Merge pull request #8611 from olemarkus/automated-cherry-pick-of-#859…

38e3bb0

…9-origin-release-1.16 Automated cherry pick of #8599: Properly detect that bpffs has been mounted

odinuge mentioned this pull request Feb 28, 2020

Use BPF_FS_MAGIC from go sys lib instead of hardcode #8650

Merged

olemarkus deleted the cilium-fix-bpffs-check branch March 25, 2020 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cilium fix bpffs check #8599

Cilium fix bpffs check #8599

olemarkus commented Feb 20, 2020

k8s-ci-robot commented Feb 20, 2020

hakman commented Feb 20, 2020

rifelpet commented Feb 21, 2020

olemarkus commented Feb 21, 2020

hakman commented Feb 21, 2020

olemarkus commented Feb 21, 2020

hakman commented Feb 21, 2020 •

edited

Loading

olemarkus commented Feb 21, 2020

olemarkus commented Feb 22, 2020

hakman commented Feb 22, 2020

hakman commented Feb 22, 2020

hakman Feb 22, 2020

olemarkus Feb 22, 2020

hakman Feb 22, 2020

hakman Feb 22, 2020 •

edited

Loading

olemarkus Feb 22, 2020

hakman Feb 22, 2020 •

edited

Loading

olemarkus Feb 22, 2020

hakman Feb 22, 2020

hakman Feb 22, 2020

rifelpet Feb 22, 2020

rifelpet Feb 22, 2020

hakman Feb 22, 2020

rifelpet commented Feb 22, 2020

k8s-ci-robot commented Feb 22, 2020

justinsb commented Feb 22, 2020

olemarkus commented Feb 22, 2020

justinsb commented Feb 22, 2020

mazzy89 commented Feb 23, 2020

olemarkus commented Feb 23, 2020

mazzy89 commented Feb 23, 2020

Cilium fix bpffs check #8599

Cilium fix bpffs check #8599

Conversation

olemarkus commented Feb 20, 2020

k8s-ci-robot commented Feb 20, 2020

hakman commented Feb 20, 2020

rifelpet commented Feb 21, 2020

olemarkus commented Feb 21, 2020

hakman commented Feb 21, 2020

olemarkus commented Feb 21, 2020

hakman commented Feb 21, 2020 • edited Loading

olemarkus commented Feb 21, 2020

olemarkus commented Feb 22, 2020

hakman commented Feb 22, 2020

hakman commented Feb 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman Feb 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman Feb 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rifelpet commented Feb 22, 2020

k8s-ci-robot commented Feb 22, 2020

justinsb commented Feb 22, 2020

olemarkus commented Feb 22, 2020

justinsb commented Feb 22, 2020

mazzy89 commented Feb 23, 2020

olemarkus commented Feb 23, 2020

mazzy89 commented Feb 23, 2020

hakman commented Feb 21, 2020 •

edited

Loading

hakman Feb 22, 2020 •

edited

Loading

hakman Feb 22, 2020 •

edited

Loading