Skip to content
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.

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

Stub out some functionality on non-Linux platforms #15355

Merged
merged 4 commits into from
Apr 12, 2021

Conversation

joestringer
Copy link
Member

This is a drive-by attempt to improve the compilability of the Cilium codebase
on non-Linux platforms, taking steps in the direction of simplifying the build
and test process for developers on those platforms. There's still some fairly
significant Linux-only dependencies in the BPF and datapath sections of the
code which this PR does not attempt to resolve, but these are some steps
forward.

  • mountinfo: Add stubs for non-Linux platforms
  • cgroups: Add stubs for non-Linux platforms
  • health: Add stubs for non-Linux platforms
  • modules: Add stubs for non-Linux platforms

Tested via:

$ GOOS=darwin go build ./pkg/$foo

@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Mar 16, 2021
@joestringer joestringer requested review from a team March 16, 2021 03:10
@joestringer joestringer requested a review from a team as a code owner March 16, 2021 03:10
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 16, 2021
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice 🍏

I've tried to test the affected packages on macOS and this is what I got:

$ for m in mountinfo cgroups health modules ; do echo "== $m"; go test ./pkg/$m; done
== mountinfo
# github.com/cilium/cilium/pkg/mountinfo [github.com/cilium/cilium/pkg/mountinfo.test]
pkg/mountinfo/mountinfo_test.go:608:37: undefined: unix.PROC_SUPER_MAGIC
pkg/mountinfo/mountinfo_test.go:613:36: undefined: FilesystemTypeBPFFS
FAIL	github.com/cilium/cilium/pkg/mountinfo [build failed]
FAIL
== cgroups
?   	github.com/cilium/cilium/pkg/cgroups	[no test files]
== health
no Go files in /Users/tklauser/go/src/github.com/cilium/cilium/pkg/health
== modules
# github.com/cilium/cilium/pkg/modules [github.com/cilium/cilium/pkg/modules.test]
pkg/modules/modules_test.go:157:22: undefined: parseModulesFile
FAIL	github.com/cilium/cilium/pkg/modules [build failed]
FAIL

The test build failure in pkg/modules should be fairly easy to fix by stubbing out parseModulesFile:

diff --git a/pkg/modules/modules_unspecified.go b/pkg/modules/modules_unspecified.go
index 7261040fe9b4..032dd464cc62 100644
--- a/pkg/modules/modules_unspecified.go
+++ b/pkg/modules/modules_unspecified.go
@@ -16,7 +16,10 @@

 package modules

-import "errors"
+import (
+       "errors"
+       "io"
+)

 var ErrNotImplemented = errors.New("not implemented")

@@ -27,3 +30,7 @@ func listModules() ([]string, error) {
 func moduleLoader() string {
        return "unknown-module-loader"
 }
+
+func parseModulesFile(r io.Reader) ([]string, error) {
+       return nil, ErrNotImplemented
+}

The tests themselves will then fail, but fixing them to deal with ErrNotSupported is certainly out of scope for this PR.

The test build failure in pkg/moutinfo requires stubbing out the FS magic numbers:

diff --git a/pkg/mountinfo/mountinfo_linux.go b/pkg/mountinfo/mountinfo_linux.go
index 12a7a42afdee..1d4913f38b6e 100644
--- a/pkg/mountinfo/mountinfo_linux.go
+++ b/pkg/mountinfo/mountinfo_linux.go
@@ -27,6 +27,8 @@ const (
 	// to be used for IsMountFS.
 	FilesystemTypeBPFFS   = unix.BPF_FS_MAGIC
 	FilesystemTypeCgroup2 = unix.CGROUP2_SUPER_MAGIC
+	filesystemTypeProc    = unix.PROC_SUPER_MAGIC
+	filesystemTypeTmpfs   = unix.TMPFS_MAGIC
 )
 
 // IsMountFS returns two boolean values, checking
diff --git a/pkg/mountinfo/mountinfo_privileged_test.go b/pkg/mountinfo/mountinfo_privileged_test.go
index fd3aa362061b..c7000855176e 100644
--- a/pkg/mountinfo/mountinfo_privileged_test.go
+++ b/pkg/mountinfo/mountinfo_privileged_test.go
@@ -36,7 +36,7 @@ func (s *MountInfoPrivilegedTestSuite) TestIsMountFSbyMount(c *C) {
 	c.Assert(err, IsNil)
 	defer os.RemoveAll(tmpDir)
 
-	mounted, matched, err := IsMountFS(unix.TMPFS_MAGIC, tmpDir)
+	mounted, matched, err := IsMountFS(filesystemTypeTmpfs, tmpDir)
 	c.Assert(err, IsNil)
 	c.Assert(mounted, Equals, false)
 	c.Assert(matched, Equals, false)
@@ -46,13 +46,13 @@ func (s *MountInfoPrivilegedTestSuite) TestIsMountFSbyMount(c *C) {
 	defer unix.Unmount(tmpDir, unix.MNT_DETACH)
 
 	// deliberately check with wrong fstype
-	mounted, matched, err = IsMountFS(unix.PROC_SUPER_MAGIC, tmpDir)
+	mounted, matched, err = IsMountFS(filesystemTypeProc, tmpDir)
 	c.Assert(err, IsNil)
 	c.Assert(mounted, Equals, true)
 	c.Assert(matched, Equals, false)
 
 	// now check with proper fstype
-	mounted, matched, err = IsMountFS(unix.TMPFS_MAGIC, tmpDir)
+	mounted, matched, err = IsMountFS(filesystemTypeTmpfs, tmpDir)
 	c.Assert(err, IsNil)
 	c.Assert(mounted, Equals, true)
 	c.Assert(matched, Equals, true)
diff --git a/pkg/mountinfo/mountinfo_test.go b/pkg/mountinfo/mountinfo_test.go
index 416c704e6b5a..5f41698dacdb 100644
--- a/pkg/mountinfo/mountinfo_test.go
+++ b/pkg/mountinfo/mountinfo_test.go
@@ -21,7 +21,6 @@ import (
 	"testing"
 
 	"github.com/cilium/cilium/pkg/checker"
-	"golang.org/x/sys/unix"
 
 	. "gopkg.in/check.v1"
 )
@@ -605,7 +604,7 @@ func (s *MountInfoTestSuite) TestGetMountInfo(c *C) {
 // system and machine to have any predictable mounts, but let's try a couple
 // of very well known paths.
 func (s *MountInfoTestSuite) TestIsMountFS(c *C) {
-	mounted, matched, err := IsMountFS(unix.PROC_SUPER_MAGIC, "/proc")
+	mounted, matched, err := IsMountFS(filesystemTypeProc, "/proc")
 	c.Assert(err, IsNil)
 	c.Assert(mounted, Equals, true)
 	c.Assert(matched, Equals, true)
diff --git a/pkg/mountinfo/mountinfo_unspecified.go b/pkg/mountinfo/mountinfo_unspecified.go
index 13450d7b7a40..0cbcdb08ea79 100644
--- a/pkg/mountinfo/mountinfo_unspecified.go
+++ b/pkg/mountinfo/mountinfo_unspecified.go
@@ -16,7 +16,17 @@
 
 package mountinfo
 
-import "errors"
+import (
+	"errors"
+)
+
+const (
+	// Dummy FilesystemType superblock magic numbers for filesystems,
+	// to be used for IsMountFS.
+	FilesystemTypeBPFFS = 0
+	filesystemTypeProc  = 0
+	filesystemTypeTmpfs = 0
+)
 
 // IsMountFS returns two boolean values, checking
 //  - whether the path is a mount point;

The tests themselves will also fail, but as with pkg/modules fixing them is out of scope here.

If you want I can also push these changes as fixup commits.

pkg/cgroups/cgroups_linux.go Outdated Show resolved Hide resolved
@joestringer
Copy link
Member Author

Thanks @tklauser , I indeed don't actually have the platform available so have been relying on go tooling for this so far. I didn't think of compiling the tests as well, I can probably figure out another command to do just that on other platforms as well. I recall there's some trick to it but I'd have to look it up.

I think that when it comes to the privileged tests, we are almost sure to be relying on Linux to perform the test since the elevated privileges are used to perform syscalls that we otherwise wouldn't have the rights for. So another approach for those tests would be to exclude them from !Linux platforms via go build tags.

Beyond that, sure feel free to push changes if you like. Otherwise I'd be happy to pick up these patch hunks, apply them on top and share authorship with you. In that case I'll take a look later on after some ☕.

@tklauser
Copy link
Member

Thanks @tklauser , I indeed don't actually have the platform available so have been relying on go tooling for this so far. I didn't think of compiling the tests as well, I can probably figure out another command to do just that on other platforms as well. I recall there's some trick to it but I'd have to look it up.

You can use e.g. GOOS=darwin go test -c ./pkg/$foo to cross-build the tests on any platform - and pass -tags privileged_test where appropriate. This will create a macOS binary ./pkg/$foo/$foo.test.

I think that when it comes to the privileged tests, we are almost sure to be relying on Linux to perform the test since the elevated privileges are used to perform syscalls that we otherwise wouldn't have the rights for. So another approach for those tests would be to exclude them from !Linux platforms via go build tags.

Agree, I think for the privileged tests it would make sense to add linux build tags.

Beyond that, sure feel free to push changes if you like. Otherwise I'd be happy to pick up these patch hunks, apply them on top and share authorship with you. In that case I'll take a look later on after some ☕.

Whatever works best for you. I've your happy applying the patch hunks, I'll let you go ahead so I don't mess with your workflow.

@joestringer joestringer marked this pull request as draft March 25, 2021 00:12
@joestringer joestringer removed the request for review from jrfastab April 8, 2021 17:28
@joestringer joestringer force-pushed the submit/drive-by-darwin-improvement branch from fcf721a to 01c4b9d Compare April 8, 2021 17:28
@joestringer joestringer marked this pull request as ready for review April 8, 2021 17:29
@joestringer
Copy link
Member Author

I finally got that ☕ ;) I applied the suggested hunks from @tklauser, added you as a co-author, and re-ran all of the package tests with GOOS=darwin to double-check.

joestringer and others added 4 commits April 8, 2021 11:40
Co-authored-by: Tobias Klauser <tobias@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer force-pushed the submit/drive-by-darwin-improvement branch from 01c4b9d to 63e635b Compare April 8, 2021 18:41
@joestringer
Copy link
Member Author

Actually, the linter didn't like defining vars for the FS magic vars so I just made the tests run on linux only instead.

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Actually, the linter didn't like defining vars for the FS magic vars so I just made the tests run on linux only instead.

Nice 🚀 as a side effect, the tests now "pass" on GOOS=darwin:

$ go test ./pkg/mountinfo
?   	github.com/cilium/cilium/pkg/mountinfo	[no test files]

@pchaigno
Copy link
Member

pchaigno commented Apr 9, 2021

test-me-please

@joestringer
Copy link
Member Author

Filed #15630 for the failure on test-1.21-4.9 since I see no way that some code tidying like this could cause only the net-next run to fail.

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.

Considering the net-next failure as a flake documented in the issue you linked, I'll go ahead and merge this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants