Adds support for docker-machine #77

labianchin · 2015-07-26T23:58:40Z

Related to issue #50.
Although it is already working I'd still consider this as a preview.
It seems to be missing to update Readme, add tests and improve the install (e.g. add_environment_variables is not important for docker-machine).
Can you please provide some feedback? And also what would you expect to get this merged?

brikis98 · 2015-07-28T03:26:44Z

Thanks for the pull request. I apologize for the delay, but I'm currently on the road with limited Internet access, so I'll take a look once I'm back (should be early August).

ilkka · 2015-07-28T05:56:53Z

Since probably in almost all cases docker-osx-dev would be run against the active machine, I think you could drop the requirement to set the machine name environment variable on every invocation, and instead just use $(docker-machine active) to get it. Thoughts?

labianchin · 2015-07-28T13:55:46Z

@brikis98, no problem, I still have to add some tests.

@ilkka, sorry getting ahead of you, but I needed it this week.
Correct, but there is the case where you can have multiple active docker-machines. What about passing the machine name as first argument?

docker-osx-dev machine-name

Also, I am trying to maintain compatibility with boot2docker. Later if needed we can drop support for it.

brikis98 · 2015-07-30T17:01:34Z

src/docker-osx-dev

+################################################################################
+
+function get_json_val () { 
+    python -c "import json,sys;sys.stdout.write(json.dumps(json.load(sys.stdin)$1))" | sed -e 's/^"//'  -e 's/"$//'


I'm a bit concerned about adding a dependency on python. Any alternative way to do this?

Agreed! That is a hard one and I will have to think better on this. I saw it is possible to do with sed, grep and awk.
Some examples that I saw using grep actually use GNU grep, which is not always available on OSX. I am open to any suggestion.

brikis98 · 2015-07-30T17:10:39Z

Thank you for the PR. This is a pretty massive change that forks the code in quite a few places. Do we know for sure that using boot2docker directly via the CLI is deprecated? Is docker-machine definitely the way to interact with Docker on the desktop in the future? If so, it's worth thinking about supporting only docker-machine. This project is hard to test (see #7), so I worry about any complications that fork the code too much.

bgentry · 2015-07-30T17:24:26Z

The boot2docker-cli deprecation notice is here: https://github.com/boot2docker/boot2docker-cli#pending-deprecation

I have used docker-machine locally on OS X and it seems to work just fine, not sure if there's any reason to keep supporting the boot2docker-cli instead. Maybe somebody from Docker or somebody that has more knowledge around that can chime in.

brikis98 · 2015-07-30T17:42:04Z

Hmm, the Docker Docs still point to boot2docker and using it as a command line tool.

labianchin · 2015-07-31T13:10:37Z

Thanks @brikis98 for the feedback!

I made some changes but, for me, there are at least two things outstanding:

Find an alternative to using python to parse docker-machine inspect json.
Add some automated tests

Regarding the boot2docker deprecation, my understanding is that it is slowly being deprecated. I'd suggest for now keeping support for both boot2docker and docker-machine. But in the near future, boot2docker support should be dropped.

brikis98 · 2015-07-31T16:26:09Z

OK, let's keep the boot2docker support a bit longer. Let me know if you have ideas on the JSON & automated test front.

ilkka · 2015-08-05T08:07:39Z

Correct, but there is the case where you can have multiple active docker-machines.

@labianchin, multiple active? I thought the active machine was determined by DOCKER_HOST in the env, meaning there can only be one?

ilkka · 2015-08-05T08:27:02Z

just tried this branch again and noticed a weird thing: my freshly generated virtualbox docker machine has a config like this:

{
    "DriverName": "virtualbox",
    "Driver": {
        "IPAddress": "192.168.99.100",
        ...
        "SSHPort": 55045,
        ...
    }
   ...
}

which makes docker-osx-dev sync try to connect to port 55045, which fails. docker-machine ssh <machine> works just fine though, as does manually ssh'ing to port 22. I have no idea why that value is there or what it's significance is, but set up like this, the sync doesn't work.

ilkka · 2015-08-05T08:33:31Z

Ah, now I understand what is happening with SSHPort: that's the port on localhost that the vbox SSH port is forwarded to. So if you're gonna use SSHPort for rsync, you should connect to localhost, not the docker machine itself.

labianchin · 2015-08-05T12:20:47Z

@labianchin, multiple active? I thought the active machine was determined by DOCKER_HOST in the env, meaning there can only be one?

@ilkka, Yes. That makes more sense when you use a driver like EC2, OpenStack or GCE. This way you can have multiple docker-machine active (and running) in the cloud. You might want to have a docker-machine per each project you have.

So in one terminal tab you can run:

cd project-foo && docker-osx-dev --machine-name machine-for-project-foo

And in the other:

cd project-bar && docker-osx-dev --machine-name machine-for-project-bar

labianchin · 2015-08-05T12:24:55Z

which makes docker-osx-dev sync try to connect to port 55045, which fails. docker-machine ssh works just fine though, as does manually ssh'ing to port 22. I have no idea why that value is there or what it's significance is, but set up like this, the sync doesn't work.

@ilkka Thanks for catching that! I was testing more with the openstack driver. There the SSHPort was 22. I will take a look on that. I guess I will have to understand better which params docker-machine ssh use.

labianchin · 2015-08-05T12:59:38Z

@ilkka

So I was looking into what is the arguments docker-machine scp use, with the virtualbox driver it is this:

/usr/bin/scp [/usr/bin/scp -o IdentitiesOnly=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o LogLevel=quiet -3 -i /Users/me/.docker/machine/machines/local/id_rsa foo.txt docker@192.168.99.100:/]

So, I just removed the -p argument for the rsync --rsh. Now should be fine.

But, one problem I am getting is that even after install_rsync_on_docker_host, and with rsync on /usr/local/bin it keeps getting -sh: rsync: not found.

It seems rsync 64bits is now available for tce-load: boot2docker/boot2docker#936 (comment). @brikis98, should we use it?

ilkka · 2015-08-05T13:05:43Z

You might want to have a docker-machine per each project you have.

@labianchin ah yes, but since active is stored in the env, it is per terminal session, so you can run multiple machines that way too, just in separate terminals. Essentially it's the same. I don't mind having the parameter, I just think that defaulting to whatever docker-machine active outputs saves typing and is in no way surprising behaviour :)

But, one problem I am getting is that even after install_rsync_on_docker_host, and with rsync on /usr/local/bin it keeps getting -sh: rsync: not found.

I found that I had to docker-osx-dev install before it found rsync after restarting (but not recreating) my docker machine, although I might have been hallucinating from all the coffee 😅 Don't know if it's related at all.

brikis98 · 2015-08-05T15:31:32Z

@labianchin: Yes, if the 64 bit rsync is now working, we should go back to using tce-load to install it. I think the original code is still in the comments. I've been meaning to do this, but haven't had a chance to get to it, so PRs are welcome :)

labianchin · 2015-08-06T01:20:58Z

@ilkka, that make sense. I agree that would be a good default. But if, for now, we are keeping the support for boot2docker, then it would be better to keep the default (no --machine-name arg) as using boot2docker.

Also, I just run docker-machine active and I get this:

dev

But when I run docker-machine ls:

NAME         ACTIVE   DRIVER       STATE     URL                         SWARM
dev                   virtualbox   Stopped
dev1                  virtualbox   Stopped
dokku-test            openstack    Running   tcp://10.254.132.163:2376
lbianchin2            openstack    Running   tcp://10.254.132.14:2376
local                 virtualbox   Running   tcp://192.168.99.102:2376

That is weird.

Fix port parameter in do_rsync Install will add to host based on docker-machine name and ip Install does not set the environment when using docker-machine

- pass --machine-name as argument, instead of env var - ssh command working also for boot2docker ssh - add_docker_environment_variables function that adds env vars only for boot2docker - add doc and rename get_json_value

arguments

brikis98 · 2015-08-06T04:11:09Z

src/docker-osx-dev

+}
+
+#
+# Configures variables based on a the docker-machine inspect from $DOCKER_MACHINE_NAME


Minor typo: "a the"

brikis98 · 2015-08-06T04:16:20Z

@labianchin: Thanks! I just left a bunch more comments. Almost there.

ilkka · 2015-08-06T07:26:07Z

@labianchin that docker-machine active behaviour is due to the slightly different ways active and ls determine the active host. active only looks at the value of DOCKER_HOST, but ls also checks that the host is running.

labianchin · 2015-08-06T13:20:30Z

src/docker-osx-dev

+  docker-machine start "$DOCKER_MACHINE_NAME"
+  eval $(docker-machine env "$DOCKER_MACHINE_NAME")
+  configure_docker_machine
+}


So, here, this eval will run something like this:

export DOCKER_TLS_VERIFY="1" export DOCKER_HOST="tcp://192.168.99.103:2376" export DOCKER_CERT_PATH="/Users/someone/.docker/machine/machines/local" export DOCKER_MACHINE_NAME="local"

We can see that DOCKER_MACHINE_NAME will be redefined.

Also, as in the docker-machine documentation, if the user runs eval "$(docker-machine env <machine-name>)", it will define the DOCKER_MACHINE_NAME to the env. Then if he runs docker-osx-dev, he would not need to pass the --machine-name argument.

I guess this is fine, I can add to the Readme.md about this behavior.

labianchin · 2015-08-06T13:25:47Z

@brikis98 thanks for the feedback, I will be making the fixes.

@ilkka Please see this: #77 (comment)

So running eval "$(docker-machine env <machine-name>)" would mean the user does not need use the --machine-name argument.

nathanleclaire · 2015-08-06T18:01:43Z

One idea I've tinkered with is some kind of --delta flag to docker-machine scp which would shell out to rsync if available instead of scp. Would that be of interest to you / help your workload on this project?

labianchin · 2015-08-06T18:06:16Z

Yes @nathanleclaire, I would be interested! Can you provide some details on what you have in mind? Probably via e-mail. Thanks!

labianchin · 2015-08-07T02:16:24Z

@brikis98 I think I got all the suggested fixes in this last commit. Can you take a look?

brikis98 · 2015-08-07T04:17:33Z

src/docker-osx-dev

+
+#
+# Read a json string from stdin and return the value of the key provided as first argument
+# Uses head -n 1 to avoid duplicates


This is an implementation detail. Instead, you should probably say something along the lines of "Does a simple string search and returns the first string value that matches the given key".

brikis98 · 2015-08-07T04:22:26Z

@labianchin: A few more questions/comments above :)

labianchin · 2015-08-07T23:51:08Z

Hey @brikis98, I've made some fixes. Please also see my previous comments.

brikis98 · 2015-08-08T04:26:49Z

README.md

+```
+> docker-machine create --driver virtualbox <machine-name>
+> eval "$(docker-machine env <machine-name>)"
+> docker-osx-dev install


Sorry, just realized I have one more question on this: do you have to run the install command every time you run docker-machine create? Or only the eval part?

So, I am little confused on what should be the role of the install for docker-machine.

install_dependencies # Should actually run before docker-machine create. Once for osx user. init_docker_host # Per machine, should run after docker-machine create. This seems only important for the next command install_rsync_on_docker_host # Is this really important? the sync will also install rsync. add_docker_host # Per machine, should run after docker-machine create. Also, is it important? add_environment_variables # Not used for docker-machine

I would avoid making the install create the docker-machine, as in the creation the user needs to supply the driver he wants and all the driver's configuration. It is also seems misleading that a install command would create a machine.

I'd suggest the following flow:

docker-osx-dev install_dependencies # once per osx user docker-machine create --driver virtualbox <machine-name> eval "$(docker-machine env <machine-name>)" cd /foo/bar docker-osx-dev

In this way there is no need to run install after docker-machine create. The install would only make sense for boot2docker.

brikis98 · 2015-08-09T04:12:33Z

I expect developers to run the docker-osx-dev script very frequently, so we should keep its startup time well under one second in the normal case. Therefore, the install command should take care of anything that:

Takes a long time (more than ~1 second)
Anything that needs to happen only once per computer
Anything that needs to be run before you can use docker (i.e., install docker itself)

This is why it includes install_dependencies, init_docker_host, install_rsync_on_docker_host, add_docker_host, and add_environment_variables. The actual docker-osx-dev script, in theory, should run just the watch_and_sync command. However, it runs a few other steps (e.g., install_rsync_no_docker_host) that:

Might go out of sync as the user tweaks their docker environment
Are very fast to run in the normal case (e.g., checking if rsync is installed is very quick)

The question I asked earlier, though I guess not very clearly, was whether the install command should automate the eval "$(docker-machine env <machine-name>)" command you have listed in the README, or any other manual steps necessary for using docker-machine?

labianchin · 2015-08-10T00:38:08Z

Yes. That really make sense. Thanks for clarifying.

Answering your question: I believe it shouldn't, as users might want to switch between different docker-machine instances (machines). So every time they run docker-osx-dev (watch_and_sync), they supply which machine name they want to run with.

So, clarifying, this steps, should run just after creating a new machine instance:

eval "$(docker-machine env <machine-name>)"
docker-osx-dev install

Which is equivalent of:

docker-osx-dev --machine-name <machine-name> install

After the instance is created and docker-osx-dev installed, users only need to run:

eval "$(docker-machine env <machine-name>)"
docker-osx-dev

Or the equivalent:

docker-osx-dev --machine-name <machine-name>

Adds support for docker-machine

brikis98 · 2015-08-10T01:42:45Z

Awesome, thanks for the PR. It's now merged.

labianchin · 2015-08-10T01:44:29Z

Thanks for all the feedback! That has been a great experience contributing to this project.

brikis98 reviewed Jul 30, 2015
View reviewed changes

labianchin force-pushed the master branch from d903b8d to 14c590a Compare August 5, 2015 12:55

labianchin force-pushed the master branch from 6588046 to 047d686 Compare August 6, 2015 01:13

labianchin added 5 commits August 5, 2015 22:26

Adds support for docker-machine

dc0c179

Add docker-machine support to readme.

f8f144c

Fix port parameter in do_rsync Install will add to host based on docker-machine name and ip Install does not set the environment when using docker-machine

Fixes from the feedback of brikis98

d0452bd

- pass --machine-name as argument, instead of env var - ssh command working also for boot2docker ssh - add_docker_environment_variables function that adds env vars only for boot2docker - add doc and rename get_json_value

Adds tests to check proper DOCKER_HOST variables configuration

d5ccbed

Remove port configuration on rsync, try to mimic docker-machine scp

bdaac15

arguments

brikis98 reviewed Aug 6, 2015
View reviewed changes

ilkka mentioned this pull request Aug 6, 2015

"active" and "ls" use different checks for active machine docker/machine#1651

Closed

labianchin reviewed Aug 6, 2015
View reviewed changes

Fixes in docs and typos, add more tests to get_json_value

e63d07b

brikis98 reviewed Aug 7, 2015
View reviewed changes

Adds more tests for get_json_value and clarify docs

882600f

brikis98 reviewed Aug 8, 2015
View reviewed changes

Adds back install_rsync_on_docker_host to watch_and_sync

0b072f9

brikis98 added a commit that referenced this pull request Aug 10, 2015

Merge pull request #77 from labianchin/master

0a9c919

Adds support for docker-machine

brikis98 merged commit 0a9c919 into brikis98:master Aug 10, 2015

brikis98 mentioned this pull request Nov 16, 2015

Added explicit machines id_rsa. #140 #145

Merged

Adds support for docker-machine #77

Adds support for docker-machine #77

Conversation

labianchin commented Jul 26, 2015

brikis98 commented Jul 28, 2015

ilkka commented Jul 28, 2015

labianchin commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Jul 30, 2015

bgentry commented Jul 30, 2015

brikis98 commented Jul 30, 2015

labianchin commented Jul 31, 2015

brikis98 commented Jul 31, 2015

ilkka commented Aug 5, 2015

ilkka commented Aug 5, 2015

ilkka commented Aug 5, 2015

labianchin commented Aug 5, 2015

labianchin commented Aug 5, 2015

labianchin commented Aug 5, 2015

ilkka commented Aug 5, 2015

brikis98 commented Aug 5, 2015

labianchin commented Aug 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Aug 6, 2015

ilkka commented Aug 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labianchin commented Aug 6, 2015

nathanleclaire commented Aug 6, 2015

labianchin commented Aug 6, 2015

labianchin commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Aug 7, 2015

labianchin commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Aug 9, 2015

labianchin commented Aug 10, 2015

brikis98 commented Aug 10, 2015

labianchin commented Aug 10, 2015