-
Notifications
You must be signed in to change notification settings - Fork 2k
Test SSH establishing a real SSH connection #665
Conversation
Fix docker#656 Signed-off-by: Guillaume Giamarchi <guillaume.giamarchi@gmail.com>
@@ -736,7 +736,37 @@ func (d *Driver) waitForSSHServer() error { | |||
"MachineId": d.MachineId, | |||
"IP": ip, | |||
}).Debug("Waiting for the SSH server to be started...") | |||
return ssh.WaitForTCP(fmt.Sprintf("%s:%d", ip, d.SSHPort)) | |||
|
|||
err = ssh.WaitForTCP(fmt.Sprintf("%s:%d", ip, d.SSHPort)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we update ssh.WaitForTCP
to accepts a retries itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or how about a new ssh.WaitForSSH function that would loop until "exit 0" could be successfully executed like @ggiamarchi implemented for the OpenStack driver? Other users of ssh.WaitForTCP could be changed to this as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctrlrsf +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggiamarchi how about you just create the new retry wait. then, as @ctrlrsf said, we can update the others as needed. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @ctrlrsf suggestion. @ggiamarchi can you please update to create the generic function in ssh
package and use it in the openstack
driver in this PR please? We will move the other drivers over to this new method when it is merged.
In the current master, there is now a generic (universal to all drivers) way that this is done. @ehazlett perhaps we can close this issue? Big thanks to @ggiamarchi for the original idea, by the way. |
Yep, this should be covered now. @ggiamarchi thanks for the idea! |
It's done in the OpenStack driver here but i think it should be generalize because this issue can potentially affect any machine.
Fix #656