Use BPF_FS_MAGIC from go sys lib instead of hardcode

[odinuge]

This also avoids overflow during conversion. 0xCAFE4A11 is bigger than
the max of int32, so doing int32(uint32(0xCAFE4A11)) (will not compile
directly unless done over two lines) will result in -0x3501b5ef.

Due to some strange errors[0] in the types in "unix.Statfs_t" for 32 bits
systems, we have to explicitly convert to uint to support those (eg.
armv7). If we only need support for 64 bit systems, we can remove the
uint conversion as well.

[0]: For 32bits systems "fsdata.Type" should be uint32 instead of the
current int32, as it is in the linux kernel. This is due to the types in
glibc that the go types are generated from. For 64 bit systems the type
is correctly set to int64.

[odinuge]

Ref. #8599

/cc @rifelpet @olemarkus

[k8s-ci-robot]

@odinuge: GitHub didn't allow me to request PR reviews from the following users: olemarkus.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Ref. #8599

/cc @rifelpet @olemarkus

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]

I wanted to suggest using BPF_FS_MAGIC instead of the hardcoded value, but bumped into this on mac, same as you see in the failed CI run. The idea itself is good, converting to uint is a better solution:

nodeup/pkg/model/network.go:79:51: undefined: unix.BPF_FS_MAGIC

[odinuge]

Hmm. So CI runs on darwin as well.. The code is compiled with GOOS=linux GOARCH=amd64, and it should never run on anything else than linux, so I think the tests should run with that too.

Since the code (nodeup) is linux only the best would be to add // +build linux to the files to avoid issues, that will also avoid issues where the binaries are compiled for darwin instead of linux, when the code in reality doesn't support darwin. This is used everywhere in k/k.

[hakman]

I totally agree that the conversion part should be fixed.
Using unix.BPF_FS_MAGIC instead of the hardcoded value requires a bit too many changes for the gain. It also comes with some downsides, like anyone using macOS will have to change various options in the editor and for new contributors may complicate things.

I would propose to only do the the conversion part and leave the hardcoded value for now.

[odinuge]

I totally agree that the conversion part should be fixed.
Using unix.BPF_FS_MAGIC instead of the hardcoded value requires a bit too many changes for the gain. It also comes with some downsides, like anyone using macOS will have to change various options in the editor and for new contributors may complicate things.

I would propose to only do the the conversion part and leave the hardcoded value for now

I guess that is ok. Updated the PR now 😄

The statfs.Type type on darwin is a uint32, while it is int64 on linux/amd64 and int32 (but in the kernel it is a uint32) in linux/arm/v7, so the types doesn't make that much sense.

Making this "linux-only-code" mac and windows "firendly" may cause some gains, but I also think it can lead to some strange bugs when developers on darwin/windows compile the code for their platform, when it wouldn't properly run there anyways. To avoid regression i do think this deserve some testing, and without fixing the compilation issues, that wouln't be possible (since bpf is linux only).

[hakman]

Thanks :).
I hope that there isn't anyone trying to run nodeup on mac or windows. BPF would be the least of their problem.
Testing is tricky anyway as it depends on the system that would runt unix.Statfs. It may be a good idea to at least extract the comparison to a separate function and add some tests for it.
/lgtm

[rifelpet]

looks good, thanks!
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: odinuge, 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