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

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

Merged
merged 1 commit into from
Nov 2, 2016
Merged

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

merged 1 commit into from
Nov 2, 2016

Conversation

aaron-prindle
Copy link
Contributor

…r failure connection and possible reasons

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Aug 1, 2016 •
edited
Loading

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
Copy link

dlorenc commented Aug 3, 2016

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

@aaron-prindle
Copy link
Contributor Author

@dhiltgen Friendly ping

@dhiltgen
Copy link
Owner

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 aaron-prindle force-pushed the error-msg-for-failure-conn branch 2 times, most recently from 947dba4 to 095fa7a Compare August 24, 2016 18:07
@aaron-prindle
Copy link
Contributor Author

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.

dhiltgen
dhiltgen reviewed Sep 22, 2016
View reviewed changes
Copy link
Owner

@dhiltgen dhiltgen left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long to get back to this. Overall looks pretty good.

kvm.go
network, err = d.conn.NetworkDefineXML(xml)

//TODO(aaron-prindle) see if this is needed?
conn, err = d.getConn()
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is needed, since you're doing it on line https://github.com/dhiltgen/docker-machine-kvm/pull/18/files#diff-bda8ba8a54ef41ed9e8a4e3427ef653dR225

kvm.go
if err != nil {
return err
}
network, err := conn.LookupNetworkByName(d.PrivateNetwork)
Copy link
Owner

Choose a reason for hiding this comment

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

nit - style wise I'd keep this as d.conn.

@aaron-prindle
Copy link
Contributor Author

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.

dhiltgen
dhiltgen requested changes Oct 31, 2016
View reviewed changes
kvm.go
@@ -7,7 +7,6 @@ import (
"encoding/xml"
"errors"
"fmt"
"github.com/alexzorin/libvirt-go"
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks the build.

% make
GOGC=off go build -i -o docker-machine-driver-kvm
# github.com/dhiltgen/docker-machine-kvm
../kvm.go:91: undefined: libvirt in libvirt.VirConnection
../kvm.go:92: undefined: libvirt in libvirt.VirDomain
Makefile:4: recipe for target 'build' failed
make: *** [build] Error 2

If I add it back, it builds.

Copy link
Contributor Author

@aaron-prindle aaron-prindle Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, go-imports removed this. I have added it back.

kvm.go

if err != nil {
return err
}
Copy link
Owner

Choose a reason for hiding this comment

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

Something seems wrong here. If the lookup fails on line 228, you set up the XML definition on 272, but then just error out instead of actually creating the network. We should try to create the network if it doesn't exist.

Copy link
Contributor Author

@aaron-prindle aaron-prindle Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This additional error check somehow got in. The network creation (NetworkDefineXML(xml)) is below (line 281) with an appropriate error catch. I will remove this error check as it does nothing.

@aaron-prindle
Copy link
Contributor Author

I have made the requested changes, PTAL. Thanks.

@dhiltgen
Copy link
Owner

dhiltgen commented Nov 2, 2016

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
Put NewVirConnection inside of PreCreateCheck, added error message for
661cec4 
failure connection and possible reasons.  Fixed previous error messages with
docker-machine.  Cleaned up code.
@aaron-prindle
Copy link
Contributor Author

I have removed the binary now. PTAL, thanks!

@dhiltgen dhiltgen merged commit 37bb4cc into dhiltgen:master Nov 2, 2016
@r2d4 r2d4 mentioned this pull request Dec 16, 2016
Closed
@ceridwen ceridwen mentioned this pull request Jun 8, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhiltgen dhiltgen dhiltgen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaron-prindle @dlorenc @dhiltgen