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 Machine.list() function to list machines #2

Closed
wants to merge 3 commits into from
Closed

Add Machine.list() function to list machines #2

wants to merge 3 commits into from

Conversation

dikarel
Copy link
Contributor

@dikarel dikarel commented Sep 21, 2016

Useful for writing utilities that need to invoke docker-machine ls.

Summary

  • No additional dependencies
  • OutputParser utility class to read CLI output into JavaScript objects
  • Sample result and explanation in the readme for the new function
  • Readme using UNIX line endings

I explored docker-machine's docs, and found out that the --format parameter of the ls command doesn't support JSON output (e.g. can't do {{json .Name}}). Ended up reformatting the output into tab-delimited lines instead, before parsing them into JavaScript objects.

To keep the main class small enough to understand, I modularized parser logic into a separate class.

I noticed that the line endings used in the project are mixed. I assume for now that UNIX-style is the correct one, as most files in the project use UNIX-style line endings.

Tested on OS X El Capitan

$ node examples/list-machines.js
[ { name: 'agent-1',
    active: false,
    activeHost: false,
    activeSwarm: false,
    driverName: 'virtualbox',
    state: 'running',
    url: 'tcp://192.168.99.101:2376',
    swarm: null,
    dockerVersion: '1.12.0',
    responseTime: null,
    error: null },
  { name: 'agent-2',
    active: false,
    activeHost: false,
    activeSwarm: false,
    driverName: 'virtualbox',
    state: 'stopped',
    url: null,
    swarm: null,
    dockerVersion: null,
    responseTime: 475,
    error: null },
  { name: 'default',
    active: true,
    activeHost: true,
    activeSwarm: false,
    driverName: 'virtualbox',
    state: 'running',
    url: 'tcp://192.168.99.100:2376',
    swarm: null,
    dockerVersion: '1.12.0',
    responseTime: null,
    error: null },
  { name: 'testong',
    active: false,
    activeHost: false,
    activeSwarm: false,
    driverName: 'virtualbox',
    state: 'stopped',
    url: null,
    swarm: null,
    dockerVersion: null,
    responseTime: null,
    error: null } ]

$ node examples/run-command.js ls /
Users
bin
dev
etc
home
init
lib
lib64
linuxrc
mnt
opt
proc
root
run
sbin
sys
tmp
usr
var

@vweevers
Copy link
Owner

To be clear, when I suggested to drop table-parser, it's not because it's an additional dependency (I'm deaf to people boosting about "No dependencies!", I'll take a well-tested dependency over a custom solution any day), but because it's a "best-guess" parser. We don't have to guess, is the point of --format.

Some thoughts about the individual fields:

  • Don't parse Active. We already have ActiveHost (which is probably what most people want) and ActiveSwarm. The value of Active is merely a human-readable combination of those two, so just take it as-is (or even leave it out).
  • I see no reason to strip the "v" from DockerVersion. If a consumer needs to do further processing, they can do semver.parse(version). If it's just for human consumption, then the "v" doesn't matter either.
  • ResponseTime can be parsed with ms. Or better yet, print it as an integer: {{ .ResponseTime | printf "%d" }}, which yields the time in nanoseconds.

With these simplifications, the amount of code required drops significantly, and there's no point to having a Parser class. We can just do:

machine.responseTime = parseInt(machine.responseTime) / 1e6
machine.state = machine.state.toLowerCase()
machine.activeHost = machine.activeHost === 'true'
machine.activeSwarm = machine.activeSwarm === 'true'

Which also makes our code easier to grasp at first sight.

As for EngineOptions and SwarmOptions. Like you mentioned, the template's scope sadly lacks a json function, so the best we could do is {{ .EngineOptions | printf "%+v" }} which yields a Go-syntax representation. Luckily, we have some alternative avenues:

  1. Read EngineOptions and SwarmOptions from ~/.docker/machine/machines/<name>/config.json, which is what docker-machine ls does anyway
  2. Run docker-machine inspect <name> for each machine, which spits out JSON with a whole lot more metadata

I went over some of the Go source code of docker-machine, but not all of it, so I can't say if docker-machine normalizes the config.json in any way. So to be on the safe side, even though it'll be less performant, I propose for the short term:

  1. Exclude EngineOptions and SwarmOptions by default
  2. Implement a Machine.inspect(name) method
  3. Add a inspect option to list. E.g. Machine.list({ inspect: true }) would merge the results of ls with Machine.inspect(name).

First off, I'm gonna refactor a bit, so that functions like command and inspect (or maybe all of them) can be reused and called both statically (Machine.inspect(name)) and on an instance (machine.inspect()). Then I'll take a stab at implementing Machine.list(). Also, it's about time I add some unit tests.

If anyone is gonna be using node-docker-machine a lot, and wants better performance, then we can talk about fully imitating docker-machine ls, which shouldn't be that hard. It's 1) reading configuration and 2) pinging the Docker Remote API of each machine.

@dikarel Sorry for the long story. If you have any quibbles with the above, LMK. I'm closing this PR, but I want to thank you for your work of course.

@vweevers vweevers closed this Sep 25, 2016
@vweevers
Copy link
Owner

Also, thanks for mentioning this:

I noticed that the line endings used in the project are mixed. I assume for now that UNIX-style is the correct one, as most files in the project use UNIX-style line endings.

You're right, it should be LF.

vweevers added a commit that referenced this pull request Sep 25, 2016
@dikarel
Copy link
Contributor Author

dikarel commented Sep 25, 2016

Thanks, and apologies for misunderstanding the reqs.
All of these sound great and would be useful for my use-case at work.

I would also like to suggest the following for Machine.list depending on how it will be implemented:

  1. via reading config files directly: Handling non-standard docker-machine directory (IIRC via MACHINE_STORAGE_PATH env variable)
  2. via calling docker-machine ls: Adding a timeout option, as docker-machine ls has a tendency to timeout once you have 10+ machines

@vweevers
Copy link
Owner

via calling docker-machine ls: Adding a timeout option, as docker-machine ls has a tendency to timeout once you have 10+ machines

Can you clarify? Is this a known bug of docker-machine?

@vweevers vweevers mentioned this pull request Sep 25, 2016
Closed
9 tasks
@dikarel
Copy link
Contributor Author

dikarel commented Sep 25, 2016

I'm running 10-20 docker machines on VirtualBox in Windows 7; I noticed that running docker-machine lswill return Timeout for all machines' states even when they're actually running. I think that there are 2 possibilities here:

  1. docker-machine's timeout is applied globally and not per-machine
  2. docker-machine is querying all my machines at the same time, causing each of them to respond sluggishly

I had to increase the timeout for it to work, like this: docker-machine ls -t 30

A configurable timeout option would also be useful in case of busy machines or remote machines over slow connection, as mentioned by this issue

Relevant: docker/machine@02c235b

@vweevers
Copy link
Owner

Thanks for the explanation. I added a timeout option in e72abb6.

@vweevers
Copy link
Owner

@dikarel published 2.0.0. Let me know how the new features work out for you.

@dikarel
Copy link
Contributor Author

dikarel commented Sep 27, 2016

It works! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants