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

amitkris · 2016-06-09T07:10:25Z

This PR will add support for a subset docker sub commands on Solaris.
This PR looks to add support for the following subcommands and explicitly disallow the rest on Solaris.

Supported subcommands:
attach, build, commit, cp, create, diff, events ,export, history, images, import, info, inspect, kill , load, login, logout, logs , network(null and bridge driver only), port, ps, pull, push, rename, restart , rm, rmi, run, save, search, start, stop, tag, top, version, wait

Unsupported commands:
exec, pause, stats, service, unpause, update, volume

TODO:

Merge and vendor containerd support docker/containerd#261
Merge and vendor libnetwork support docker/libnetwork#1225
Enable tests (unit tests done, integration tests will be a follow on effort)
Vendor sys/unix in the Engine for libcontainerd/process_unix.go
Merge and vendor support for github.com/google/certificate-transparency/go/x509/ Add solaris to builds in root_unix.go google/certificate-transparency#1321

Signed-off-by: Amit Krishnan [email protected]

justincormack · 2016-06-09T08:48:19Z

The CI is complaining about some Go linting issues, might be good to clean up.

thaJeztah · 2016-06-09T08:58:52Z

08:18:29 # github.com/docker/docker/libcontainerd
08:18:29 libcontainerd/client_linux.go:4: imported and not used: "encoding/json"
08:18:29 libcontainerd/client_linux.go:7: imported and not used: "path/filepath"
08:18:29 libcontainerd/client_linux.go:9: imported and not used: "sync"
08:18:29 libcontainerd/client_linux.go:14: imported and not used: "github.com/docker/docker/pkg/idtools"
08:18:53 Build step 'Execute shell' marked build as failure

thaJeztah · 2016-07-29T11:15:19Z

ping @amitkris can you rebase? 1.12 is released, so we can begin moving this forward

amitkris · 2016-07-29T16:47:49Z

@thaJeztah thats great! thanks for the update. will ping here when done.

amitkris · 2016-08-09T06:53:55Z

ping @thaJeztah : rebased changes, its ready to be reviewed.

lowenna · 2016-09-15T18:44:43Z

daemon/config_windows.go

Hmmm. There is no runc on Windows....

@jhowardmsft : this change was introduced because the stock runtime was being initialized to runc for all platforms and we wanted it to be runz for Solaris. Due to changes that have come about since then this is no longer required, and the logic can go back to what it was.
I'll revert this change.

justincormack · 2016-11-02T11:53:36Z

container/container_notsolaris.go

This only actually builds on Linux, FreeBSD does not have unix.MNT_DETACH, so looks like this should be container_linux and FreeBSD and Solaris can share the other file.

If you don't want to change this, feel free to leave it and I will try to tidy up the mess that is our bitrotted FreeBSD support later...

I've changed the files to container_linux.go and container_notlinux.go

justincormack · 2016-11-02T11:57:43Z

daemon/cluster/listen_addr.go

this should be split out into a conditionally compiled file ideally

This was necessitated because the net package isn't fully threshed out on Solaris.
We have to shell out to the CLI in the interim via this function and it will be removed soon.
Since its temporary I thought it would be best to keep it in the common file for now as opposed to making it platform specific and stubbing it out for all the platforms.
I can change it if you feel strongly about it..

no fine to keep it temporarily

justincormack · 2016-11-02T12:00:57Z

daemon/commit.go

It would be nicer to refactor this as a set of daemon feature flags eg daemon.feature.commitRunning = false and test these rather than OS conditionals, then collect them all together in a per OS file.

I'm not sure thats required for one feature?
A multi-platform check such as this is done only in this file (all others are in test files and not feature specific). If such checks show up for multiple features in the future I'd be happy to implement that. For now I think its simplest this way.

This is not the only feature, there are more, but happy if we fix later.

As a matter of principle, we should not have references to runtime anywhere. These are static features.

justincormack · 2016-11-02T12:05:10Z

daemon/daemon_solaris.go

Not sure about ignoring these values, I think it might be better to error and fail.

Thats a good point. Some of the features here have been stubbed pending certain minor changes in Solaris OS code which are being worked upon. Once we have a fully threshed out list of what we're looking to support going forward we will error out on those unsupported.
So until then we'd like to keep it this way.

justincormack · 2016-11-02T12:07:34Z

hack/make/cross

Confusing comment, do you mean "Solaris cannot be cross built"?

Yes. I've updated the comment.

justincormack · 2016-11-02T12:10:47Z

pkg/archive/archive_test.go

Are you sure? The man page says that would be a 1 byte blocksize! bs=1024 should work for all platforms.

great catch! thanks! I've updated this as you suggested.

justincormack · 2016-11-02T12:14:03Z

@amitkris hi, have looked through this and added some comments. Generally I would be happy if most/all of them were addressed in followup PRs instead and I think it would be ok to merge as is.

amitkris · 2016-11-02T17:55:41Z

@justincormack : Hi! Thank you for the comments :)
I've addressed some of your comments and replied to the rest. I've added an additional changeset with the changes. Please let me know what you think.

thaJeztah · 2016-11-03T18:42:27Z

@amitkris looks like it needs a rebase

amitkris · 2016-11-03T21:41:40Z

@thaJeztah : done

amitkris · 2016-11-03T23:20:41Z

thanks @tiborvass !

thaJeztah · 2016-11-03T23:30:11Z

oh, @amitkris is going to squash the last commit before merging, so hold your horses 😄

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

amitkris · 2016-11-04T00:18:08Z

thanks @justincormack! squashing the topmost changeset now.

…laris support. This is because the Solaris platform does not support the syscall package. Note that while Solaris supports all the common functionality in sys/unix there are platform specific features (for eg. MNT_DETACH) which are not implemented for the Solaris platform. Signed-off-by: Amit Krishnan <[email protected]>

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

thaJeztah · 2016-11-04T05:53:01Z

Rebased/Carried in #28056

tiborvass · 2016-11-04T16:53:15Z

In that case I'm closing this one.

GordonTheTurtle added the status/0-triage label Jun 9, 2016

amitkris force-pushed the solaris-1.12 branch 3 times, most recently from 49e2401 to bd8a403 Compare June 9, 2016 07:36

thaJeztah added status/1-design-review and removed status/0-triage labels Jun 9, 2016

amitkris force-pushed the solaris-1.12 branch 3 times, most recently from eaceb51 to d9700d8 Compare June 10, 2016 07:50

amitkris force-pushed the solaris-1.12 branch from d9700d8 to 7e2975f Compare June 17, 2016 01:43

tiborvass added the status/needs-attention Calls for a collective discussion during a review session label Jun 20, 2016

thaJeztah added this to the 1.13.0 milestone Jun 20, 2016

amitkris force-pushed the solaris-1.12 branch from 7e2975f to 19a5878 Compare June 28, 2016 20:52

amitkris force-pushed the solaris-1.12 branch 4 times, most recently from 8248e41 to 14328d9 Compare August 9, 2016 04:31

amitkris force-pushed the solaris-1.12 branch 2 times, most recently from 340013d to 0a3df6b Compare August 17, 2016 23:58

amitkris changed the title ~~[WIP] Add funtional support for Docker sub commands on Solaris~~ Add funtional support for Docker sub commands on Solaris Aug 24, 2016

amitkris force-pushed the solaris-1.12 branch 2 times, most recently from db9feb6 to 86e14f9 Compare September 3, 2016 23:24

amitkris force-pushed the solaris-1.12 branch from 86e14f9 to de8eee1 Compare September 12, 2016 23:02

thaJeztah changed the title ~~Add funtional support for Docker sub commands on Solaris~~ Add functional support for Docker sub commands on Solaris Sep 15, 2016

lowenna reviewed Sep 15, 2016

View reviewed changes

thaJeztah added impact/changelog and removed impact/changelog labels Nov 1, 2016

justincormack reviewed Nov 2, 2016

View reviewed changes

amitkris force-pushed the solaris-1.12 branch from 7c28337 to 99c2fa0 Compare November 2, 2016 18:00

amitkris force-pushed the solaris-1.12 branch from 99c2fa0 to 6a20f74 Compare November 3, 2016 19:07

tiborvass approved these changes Nov 3, 2016

View reviewed changes

justincormack approved these changes Nov 4, 2016

View reviewed changes

Add funtional support for Docker sub commands on Solaris

d333bb7

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

amitkris added 3 commits November 3, 2016 17:18

Vendor in libnetwork/drivers/solaris/bridge

e1022f7

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

Uprev github.com/tonistiigi/fifo/fifo.go

39c0636

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

amitkris force-pushed the solaris-1.12 branch from 6a20f74 to 39c0636 Compare November 4, 2016 00:20

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

LK4D4 mentioned this pull request Nov 4, 2016

Add functional support for Docker sub commands on Solaris #28056

Merged

tiborvass closed this Nov 4, 2016

Conversation

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

Uh oh!

Uh oh!

justincormack commented Jun 9, 2016

Uh oh!

thaJeztah commented Jun 9, 2016

Uh oh!

thaJeztah commented Jul 29, 2016

Uh oh!

amitkris commented Jul 29, 2016

Uh oh!

amitkris commented Aug 9, 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!

amitkris Nov 2, 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!

justincormack commented Nov 2, 2016

Uh oh!

amitkris commented Nov 2, 2016

Uh oh!

thaJeztah commented Nov 3, 2016

Uh oh!

amitkris commented Nov 3, 2016

Uh oh!

amitkris commented Nov 3, 2016

Uh oh!

thaJeztah commented Nov 3, 2016

Uh oh!

amitkris commented Nov 4, 2016

Uh oh!

thaJeztah commented Nov 4, 2016

Uh oh!

tiborvass commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

amitkris commented Jun 9, 2016 •

edited

Loading

amitkris Nov 2, 2016 •

edited

Loading