Skip to content

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

Closed
AkihiroSuda wants to merge 1 commit intomoby:masterfrom
AkihiroSuda:test-runc-2750
Closed

[DNM] trigger CI with runc#2750 ("seccomp: prepend -ENOSYS stub to all filters)#41900
AkihiroSuda wants to merge 1 commit intomoby:masterfrom
AkihiroSuda:test-runc-2750

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

cyphar commented Jan 28, 2021

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

@cyphar
Copy link
Copy Markdown
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
Copy Markdown
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.

2 participants