Move libcontainer to x/sys/unix #1442

clnperez · 2017-05-09T22:28:56Z

Since syscall is outdated and broken for some architectures,
use x/sys/unix instead.

There are still some dependencies on the syscall package that will
remain in syscall for the forseeable future:

Errno
Signal
SysProcAttr

Additionally, os still uses syscall, so it needs to be kept for anything
returning *os.ProcessState, such as process.Wait.

Signed-off-by: Christy Perez [email protected]

clnperez · 2017-05-09T22:30:19Z

This fixes several tests that were failing on POWER (ppc64le) -- including some criu tests. I tested using make test but with a modified dockerfile and makefile to pull in the right power bits.

clnperez · 2017-05-09T22:46:26Z

On a closer look, there's not that much code outside of libcontainer that doesn't also need to be changed, so WDYT about replacing syscall everywhere else as well?

clnperez · 2017-05-09T22:58:05Z

Hm. Tests failing again. Hold off on this for now.

wking · 2017-05-11T12:17:41Z

libcontainer/specconv/spec_linux.go

@@ -710,7 +711,7 @@ func setupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) {
 		return nil, nil
 	}

-	// No default action specified, no syscalls listed, assume seccomp disabled
+	// No default action specified, no unixs listed, assume seccomp disabled


I think this (and a few later entries) should keep syscalls. In #1445 I replaced sycalls. with unix. (including the trailing period), and while that can still have false positives (e.g. when the word is at the end of a comment sentence), it won't accidentally do things like this.

Oops. Yep. I thought I caught all the spots I needed to keep syscall. I'll take a closer look.

clnperez · 2017-05-11T14:59:14Z

Fixed those error message and comment false positives @wking. PTAL.

wking · 2017-05-11T16:08:49Z

On Thu, May 11, 2017 at 07:59:15AM -0700, christyp wrote: Fixed those error message and comment false positives @wking. PTAL.

I haven't looked at the rest of this PR, but as of 67d40a8, the only difference between it and #1445 for libcontainer/specconv/spec_linux.go is a blank line in the import block. So that looks fine to me.

wking · 2017-05-11T16:33:40Z

Travis isn't happy. It looks like the tests hung?

dqminh · 2017-05-11T22:53:45Z

libcontainer/factory_linux.go

@@ -303,7 +304,7 @@ func (l *LinuxFactory) StartInitialization() (err error) {
 		return err
 	}

-	// If Init succeeds, syscall.Exec will not return, hence none of the defers will be called.
+	// If Init succeeds, unix.Exec will not return, hence none of the defers will be called.


this should still be syscall.Exec

dqminh · 2017-05-11T22:54:37Z

libcontainer/init_linux.go

@@ -19,6 +19,8 @@ import (
 	"github.com/opencontainers/runc/libcontainer/user"
 	"github.com/opencontainers/runc/libcontainer/utils"
 	"github.com/vishvananda/netlink"
+
+	sysunix "golang.org/x/sys/unix"


why do we use sysunix import name here ?

Weird. I swear there was a conflict here. I'll change it to unix, thanks.

dqminh · 2017-05-11T22:57:35Z

libcontainer/system/setns_linux.go

-		return err
-	}
-	return nil
-}


thanks for removing unused functions !

dqminh · 2017-05-11T22:59:41Z

list.go

@@ -7,7 +7,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"syscall"
+	"syscall" // avoid use if possible


i think this comment is not required.

clnperez · 2017-05-12T18:51:00Z

Strange that this failed with go 1.8, but not 1.7, and not go tip. Is that failure something that anyone has seen before?

clnperez · 2017-05-15T15:50:45Z

For reference (because I'm about to rebase) the failure seen on go 1.8 is:

--- FAIL: TestNotifyOnOOM (0.00s)
	notify_linux_test.go:99: expected event control to be closed
	notify_linux_test.go:103: expected event fd to be closed

clnperez · 2017-05-15T15:59:03Z

😆 So this time -- that same test passed with 1.8 and failed with go tip and 1.7. I'll take a look at that test.

clnperez · 2017-05-15T19:12:49Z

I added the error to the debug output so that I could see what error it's getting, since not unix.EBADF -- but this time all three go versions' tests passed. WDYT @dqminh, @wking?

wking · 2017-05-15T19:16:11Z

On Mon, May 15, 2017 at 12:12:51PM -0700, christyp wrote: … but this time all three go versions' tests passed. WDYT @dqminh, @wking?

Maybe amend the commit to get a new commit date (and commit hash), and repeat until you can trigger the failure again? It certainly seems like a flaky-test issue to me, but I'm not a maintainer so I can't speak to whether fixing the test is a blocker for this PR or not.

clnperez · 2017-05-16T21:51:45Z

Welp, I tried several times and just can't get this to fail again.

clnperez · 2017-05-22T18:54:36Z

Does this look okay to merge? I can't get the test to fail again so it must be flaky.

crosbymichael · 2017-05-22T19:45:18Z

@clnperez this needs to be rebased

crosbymichael · 2017-05-22T21:13:31Z

libcontainer/devices/devices_linux.go

 		devType = 'b'
 	}
-	stat_t, ok := fileInfo.Sys().(*syscall.Stat_t)
+	stat_t, ok := fileInfo.Sys().(*unix.Stat_t)


May need to verify this line, i'm not sure if the cast will work

Ya, you will have to revert this line.

panic: interface conversion: interface {} is *syscall.Stat_t, not *unix.Stat_t

i thought i had fixed that issue. hold tight on reviews while i take a look at my rebase.

It also looks like you have an unrelated commit shfmt'ing files

i put that in there b/c the tests failed (but also kept it separate b/c it is unrelated)

Since syscall is outdated and broken for some architectures, use x/sys/unix instead. There are still some dependencies on the syscall package that will remain in syscall for the forseeable future: Errno Signal SysProcAttr Additionally: - os still uses syscall, so it needs to be kept for anything returning *os.ProcessState, such as process.Wait. Signed-off-by: Christy Perez <[email protected]>

Signed-off-by: Christy Perez <[email protected]>

clnperez · 2017-05-22T22:48:08Z

@crosbymichael PTAL. Three questions:

Can I leave the other usages of Stat_t as-is (unix.Stat_t)?
What should I do about the unrelated shfmt commit? The CI fails me I don't fix those up.
Should I squash the two code changes (or three...) into one commit? I was originally planning on only updating libcontainer but there weren't many more files outside of it that needed switching over.

Signed-off-by: Christy Perez <[email protected]>

clnperez · 2017-05-25T13:56:17Z

Tests seem a little flaky. All passing now.

crosbymichael · 2017-05-25T17:15:10Z

LGTM

I don't know anything about why the tests are failing because of shfmt.

unix.Stat_t is food for all the other cases because you are using it in the syscall and not what FileInfo returns.

Thanks!

dqminh · 2017-05-26T11:18:24Z

LGTM

tianon mentioned this pull request May 11, 2017

libcontainer/specconv/spec_linux: Use golang.org/x/sys/unix #1445

Closed

wking reviewed May 11, 2017

View reviewed changes

clnperez force-pushed the libcontainer-sys-unix branch from 1613aa1 to 852f375 Compare May 11, 2017 14:57

clnperez force-pushed the libcontainer-sys-unix branch from 67d40a8 to d3618be Compare May 11, 2017 20:15

dqminh reviewed May 11, 2017

View reviewed changes

clnperez force-pushed the libcontainer-sys-unix branch from d3618be to 9058649 Compare May 12, 2017 14:25

wking mentioned this pull request May 13, 2017

config.md: fix typo of context opencontainers/runtime-spec#807

Merged

clnperez force-pushed the libcontainer-sys-unix branch from 9058649 to 402f178 Compare May 15, 2017 15:51

clnperez force-pushed the libcontainer-sys-unix branch from 402f178 to 8f12a57 Compare May 15, 2017 19:01

clnperez force-pushed the libcontainer-sys-unix branch 3 times, most recently from ad1bf68 to a958c89 Compare May 16, 2017 21:41

clnperez force-pushed the libcontainer-sys-unix branch 2 times, most recently from eb81e24 to 9e7fc57 Compare May 22, 2017 20:59

crosbymichael reviewed May 22, 2017

View reviewed changes

clnperez force-pushed the libcontainer-sys-unix branch from 59231a1 to 40d0d47 Compare May 22, 2017 21:16

clnperez force-pushed the libcontainer-sys-unix branch 2 times, most recently from fc52b35 to a11a682 Compare May 22, 2017 21:30

clnperez added 2 commits May 22, 2017 17:35

Moving the rest of runc to x/sys/unix

187d2d8

Signed-off-by: Christy Perez <[email protected]>

clnperez force-pushed the libcontainer-sys-unix branch from a11a682 to 187d2d8 Compare May 22, 2017 22:39

wking mentioned this pull request May 24, 2017

Mount correction opencontainers/runtime-spec#854

Merged

clnperez force-pushed the libcontainer-sys-unix branch from c97ea57 to c78515c Compare May 24, 2017 21:34

shfmt'ing files

9ed7e9b

Signed-off-by: Christy Perez <[email protected]>

clnperez force-pushed the libcontainer-sys-unix branch from c78515c to 9ed7e9b Compare May 24, 2017 21:43

dqminh merged commit 67bd2ab into opencontainers:master May 26, 2017

clnperez deleted the libcontainer-sys-unix branch May 30, 2017 18:45

thaJeztah mentioned this pull request Nov 13, 2018

[17.06 backport] libcontainer: ability to compile without kmem docker-archive/runc#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move libcontainer to x/sys/unix #1442

Move libcontainer to x/sys/unix #1442

clnperez commented May 9, 2017

clnperez commented May 9, 2017 •

edited

Loading

clnperez commented May 9, 2017

clnperez commented May 9, 2017

wking May 11, 2017

clnperez May 11, 2017 •

edited

Loading

clnperez commented May 11, 2017

wking commented May 11, 2017 via email

wking commented May 11, 2017

dqminh May 11, 2017

dqminh May 11, 2017

clnperez May 12, 2017

dqminh May 11, 2017

dqminh May 11, 2017

clnperez commented May 12, 2017

clnperez commented May 15, 2017

clnperez commented May 15, 2017

clnperez commented May 15, 2017

wking commented May 15, 2017 via email

clnperez commented May 16, 2017

clnperez commented May 22, 2017

crosbymichael commented May 22, 2017

crosbymichael May 22, 2017

crosbymichael May 22, 2017

clnperez May 22, 2017

crosbymichael May 22, 2017

clnperez May 22, 2017

clnperez commented May 22, 2017

clnperez commented May 25, 2017

crosbymichael commented May 25, 2017 •

edited by caniszczyk

Loading

dqminh commented May 26, 2017 •

edited by caniszczyk

Loading

Move libcontainer to x/sys/unix #1442

Move libcontainer to x/sys/unix #1442

Conversation

clnperez commented May 9, 2017

clnperez commented May 9, 2017 • edited Loading

clnperez commented May 9, 2017

clnperez commented May 9, 2017

Choose a reason for hiding this comment

clnperez May 11, 2017 • edited Loading

Choose a reason for hiding this comment

clnperez commented May 11, 2017

wking commented May 11, 2017 via email

wking commented May 11, 2017

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

clnperez commented May 12, 2017

clnperez commented May 15, 2017

clnperez commented May 15, 2017

clnperez commented May 15, 2017

wking commented May 15, 2017 via email

clnperez commented May 16, 2017

clnperez commented May 22, 2017

crosbymichael commented May 22, 2017

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

clnperez commented May 22, 2017

clnperez commented May 25, 2017

crosbymichael commented May 25, 2017 • edited by caniszczyk Loading

dqminh commented May 26, 2017 • edited by caniszczyk Loading

clnperez commented May 9, 2017 •

edited

Loading

clnperez May 11, 2017 •

edited

Loading

crosbymichael commented May 25, 2017 •

edited by caniszczyk

Loading

dqminh commented May 26, 2017 •

edited by caniszczyk

Loading