Added support for dns-search and fixes #30102 by msabansal · Pull Request #30117 · moby/moby

msabansal · 2017-01-13T05:59:25Z

- What I did
This PR adds support for --dns-search option for windows. It also fixes #30102 Support for dns-search requires platform support and will only work with the next version of windows.

- How I did it

- How to verify it
Verified the bug fix using the steps Jason mentioned and also validated that dns-suffix is set on the latest builds.
- Description for the changelog

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

msabansal · 2017-01-13T06:07:12Z

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

lowenna · 2017-01-13T17:50:01Z

daemon/container_operations.go

Can remove the else here and just have a second if-statement

lowenna · 2017-01-13T17:52:28Z

vendor.conf

Can we get this tagged with a version?

v0.5.10 just released

lowenna · 2017-01-13T17:54:17Z

vendor/github.com/Microsoft/hcsshim/interface.go

Just realized - this should be omitempty. Sorry 😇

Updating hcsshim

lowenna · 2017-01-13T17:56:06Z

vendor/github.com/Microsoft/hcsshim/interface.go

(Nit, not sure if it makes sense), but consider updating the example at the top of libcontainerd/client_windows.go to include this.

lowenna · 2017-01-13T17:59:43Z

daemon/daemon_windows.go

This change looks unrelated. Could you update the comment (or is that still accurate?)

The comment includes that it fixes #30102. Should I do a separate PR for that?

Ah, I was referring to the comment immediate above this line of code.

lowenna · 2017-01-13T18:02:00Z

libcontainerd/types_windows.go

Could/should lthis be combined with the existing NetworkEndpointsOption to avoid more proliferations to this pattern of side-band options? Not a problem if it doesn't make sense, just asking the question 😇

Yes it can be. I just thought It would be better to have it as a separate option. I will combine them

Thanks - yes, I'm trying to figure out how to get rid of side-band options and have everything in OCI.

lowenna · 2017-01-13T19:04:28Z

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

I think that would probably be a good idea.

lowenna · 2017-01-13T19:04:43Z

Does this also require any doc updates?

Signed-off-by: msabansal <[email protected]>

Signed-off-by: Sandeep Bansal <[email protected]>

msabansal · 2017-01-13T20:15:24Z

@jhowardmsft Not sure if it requires a doc update. Perhaps we can add somewhere that dns-search is only supported for rs2. Since we are now throwing an error it should be alright.

lowenna · 2017-01-13T20:26:31Z

LGTM. Thanks for doing the updates @msabansal

johnstep

LGTM

It might be better to separate the fix for #30102, though.

johnstep · 2017-01-13T20:30:46Z

daemon/daemon_windows.go

+		// encountered if it doesn't already exist
+		if !defaultNetworkExists &&
+			runconfig.DefaultDaemonNetworkMode() == containertypes.NetworkMode(strings.ToLower(v.Type)) &&
+			n == nil {


Maybe check n == nil first, or at least second.

ping @msabansal

msabansal · 2017-01-21T04:14:31Z

@jhowardmsft @johnstep Can we merge or do I split these?

The test failures don't look related to this change. Can we rerun or should I triage?

LK4D4 · 2017-01-30T21:42:12Z

@msabansal looks like @johnstep has minor comment for you

vdemeester · 2017-01-31T10:05:09Z

@LK4D4 code looks good and the comment is really a nit. Let's merge this and do a quick follow-up 👼

Reuse existing structures and rely on json serialization to deep copy Container objects. Also consolidate all "save" operations on container.CheckpointTo, which now both saves a serialized json to disk, and replicates state to the ACID in-memory store. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit edad527) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/container_operations.go: missing moby#30117 - daemon/daemon.go: missing moby#32821 - daemon/list.go: missing moby#27557, moby#33241 etc.

Fix a deadlock caused by re-entrant locks on container objects. Signed-off-by: Fabio Kung <[email protected]> (cherry picked from commit 37addf0) Signed-off-by: Kir Kolyshkin <[email protected]> Conflicts: - daemon/container_operations.go: missing moby#30117

GordonTheTurtle added the status/0-triage label Jan 13, 2017

msabansal changed the title ~~Added support for dns-suffix and fixes #30102~~ Added support for dns-search and fixes #30102 Jan 13, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Jan 13, 2017

vdemeester assigned johnstep and lowenna and unassigned johnstep Jan 13, 2017

vdemeester requested a review from johnstep January 13, 2017 09:52

lowenna suggested changes Jan 13, 2017

View reviewed changes

lowenna added status/2-code-review and removed status/1-design-review labels Jan 13, 2017

msabansal and others added 2 commits January 13, 2017 12:01

Added support for dns-search and fixes #30102

e696248

Signed-off-by: msabansal <[email protected]>

Vendoring latest hcsshim

0f25dc2

Signed-off-by: Sandeep Bansal <[email protected]>

lowenna approved these changes Jan 13, 2017

View reviewed changes

johnstep approved these changes Jan 13, 2017

View reviewed changes

vdemeester added the rebuild/janky label Jan 23, 2017

GordonTheTurtle removed the rebuild/janky label Jan 23, 2017

vdemeester added the rebuild/experimental label Jan 23, 2017

GordonTheTurtle removed the rebuild/experimental label Jan 23, 2017

vdemeester approved these changes Jan 31, 2017

View reviewed changes

vdemeester merged commit c0a1d2e into moby:master Jan 31, 2017

GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017

thaJeztah added the impact/changelog label Feb 1, 2017

kolyshkin mentioned this pull request May 2, 2020

backport: No container locks on docker ps projectatomic/docker#375

Merged

Conversation

msabansal commented Jan 13, 2017

Uh oh!

msabansal commented Jan 13, 2017

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!

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!

lowenna commented Jan 13, 2017

Uh oh!

lowenna commented Jan 13, 2017

Uh oh!

msabansal commented Jan 13, 2017

Uh oh!

lowenna commented Jan 13, 2017

Uh oh!

johnstep left a comment

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!

msabansal commented Jan 21, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

vdemeester commented Jan 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants