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.

Sign up for GitHub

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

Jump to bottom

Fix an issue where settings were not taking effect #197

Closed
wants to merge 1 commit into from
Closed

Fix an issue where settings were not taking effect #197

wants to merge 1 commit into from

Conversation

achanda
Copy link
Contributor

@achanda achanda commented Oct 2, 2015

catsby
catsby reviewed Oct 2, 2015
View reviewed changes
client/driver/docker.go
mode, ok := task.Config["network_mode"]
if !ok || mode == "" {
// docker default
logger.Printf("[WARN] driver.docker: no mode specified for networking, defaulting to bridge")
Copy link
Contributor

Choose a reason for hiding this comment

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

"defaulting to bridge" should probably change to "using Docker default" or something, since we're no longer actually specifying bridge here

@catsby
Copy link
Contributor

catsby commented Oct 2, 2015

This looks fine to me, other than a comment. I'm curious, re: #196, do you recall (or can you reproduce) which message was logged? Looks like one of these:

  • "[WARN] driver.docker: no mode specified for networking, defaulting to bridge"
  • "[DEBUG] driver.docker: using %s as network mode", mode
  • "[WARN] invalid setting for network mode %s, defaulting to bridge mode on docker0", mode

Knowing which one, and what specific mode you set, may shed some clues as to why this change is necessary.

@achanda
Copy link
Contributor Author

achanda commented Oct 2, 2015

@catsby the second one was being printed with mode set to whatever I set in the input file. But when I do a docker inspect on the launched container, mode is always default. I dumped the HostConfig struct right after the assignment and it has correct value. I thought the library might be misbehaving, which turned out to be false. Not sure why was it not working.

Fix an issue where settings were not taking effect
49874be 
- This fixes #196
- Also delegates default mode selection to docker
dadgar
dadgar reviewed Oct 2, 2015
View reviewed changes
client/driver/docker.go
@@ -76,6 +76,22 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
func createHostConfig(task *structs.Task) *docker.HostConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass the logger into this method.

@dadgar
Copy link
Contributor

dadgar commented Oct 2, 2015

Please also run go-fmt on the code.

ryanuber
ryanuber reviewed Oct 2, 2015
View reviewed changes
client/driver/docker.go
logger.Printf("[DEBUG] driver.docker: using %s as network mode", mode)
default:
logger.Printf("[WARN] invalid setting for network mode %s, defaulting to bridge mode on docker0", mode)
mode = "default"
Copy link
Member

Choose a reason for hiding this comment

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

We should either set the mode to "bridge", or change the message to say "using default network mode on docker0".

@achanda
Copy link
Contributor Author

achanda commented Oct 2, 2015

@ryanuber @dadgar @catsby I think I figured out why my original patch did not work. createHostConfig is being called twice in https://github.com/hashicorp/nomad/blob/master/client/driver/docker.go The second call does not take into account my changes for network mode, hence we always get the default HostConfig. Changing line 270 in that file to err = client.StartContainer(container.ID, container.HostConfig) fixes this. This will also fix #185

@achanda
Copy link
Contributor Author

achanda commented Oct 2, 2015

I will close this one and fix this in another PR.

@achanda achanda closed this Oct 2, 2015
@achanda achanda deleted the docker_net branch October 2, 2015 17:39
@github-actions
Copy link

github-actions bot commented May 9, 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 May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docker: setting network mode is not working
4 participants
@achanda @catsby @dadgar @ryanuber