Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Regression bug fix for pull request #3654#3728

Open
wzymaster wants to merge 1 commit intodocker-archive-public:masterfrom
wzymaster:master
Open

Regression bug fix for pull request #3654#3728
wzymaster wants to merge 1 commit intodocker-archive-public:masterfrom
wzymaster:master

Conversation

@wzymaster
Copy link
Copy Markdown
Contributor

To enable Docker Daemon running on private IP if environment variable - SOFTLAYER_DOCKER_ON_PRIVATE_IP is set to true.

@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:wzymaster/machine.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: 众商汇 <wzymaster@gmail.com>
@wzymaster
Copy link
Copy Markdown
Contributor Author

@nathanleclaire all checks are passed and please feel free to review it.

@nathanleclaire
Copy link
Copy Markdown
Contributor

Which issue does this fix? Seems a bit of a wide-ranging change for just a regression fix, no?

@wzymaster
Copy link
Copy Markdown
Contributor Author

wzymaster commented Sep 6, 2016

@nathanleclaire probably regression fix isn't a good description. I found that the final goal is to enable Docker Daemon running on Private IP and my previous pull request #3654 only does displaying part but doesn't do the real job.

@nathanleclaire
Copy link
Copy Markdown
Contributor

@wzymaster OK -- couple things:

  1. This (checking environment variable) will inevitably fail when it is not set (i.e., operations invoked which aren't create), you should be checking the value in the Driver struct which will be unmarshaled instead -- sorry, should have caught that in initial code review
  2. Why do a driver-specific check in the provisioner here? I don't like that -- ideally provisioner should be driver-agnostic and only require basics like SSH connection. Driver.GetURL is intended to return the "Docker URL" (e.g., <private-ip>:2376) so consider switching that out instead. However it's a bit tricky with all the dockerPort stuff.

@wzymaster
Copy link
Copy Markdown
Contributor Author

wzymaster commented Sep 6, 2016

@nathanleclaire

  1. In fact, if environment variable isn't set up, my existing codes will handle it well, like how SOFTLAYER_HOURLY_BILLINGis handled in the existing codes for now
  2. Over here, I want Docker Daemon to bind on private IP instead of '0.0.0.0' which is hard coded in the existing machine for all provisioners. I don't quite understand what you mean Driver.GetURL is related to this ? Is there a place to set something in the codes ?
[Service]
Type=notify
ExecStart=/usr/bin/docker daemon -H  tcp://0.0.0.0:{{.DockerPort}} -H unix:///var/run/docker.sock --storage-driver {{.EngineOptions.StorageDriver}} --tlsverify --tlscacert {{.AuthOptions.CaCertRemotePath}} --tlscert {{.AuthOptions.ServerCertRemotePath}} --tlskey {{.AuthOptions.ServerKeyRemotePath}} {{ range .EngineOptions.Labels }}--label {{.}} {{ end }}{{ range .EngineOptions.InsecureRegistry }}--insecure-registry {{.}} {{ end }}{{ range .EngineOptions.RegistryMirror }}--registry-mirror {{.}} {{ end }}{{ range .EngineOptions.ArbitraryFlags }}--{{.}} {{ end }}
ExecReload=/bin/kill -s HUP $MAINPID
MountFlags=slave
LimitNOFILE=infinity
LimitNPROC=infinity
LimitCORE=infinity
TimeoutStartSec=0
Delegate=yes
KillMode=process
Environment={{range .EngineOptions.Env}}{{ printf "%q" . }} {{end}}

@wzymaster
Copy link
Copy Markdown
Contributor Author

@nathanleclaire Can you tell me where this pull request will go ?

@nathanleclaire
Copy link
Copy Markdown
Contributor

nathanleclaire commented Sep 8, 2016

I just don't like the reference to softlayer driver specifically in the provisioner. I think it might be case that if we just always set it to GetIP output the bind will still work. Since most times it's public IP and service should be able to advertise on that IP. Do you have another driver you can try with? especially a cloudy one such as digitalocean

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants