Add functional support for Docker sub commands on Solaris by LK4D4 · Pull Request #28056 · moby/moby

LK4D4 · 2016-11-04T01:55:19Z

This is the carry of #23397 to free @amitkris from hell of a rebase. That PR was in merged state, so we can merge this when CI will pass.
Closes #23397

LK4D4 · 2016-11-04T04:53:29Z

@amitkris would you mind to try before merge?

amitkris · 2016-11-04T05:48:21Z

@LK4D4 : Thank you so much for doing this!
How do I clone your branch so that I can quickly test the changes?

thaJeztah · 2016-11-04T06:24:16Z

@amitkris easy way; install GitHub hub; https://hub.github.com / https://github.com/github/hub

Or manually; https://gist.github.com/piscisaureus/3342247

amitkris · 2016-11-04T09:00:38Z

@thaJeztah thanks!

@LK4D4 : There were two things that need to be modified for the above changeset to work on Solaris.
In #27912 in hack/make.sh https://github.com/docker/docker/blob/master/hack/make.sh#L104
was modified
from : export GOPATH="${PWD}/.gopath:${PWD}/vendor"
to : export GOPATH="${PWD}/.gopath"
We build with AUTO_GOPATH=1 on Solaris and we need $(PWD)/vendor to be in the GOPATH to build the source. Can this be added back? It can even be in the Solaris specific if statement that follows this line if it is not desired as a general setting for the GOPATH.

Again in #27912 there is a difference in the version number of the fsnotify package that was previously in hack/vendor.sh and that was added in vendor.conf
hack/vendor.sh
clone git gopkg.in/fsnotify.v1 v1.2.11
vendor.conf
github.com/fsnotify/fsnotify v1.2.1
v1.2.11 of fsnotify contains Solaris support. Could we please restore this to v1.2.11?

With these two changes this changeset builds and runs on Solaris as expected. Thank you once again for helping me out with this rebase!

LK4D4 · 2016-11-04T15:18:54Z

@amitkris actually AUTO_GOPATH should work. vendor directory is no more "gopath" on itself.
I'll update fsnotify now.

LK4D4 · 2016-11-04T15:27:58Z

@amitkris updated fsnotify. Now we need to figure out stuff with AUTO_GOPATH. Pls ping me in IRC if you have time

amitkris · 2016-11-04T18:27:14Z

Sorted out all outstanding issues with @LK4D4 offline.
The functionality works as expected on Solaris now.
LGTM! :D

LK4D4 · 2016-11-04T21:09:27Z

@mlaventure has some concerns about this.

mlaventure · 2016-11-04T20:55:59Z

daemon/cluster/listen_addr.go

I'd rather have listSystemIPs() be in a platform specific file than making use of runtime.GOOS.

mlaventure · 2016-11-04T21:02:29Z

daemon/container_operations_solaris.go

It's a copy of the version in container_operations_unix.go, could it be moved to a common files to avoid duplications? We'd definitely forget to update the solaris version if we ever had to make changes to this routine.

mlaventure · 2016-11-04T21:04:45Z

daemon/container_operations_solaris.go

The only difference here is the lack of call to mount.MakeRPrivate could we just have a stub for solaris in pkg/mount/sharedsubtree_solaris.go?

edit: And I just saw that this file actually exists :)

mlaventure · 2016-11-04T21:06:45Z

daemon/daemon_solaris.go

Wrong comment format

mlaventure · 2016-11-04T21:07:40Z

daemon/daemon_solaris.go

remove the \n

mlaventure · 2016-11-04T21:40:40Z

daemon/start.go

remove \n

mlaventure · 2016-11-04T21:43:07Z

daemon/start.go

Currently on Linux this just fails when we try to actually run the image, wouldn't the same happen on Solaris?

Also, this probably make more sense in the containerCreate side of things, might as well fails as early as possible

mlaventure · 2016-11-04T21:45:35Z

libcontainerd/client_solaris.go

defer should be just after the Lock

mlaventure · 2016-11-04T21:48:31Z

libcontainerd/remote_unix.go

runtime == runc? I thought it was named runz? :)

Also this can be removed, if the user specify an invalid runtime this will fails and that's the expected behavior.

same as above. For maximum reutilization of common code and due to the stockRuntimeName issues we're sticking to runc in the engine for Soalris.

mlaventure · 2016-11-04T21:49:46Z

libcontainerd/remote_unix.go

nit: I usually prefer having a dedicated file with a stub than checking GOOS. Easier to maintain and expand IMHO.

mlaventure · 2016-11-04T22:11:38Z

In which case the code can stay unmodified.

If the user explicitly specify a non compatible runtime, it's up to him to
fix it, the default would still work.

On Fri, Nov 4, 2016 at 3:08 PM Amit Krishnan [email protected]
wrote:

@amitkris commented on this pull request.

In libcontainerd/remote_unix.go
#28056:
    "--metrics-interval=0",
  "--start-timeout", "2m",
  "--state-dir", filepath.Join(r.stateDir, containerdStateDir),
}
if r.runtime != "" {
args = append(args, "--runtime")
args = append(args, r.runtime)
if goruntime.GOOS == "solaris" {
args = append(args, "--shim", "containerd-shim", "--runtime", "runc")
same as above. For maximum reutilization of common code and due to the
stockRuntimeName issues we're sticking to runc in the engine for Soalris.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28056, or mute the thread
https://github.com/notifications/unsubscribe-auth/APUifizBGK78fCmuQCsvCFKX8wU-h_j8ks5q66zugaJpZM4KpHYy
.

LK4D4 · 2016-11-05T23:34:27Z

I've fixed some nits, but splitting to files is still problematic for me. Because solaris uses many functions from _unix.go files which are also used for linux and freebsd, but some of them should be split to some OTHER file for linux and freebsd and I don't know how to call it(linux_freebsg.go? unix_not_solaris.go?).

LK4D4 · 2016-11-05T23:41:31Z

hack/make/test-unit

@amitkris shouldn't we hide tests behind tag? This really looks bad.

@LK4D4: My only intention there was to get daemon/graphdriver tests to not run for Solaris. We support only zfs and running the other graph plugins doesn't make sense.
With my limited understanding of hack/make/test-unit this seemed to be the simplest way to do it.
Any logic that would help accomplish this better is fine.

ping @tiborvass @mlaventure

@amitkris let me know which tests fail for you. They all should be behind linux tag.

@LK4D4 : everything in graphdriver except zfs fails for Solaris.

@amitkris What the errors? All graphdrivers behind linux build tag.

@LK4D4 : sorry misunderstood your question the first time. The issue is with graphtest/graphtest_unix.go. Enabling that for Solaris calls into devmapper for some reason and that fails and then terminates the entire test run. Please remove the Solaris build tag from graphtest_unix.go for now. I will investigate and fix that in a follow up.
Once the solaris build tag is removed frm that file hack/make/test-unit can go back to the way it was.

Thanks for explanation.

LK4D4 · 2016-11-05T23:41:56Z

hack/make/tgz

@amitkris can you describe more

@LK4D4 : The libnetwork changes that I vendored in with my original PR were being called only from Solaris files. This caused an issue with the vendor CI which being run on a linux box did not detect the dependencies in use by Solaris files.
To fix this I added the solaris/amd64 tag in vendor. As a result of this make/cross tried to build Solaris binaries and failed due to the CGO calls which call into Solaris specific header files.
So to make the vendor CI and make/cross pass we added bypassed the cross build for Solaris with the above elif continue.

LK4D4 · 2016-11-05T23:42:32Z

Also, I've rebased on master. So, now changes are way smaller.

mlaventure · 2016-11-06T02:31:13Z

It doesn't build

LK4D4 · 2016-11-07T16:04:45Z

By the way it's common problem with files. You can find examples of duplication in net standard package :(

mlaventure · 2016-11-07T16:59:49Z

I'll open an issue for tracking the deduplication as far as Solaris is concerned then so this can be moved along.

LK4D4 · 2016-11-07T17:03:22Z

@mlaventure btw, I think I've fixed duplication issues which you mentioned.

Signed-off-by: Amit Krishnan <[email protected]> Signed-off-by: Alexander Morozov <[email protected]>

tiborvass · 2016-11-08T00:15:04Z

LGTM

amitkris · 2016-11-08T16:46:32Z

thank you @LK4D4 @thaJeztah @mlaventure @tiborvass! :D

GordonTheTurtle added the status/0-triage label Nov 4, 2016

LK4D4 added status/4-merge and removed status/0-triage labels Nov 4, 2016

thaJeztah added this to the 1.13.0 milestone Nov 4, 2016

thaJeztah mentioned this pull request Nov 4, 2016

Add functional support for Docker sub commands on Solaris #23397

Closed

5 tasks

LK4D4 added status/2-code-review and removed status/4-merge labels Nov 4, 2016

LK4D4 mentioned this pull request Nov 4, 2016

vendor: update package for solaris support #28081

Merged

mlaventure suggested changes Nov 4, 2016

View reviewed changes

LK4D4 commented Nov 5, 2016

View reviewed changes

mlaventure mentioned this pull request Nov 7, 2016

Move duplicated unix code into a common file #28129

Closed

Add functional support for Docker sub commands on Solaris

934328d

Signed-off-by: Amit Krishnan <[email protected]> Signed-off-by: Alexander Morozov <[email protected]>

mlaventure approved these changes Nov 7, 2016

View reviewed changes

tiborvass merged commit 109c26b into moby:master Nov 8, 2016

LK4D4 deleted the solaris_me branch November 8, 2016 19:16

Conversation

LK4D4 commented Nov 4, 2016

Uh oh!

LK4D4 commented Nov 4, 2016

Uh oh!

amitkris commented Nov 4, 2016

Uh oh!

thaJeztah commented Nov 4, 2016

Uh oh!

amitkris commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LK4D4 commented Nov 4, 2016

Uh oh!

LK4D4 commented Nov 4, 2016

Uh oh!

amitkris commented Nov 4, 2016

Uh oh!

LK4D4 commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure commented Nov 4, 2016

@amitkris commented on this pull request.

Uh oh!

LK4D4 commented Nov 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 5, 2016

Uh oh!

mlaventure commented Nov 6, 2016

Uh oh!

LK4D4 commented Nov 7, 2016

Uh oh!

mlaventure commented Nov 7, 2016

Uh oh!

LK4D4 commented Nov 7, 2016

Uh oh!

tiborvass commented Nov 8, 2016

amitkris commented Nov 4, 2016 •

edited

Loading

mlaventure Nov 4, 2016 •

edited

Loading