libnetwork support for docker on Solaris by puneetpruthi · Pull Request #1225 · moby/libnetwork

puneetpruthi · 2016-06-08T03:09:58Z

This PR is targeting to add following libnetwork support for docker on Solaris:

Null network Driver
Bridge network Driver (w/ Port Mapping)
Unit tests for networks supported on Solaris

What this PR does not add support for:

docker network connect/disconnect operation(s)

The code changes for this PR are stable and open to review. The PR has been tested on Linux as well.

TODO:

Integration testing with docker-engine on Solaris
Merge vendor(s) dependency PR uprev docker/pkg/reexec and Azure/go-ansiterm for libnetwork #1315

Signed-off-by: Puneet Pruthi puneetpruthi@gmail.com

amitkris · 2016-06-08T18:16:05Z

@aboch: ping
This is a follow up to #1110. Could you take a look?

puneetpruthi · 2016-09-12T20:05:58Z

@aboch: Thanks for comments. Incorporated them and updated the branch.

aboch · 2016-09-14T17:10:06Z

drivers/solaris/bridge/bridge.go

+func (d *driver) createNetwork(config *networkConfiguration) error {
+	var err error
+
+	fmt.Println("Creating bridge network:", config.ID,


I think you can now replace the print calls with logrus ones.

aboch · 2016-09-14T17:18:07Z

A considerable amount of code is a duplicate from the current bridge driver implementation.
But at this stage, I think it is better this way, keep the two bridge drivers separated until the solaris one gets more testing. Minor comment and rest LGTM.

puneetpruthi · 2016-09-15T18:23:55Z

@aboch .. thanks for the comments. Re-synced the branch and replaced fmt.Printlns with logrus. I agree about the code duplication and had the same train of thought going ahead.
Let me know in case any more changes are required.

puneetpruthi · 2016-09-30T17:23:38Z

@aboch ... Can you please have a look if the PR needs anything else to merge ?

aboch · 2016-09-30T17:32:16Z

LGTM again

ping @mavenugo

mavenugo · 2016-10-13T18:34:31Z

@puneetpruthi IT is failing :( .... can you please rebase to the latest libnetwork which has the updated fix for the CI issue.

puneetpruthi · 2016-10-13T20:09:19Z

@mavenugo ... resynced and resolved the issues..

mavenugo

~~just 1 minor comment. LGTM otherwise.~~

mavenugo · 2016-10-13T20:29:46Z

store_test.go

 	return New(cfgOptions...)
 }

-func TestBoltdbBackend(t *testing.T) {


Curious to know why is this removed for solaris platform ?

@puneetpruthi ^^

@mavenugo ... libnetwork on Solaris does not support 'host' networking used by this test.

mavenugo · 2016-10-14T12:27:11Z

drivers/solaris/bridge/bridge.go

+	return nil
+}
+
+func (d *driver) RevokeExternalConnectivity(nid, eid string) error {


@aboch can u pls take a closer look at this API not being handled ?

Did not notice the programExternalConenctivity() is implemented and the revoke is not.
@puneetpruthi I know this PR has been open for long time and you kept on improving it, has the above asymmetry always been there ? Can you call the counterpart removePFRule() in RevokeExternalConnectivity as long with the releasePorts().

It might be an oversight on my part as we were not really using RevokeExternalConnectivity() to release the ports. Can add that. Thanks @aboch...
@mavenugo ... can I update the PR with this change ? Or should I wait for further comments ?

Thanks @puneetpruthi . pls get this comment addressed and I dont have any more comments

Thanks @mavenugo .... added another commit with the changes.

mavenugo · 2016-10-14T12:59:37Z

osl/sandbox_solaris.go

+
+// NewSandbox provides a new sandbox instance created in an os specific way
+// provided a key which uniquely identifies the sandbox
+func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error) {


@mrjana could you pls chime in on the implications of not implementing the NewSandbox API ?
I know some other platforms also take this route. But am curious here because I dont have a way currently to verify this change.

Yeah it's fine. Ideally we want every OS to implement this api but I understand that can be difficult to achieve in other OSes. But without being able to do this I am not sure how multiple networks can be attached to a container so that is an open question.

Thanks @mrjana

mavenugo · 2016-10-14T23:36:41Z

@puneetpruthi can u pls squash the changes ?

Signed-off-by: Puneet Pruthi <puneetpruthi@gmail.com>

puneetpruthi · 2016-10-14T23:38:31Z

@mavenugo ... done.

puneetpruthi · 2016-10-15T00:58:15Z

Thanks @mavenugo !

puneetpruthi force-pushed the solaris_integ branch 3 times, most recently from dcda412 to a290451 Compare June 8, 2016 04:06

GordonTheTurtle added dco/no labels Jun 8, 2016

puneetpruthi force-pushed the solaris_integ branch from 5dc5646 to 674d94c Compare June 8, 2016 04:57

GordonTheTurtle removed the dco/no label Jun 8, 2016

puneetpruthi force-pushed the solaris_integ branch 2 times, most recently from f33120e to 24257e4 Compare June 8, 2016 06:48

amitkris mentioned this pull request Jun 9, 2016

Add functional support for Docker sub commands on Solaris moby/moby#23397

Closed

5 tasks

puneetpruthi force-pushed the solaris_integ branch 2 times, most recently from 98c7882 to c69fbda Compare June 24, 2016 21:32

puneetpruthi force-pushed the solaris_integ branch 11 times, most recently from fbf377b to b96065c Compare July 9, 2016 00:20

puneetpruthi mentioned this pull request Jul 9, 2016

uprev docker/pkg/reexec and Azure/go-ansiterm for libnetwork #1315

Merged

puneetpruthi force-pushed the solaris_integ branch 2 times, most recently from 3486790 to b96065c Compare July 9, 2016 13:01

puneetpruthi force-pushed the solaris_integ branch from b96065c to 634da10 Compare August 1, 2016 20:39

puneetpruthi changed the title ~~[WIP] libnetwork support for docker on Solaris~~ libnetwork support for docker on Solaris Aug 1, 2016

puneetpruthi mentioned this pull request Aug 1, 2016

Adding null driver support for libnetwork on Solaris #1242

Closed

aboch reviewed Sep 14, 2016
View reviewed changes

puneetpruthi force-pushed the solaris_integ branch from e61a191 to 7d0499e Compare September 15, 2016 17:58

puneetpruthi force-pushed the solaris_integ branch 3 times, most recently from 670b903 to 192d69c Compare September 30, 2016 16:36

puneetpruthi force-pushed the solaris_integ branch from 192d69c to a134e79 Compare October 12, 2016 00:25

aboch added the status/2-needs-code-review label Oct 12, 2016

puneetpruthi force-pushed the solaris_integ branch from a134e79 to 805089e Compare October 13, 2016 19:53

mavenugo reviewed Oct 14, 2016

View reviewed changes

GordonTheTurtle added dco/no labels Oct 14, 2016

puneetpruthi force-pushed the solaris_integ branch from 41441af to f15210c Compare October 14, 2016 23:20

GordonTheTurtle removed dco/no labels Oct 14, 2016

mavenugo approved these changes Oct 14, 2016

View reviewed changes

puneetpruthi force-pushed the solaris_integ branch from f15210c to 566903d Compare October 14, 2016 23:37

libnetwork support for Solaris

566903d

Signed-off-by: Puneet Pruthi <puneetpruthi@gmail.com>

mavenugo merged commit fad68bd into moby:master Oct 15, 2016

Conversation

puneetpruthi commented Jun 8, 2016 • edited by aboch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitkris commented Jun 8, 2016

Uh oh!

puneetpruthi commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch commented Sep 14, 2016

Uh oh!

puneetpruthi commented Sep 15, 2016

Uh oh!

puneetpruthi commented Sep 30, 2016

Uh oh!

aboch commented Sep 30, 2016

Uh oh!

mavenugo commented Oct 13, 2016

Uh oh!

puneetpruthi commented Oct 13, 2016

Uh oh!

mavenugo left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavenugo commented Oct 14, 2016

Uh oh!

puneetpruthi commented Oct 14, 2016

Uh oh!

puneetpruthi commented Oct 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

puneetpruthi commented Jun 8, 2016 •

edited by aboch

Loading

mavenugo left a comment •

edited

Loading