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

[DNM] trigger CI with runc#2750 ("seccomp: prepend -ENOSYS stub to all filters) #41900

Closed
wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda
Copy link
Member Author

This one seems failing consistently, though it doesn't really look related

=== RUN   TestDockerSuite/TestSaveDirectoryPermissions
    --- FAIL: TestDockerSuite/TestSaveDirectoryPermissions (1.20s)
        docker_cli_save_load_test.go:310: assertion failed: expression is false: found: failed to find the layer with the right content listing

cc @cyphar

@cyphar
Copy link
Contributor

cyphar commented Jan 27, 2021

I took a look at the failing test. I think it's an issue with updating runc than with the seccomp change -- the test in question assumes that the top-most layer will just contain opt/ opt/a/ opt/a/b/ opt/a/b/c but with the runc update it now also contains a tmp/ entry because runc now pre-fills /tmp I guess?

My guess is we just want a patch like the following added to the test:

diff --git a/integration-cli/docker_cli_save_load_test.go b/integration-cli/docker_cli_save_load_test.go
index 5f32d1c1d2b8..348b676f6285 100644
--- a/integration-cli/docker_cli_save_load_test.go
+++ b/integration-cli/docker_cli_save_load_test.go
@@ -283,7 +283,6 @@ func (s *DockerSuite) TestSaveDirectoryPermissions(c *testing.T) {
 
 	found := false
 	for _, entry := range dirs {
-		var entriesSansDev []string
 		if entry.IsDir() {
 			layerPath := filepath.Join(extractionDirectory, entry.Name(), "layer.tar")
 
@@ -293,14 +292,19 @@ func (s *DockerSuite) TestSaveDirectoryPermissions(c *testing.T) {
 			defer f.Close()
 
 			entries, err := listTar(f)
+			assert.NilError(c, err, "encountered error while listing tar entries: %s", err)
+
+			var entriesSansBuiltin []string
 			for _, e := range entries {
-				if !strings.Contains(e, "dev/") {
-					entriesSansDev = append(entriesSansDev, e)
+				// Filter out built-in directories added to layers (like /dev
+				// inodes and /tmp tmpfs) since those may change if our default
+				// config changes.
+				if !strings.Contains(e, "dev/") && e != "tmp/" {
+					entriesSansBuiltin = append(entriesSansBuiltin, e)
 				}
 			}
-			assert.NilError(c, err, "encountered error while listing tar entries: %s", err)
 
-			if reflect.DeepEqual(entriesSansDev, layerEntries) || reflect.DeepEqual(entriesSansDev, layerEntriesAUFS) {
+			if reflect.DeepEqual(entriesSansBuiltin, layerEntries) || reflect.DeepEqual(entriesSansBuiltin, layerEntriesAUFS) {
 				found = true
 				break
 			}

I don't see how the seccomp change specifically would trigger this.

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Jan 27, 2021

Moby CI with opencontainers/runc@c69ae75 (master, Jan 22) + f266f13 was passing TestDockerSuite/TestSaveDirectoryPermissions: https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-41645/9/pipeline (ppc64le failure is unrelated)

@cyphar

This comment has been minimized.

@cyphar
Copy link
Contributor

cyphar commented Jan 28, 2021

Oh I just figured out what it was while writing the above comment (and it took me way too long to realise this) -- it's because I created a temporary file in /tmp in order to use seccomp_export_bpf(3) (which only takes an fd). I will switch it to an actual pipe so we don't need to touch the filesystem...

@cyphar
Copy link
Contributor

cyphar commented Jan 28, 2021

Yup, with the latest commits the test no longer fails.

@cyphar
Copy link
Contributor

cyphar commented Jan 28, 2021

Sorry, I pushed a new commit you'll need to rebase this.

…l filters")

Testing compatibility of opencontainers/runc#2750 .

Commits: https://github.com/cyphar/runc/commits/seccomp-patched-bpf

DO NOT MERGE.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda
Copy link
Member Author

The runc PR is now merged, thanks @cyphar !

@AkihiroSuda AkihiroSuda closed this Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants