Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include Endpoint List for Shared Endpoints #33662

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

madhanrm
Copy link
Contributor

@madhanrm madhanrm commented Jun 14, 2017

Signed-off-by: Madhan Raj Mookkandy madhanm@microsoft.com

- What I did
Include endpoint Id for shared container support to be passed down to the platform
- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@madhanrm
Copy link
Contributor Author

Ping @msabansal @jhowardmsft

@madhanrm
Copy link
Contributor Author

ping @mavenugo

@madhanrm madhanrm changed the title (1) Include Endpoint List for Shared Endpoints Include Endpoint List for Shared Endpoints Jun 16, 2017
@cpuguy83
Copy link
Member

Can you explain what this is for us mere mortals :)

@madhanrm
Copy link
Contributor Author

For providing end to end networking functionality in windows, for option --net=container:, the list of endpoints of the primary or referred container has to be passed on to the platform, so that it can replicate some of the required registry keys onto the containers. For this specific work, a required registry related to Tcpip is mirrored to make sure the same DNS servers are configured on the new containers that is just created. More could be done with this information.

Copy link
Member

@lowenna lowenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The docker-py failure is not related to this - see #33757

@madhanrm
Copy link
Contributor Author

Added another error check to throw meaningful error, when trying to create a container with shared network of another container, which is running with hyperv isolation as platform doesn't support this scenario

@@ -109,6 +109,7 @@ type Container struct {

// Fields here are specific to Windows
NetworkSharedContainerID string
SharedEndpointList []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like either of these fields need to be persisted on the container object. Can make sure the fields aren't marshaled to json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 Can you point me out where the marshaled json is used & why do you think the windows specific fields are not needed. I am just trying to understand the use case, so that we do not miss anything for windows.
@jhowardmsft Is it safe to make this change proposed by @cpuguy83

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because these are populated only for startup (IIRC) and not generally needed across daemon restarts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpuguy83 Done

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lowenna
Copy link
Member

lowenna commented Jul 6, 2017

CI failures are real:

11:46:39 These files are not properly gofmt'd:
11:46:39  - container/container.go
11:46:39  - daemon/container_operations.go
11:46:39 
11:46:39 Please reformat the above files using "gofmt -s -w" and commit the result.

Do not allow sharing of container network with hyperv containers

Signed-off-by: Madhan Raj Mookkandy <madhanm@microsoft.com>
@lowenna
Copy link
Member

lowenna commented Jul 6, 2017

LGTM. Moving to status 4/merge

@yongtang
Copy link
Member

yongtang commented Jul 7, 2017

All Jenkins tests passes after windowsRS1 rerun. Merging...

@yongtang yongtang merged commit 9aecbbf into moby:master Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants