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

Add network_aliases for docker driver #1980

Merged
merged 1 commit into from
Dec 19, 2016
Merged

Conversation

dmexe
Copy link
Contributor

@dmexe dmexe commented Nov 11, 2016

#1948 Add config.network_aliases key. PL doesn't have tests because fsouza/go-dockerclient doesn't provide query api for aliases.

EndpointsConfig: make(map[string]*docker.EndpointConfig),
}

if len(driverConfig.NetworkAliases) > 0 && hostConfig.NetworkMode != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the need to gate on && hostConfig.NetworkMode != ""?

@dadgar
Copy link
Contributor

dadgar commented Nov 14, 2016

@dmexe dmexe force-pushed the network-aliases branch 6 times, most recently from 93ceb0f to 693dbc2 Compare November 14, 2016 21:56
@dmexe
Copy link
Contributor Author

dmexe commented Nov 14, 2016

@dadgar done, please review

@jorgemarey
Copy link
Contributor

Is it possible/interesting to add the task name as a default network alias?

}

if len(driverConfig.NetworkAliases) > 0 {
endpointNetworkName := hostConfig.NetworkMode
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed? On Line 729 hostConfig.NetworkMode is set some default.

if hostConfig.NetworkMode == "" {
    // docker default
    d.logger.Printf("[DEBUG] driver.docker: networking mode not specified; defaulting to %s", defaultNetworkMode)
    hostConfig.NetworkMode = defaultNetworkMode
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if endpointNetworkName == "" {
endpointNetworkName = "default"
}
networkingConfig.EndpointsConfig[endpointNetworkName] = &docker.EndpointConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that set aliases on defaultNetworkMode, bridge, is not allowed.

@dmexe
Copy link
Contributor Author

dmexe commented Nov 16, 2016

Is it possible/interesting to add the task name as a default network alias?

Network aliases only allowed for user defined networks, not for docker predefined networks, eg

network_mode = "bridge"
network_aliases = ["foobar"]

will raise an error.

If nomad adds default aliases for default networks bridge, host a job will fail. In theory, predefined networks may be detected by their name, but docker internals isn't pretty stable and sometimes changing.

@dmexe
Copy link
Contributor Author

dmexe commented Dec 19, 2016

@jorgemarey Is there any news for this pull request?

@dadgar
Copy link
Contributor

dadgar commented Dec 19, 2016

Thanks! Great work!

@dadgar dadgar merged commit feb6f53 into hashicorp:master Dec 19, 2016
@seifer
Copy link

seifer commented Jan 23, 2017

Great improvement. When it will be released?

@dmexe dmexe deleted the network-aliases branch April 20, 2017 16:47
@github-actions
Copy link

github-actions bot commented Apr 2, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants