search

dhiltgen / docker-machine-kvm

KVM driver for docker-machine
Apache License 2.0
376 stars 117 forks source link

Put NewVirConnection inside of PreCreateCheck, added error message fo… #18

Closed aaron-prindle closed 7 years ago

aaron-prindle commented 7 years ago

…r failure connection and possible reasons

aaron-prindle commented 7 years ago

This PR modified PreCreateCheck to use NewVirConnection so that errors could be propagated outside of the driver and also added an error message if libvirt failed to connect + a likely reason for this error.

kubernetes/minikube#278

dlorenc commented 7 years ago

This should also fix #13 and #14 in this repo.

aaron-prindle commented 7 years ago

@dhiltgen Friendly ping

dhiltgen commented 7 years ago

This breaks docker-machine ls

Master: (with a couple machines created)

 % docker-machine ls
NAME       ACTIVE   DRIVER   STATE     URL                        SWARM   DOCKER    ERRORS
node0      -        kvm      Running   tcp://192.168.42.52:2376           v1.12.0   
selenium   -        kvm      Stopped                                      Unknown

This PR:

 % docker-machine ls
Error attempting call to get driver name: connection is shut down
Error attempting call to get driver name: connection is shut down
NAME       ACTIVE   DRIVER   STATE   URL   SWARM   DOCKER    ERRORS
node0      -                 Error                 Unknown   unexpected EOF
selenium   -                 Error                 Unknown   unexpected EOF

This also breaks docker-machine rm <name>

% docker-machine rm -y node0
About to remove node0
WARNING: This action will delete both local reference and remote instance.
Error removing host "node0": unexpected EOF

I didn't try other commands.

aaron-prindle commented 7 years ago

Hi @dhiltgen, I have now tested with docker-machine using the create, ls, and rm commands and everything appears to be working correctly. Please take a look when you get a chance.

aaron-prindle commented 7 years ago

I have submitted a new PR, removing the additional d.getConn(). I did not quite understand your nit, I tried to keep all d.conn calls in getConn. PTAL, thanks.

aaron-prindle commented 7 years ago

I have made the requested changes, PTAL. Thanks.

dhiltgen commented 7 years ago

I have made the requested changes, PTAL. Thanks. Hide all reviewers

Changes look good - thanks!

Looks like you accidentally added the binary to your commit - please remove that so it's just the source and I think this is ready to merge.

aaron-prindle commented 7 years ago

I have removed the binary now. PTAL, thanks!