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

Add container's short-id as default network alias #21901

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
8 participants
@mavenugo
Contributor

mavenugo commented Apr 9, 2016

- What I did
link feature in docker0 bridge by default provides short-id as a container alias. With built-in SD feature for user-defined networks, providing a container short-id as a network alias by default will fill that gap.

Without the short-id alias, compose like applications are forced to perform artificial steps such as -

  • create a container in a network & get the container-id
  • disconnect the container from the network
  • connect the same container again to the same network and this time with --alias={short-id}

By providing the short-id as a default alias (similar to links), we can avoid such artificial steps

This also helps with docker/libnetwork#996.

Not targeting this for 1.11 release

Signed-off-by: Madhu Venugopal madhu@docker.com

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Apr 9, 2016

Contributor

@bfirsh @dnephin @taylorb-microsoft @aboch FYI. since this is of interest to the compose + TP5 integration, I don't think this is of urgency for the 1.11 release.

Contributor

mavenugo commented Apr 9, 2016

@bfirsh @dnephin @taylorb-microsoft @aboch FYI. since this is of interest to the compose + TP5 integration, I don't think this is of urgency for the 1.11 release.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 9, 2016

Member

If you think it would be good for 1.11, let me know, it's a small diff.

Member

thaJeztah commented Apr 9, 2016

If you think it would be good for 1.11, let me know, it's a small diff.

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 11, 2016

Contributor

@thaJeztah I think @mavenugo is on vacation - could you potentially find someone else to carry it?

Contributor

friism commented Apr 11, 2016

@thaJeztah I think @mavenugo is on vacation - could you potentially find someone else to carry it?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 11, 2016

Member

@friism I don't think it needs a carry, just a review

Member

thaJeztah commented Apr 11, 2016

@friism I don't think it needs a carry, just a review

Show outdated Hide outdated daemon/container_operations.go Outdated
@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Apr 16, 2016

Contributor

@thaJeztah @calavera addressed your comments. PTAL

Contributor

mavenugo commented Apr 16, 2016

@thaJeztah @calavera addressed your comments. PTAL

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 17, 2016

Member

LGTM

Member

thaJeztah commented Apr 17, 2016

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 17, 2016

ping @jhowardmsft PTAL

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Apr 18, 2016

Contributor

Seems reasonable to me, but adding @msabansal to comment on networking from the windows side. Shame none of those tests actually run on Windows though :(

Contributor

jhowardmsft commented Apr 18, 2016

Seems reasonable to me, but adding @msabansal to comment on networking from the windows side. Shame none of those tests actually run on Windows though :(

shortID := stringid.TruncateID(container.ID)
for _, alias := range endpointConfig.Aliases {
if alias == shortID {
addShortID = false

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 18, 2016

Contributor

Can we break here?

@cpuguy83

cpuguy83 Apr 18, 2016

Contributor

Can we break here?

This comment has been minimized.

@mavenugo

mavenugo Apr 18, 2016

Contributor

yes. added.

@mavenugo

mavenugo Apr 18, 2016

Contributor

yes. added.

Add container's short-id as default network alias
link feature in docker0 bridge by default provides short-id as a
container alias. With built-in SD feature, providing a container
short-id as a network alias will fill that gap.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Apr 18, 2016

Contributor

@jhowardmsft @taylorb-microsoft @PatrickLang can you ask the Windows networking folks have a look at this? I haven't tested yet, but I believe it's what's gonna get compose v2 files working with windows

cc @mavenugo

Contributor

friism commented Apr 18, 2016

@jhowardmsft @taylorb-microsoft @PatrickLang can you ask the Windows networking folks have a look at this? I haven't tested yet, but I believe it's what's gonna get compose v2 files working with windows

cc @mavenugo

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Apr 18, 2016

Contributor

@msabansal ^^ (@friism, You want Sandeep for networking questions as I put above)

Contributor

jhowardmsft commented Apr 18, 2016

@msabansal ^^ (@friism, You want Sandeep for networking questions as I put above)

@msabansal

This comment has been minimized.

Show comment
Hide comment
@msabansal

msabansal Apr 18, 2016

Contributor

LGTM for Linux. Windows doesn't have name resolution support at this time. Once we bring in that support we should be able to support this.

Contributor

msabansal commented Apr 18, 2016

LGTM for Linux. Windows doesn't have name resolution support at this time. Once we bring in that support we should be able to support this.

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Apr 19, 2016

Contributor

@msabansal Just to be clear on the intention of this PR, it solves 2 problems -

  • Adds short-id alias by default to the name-resolution
  • Because the short-id alias is added by default, Compose V2 need not perform the disconnect/connect routine. Hence Compose V2 can be used in windows for TP5 (since windows network driver doesn't support disconnect / connect options).

We are looking for a 👍 from Windows side to make sure Compose V2 can be used (with docker/compose#3337) for TP5.

Contributor

mavenugo commented Apr 19, 2016

@msabansal Just to be clear on the intention of this PR, it solves 2 problems -

  • Adds short-id alias by default to the name-resolution
  • Because the short-id alias is added by default, Compose V2 need not perform the disconnect/connect routine. Hence Compose V2 can be used in windows for TP5 (since windows network driver doesn't support disconnect / connect options).

We are looking for a 👍 from Windows side to make sure Compose V2 can be used (with docker/compose#3337) for TP5.

@msabansal

This comment has been minimized.

Show comment
Hide comment
@msabansal

msabansal Apr 19, 2016

Contributor

@mavenugo I understand. I don't see any problem there. Some of the compose features will obviously not be supported for windows and we will add support as we progress.

Contributor

msabansal commented Apr 19, 2016

@mavenugo I understand. I don't see any problem there. Some of the compose features will obviously not be supported for windows and we will add support as we progress.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 19, 2016

Contributor

LGTM

Contributor

cpuguy83 commented Apr 19, 2016

LGTM

@cpuguy83 cpuguy83 merged commit 8adc8c3 into moby:master Apr 19, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 17725 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 4562 has succeeded
Details
janky Jenkins build Docker-PRs 26554 has succeeded
Details
userns Jenkins build Docker-PRs-userns 8763 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 25006 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 770 has succeeded
Details

@thaJeztah thaJeztah added this to the 1.12.0 milestone Apr 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment