New Driver: Vultr #958
New Driver: Vultr #958
Conversation
|
@janeczku - I'd like to try this out, can you rebase it please? :) |
|
@hairyhenderson Can do - probably tomorrow. |
|
Rebased the PR. Please test to break :) @hairyhenderson |
|
Thanks @janeczku! |
| MachineID string | ||
| MachineName string | ||
| PublicIP string | ||
| PrivateIP string |
There was a problem hiding this comment.
Can you add an IPAddress property too? It should be the same as PublicIP, or PrivateIP if PrivateNetworking is true. (see #1043)
There was a problem hiding this comment.
I misunderstood PrivateNetworking :)... So, can you either add IPAddress which is in sync with PublicIP, or can you rename PublicIP to IPAddress?
There was a problem hiding this comment.
I think its best to just rename PublicIP.
|
Ok, so here's an issue, but I don't know if it's in this code or if has to do with some of the recent SSH changes: It appears that the resulting VM is working fine, so it might be nothing. /cc @nathanleclaire @ehazlett @sthulb - I vaguely recall something recently that might be related? |
| } | ||
|
|
||
| if d.PrivateIP == "" || d.PrivateIP == "0" { | ||
| d.PrivateIP = "N/A" |
There was a problem hiding this comment.
Any reason this is N/A instead of empty string? The convention in other drivers is that it gets set to ""
There was a problem hiding this comment.
yes indeed, it should be an empty string if the IP is not set.
There was a problem hiding this comment.
@hairyhenderson i've seen this when you use an invalid username.
EDIT: sorry this was meant for the Error dialing TCP: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain error.
|
@janeczku - looks like the start command is asynchronous. Can you change |
| cli.BoolFlag{ | ||
| EnvVar: "VULTR_IPV6", | ||
| Name: "vultr-ipv6", | ||
| Usage: "enable ipv6 for droplet", |
There was a problem hiding this comment.
Maybe:
Usage: "Enable IPv6 for VPS",
|
@janeczku - overall, this is looking pretty good. Might be nice too to add a bit more info-level logging, especially when long operations (during |
|
@hairyhenderson thanks for your valuable observations!
This error occurs when the SSHKey has not yet been provisioned to the new instance (but the SSHd is already running). So this is expected behavior. The provisioning does succeed after all.
Can do. See also PR #1216 which would make this unnecessary. |
|
Merged reviewers suggestions, rebased again and squashed. |
|
@janeczku looks like you'll need to rebase again |
|
Hey @hairyhenderson, would you take a final look at the patch and (hopefully) give your OK? I rebased again and added a tiny bit of code making /cc @ibuildthecloud @imikushin |
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
@hairyhenderson FYI, these two functions have been added after you last took a look (to allow booting RancherOS).
Yep! I'll have a look. Might need to wait until Monday or Tuesday, but I'll get to this :)
Thanks for the build! And, too bad the free credit is only for new users ;) |
|
@hairyhenderson Thank you! |
| // RancherOS - Create iPXE boot script | ||
| func (d *Driver) createBootScript() error { | ||
| content := `#!ipxe | ||
| set base-url https://releases.rancher.com/os/latest |
There was a problem hiding this comment.
Would https: work w/ iPXE?
There was a problem hiding this comment.
@imikushin Not in every environment. HTTPS support has to be specifically enabled when compiling the iPXE firmware. So for Vultr HTTPS works, for other iPXE environments one might need to use plain HTTP.
|
ping @hairyhenderson 😏 |
|
@janeczku - hey, sorry, I've been super-busy with a new job and a new baby (born yesterday!) so it might be a little while before I can take a good look at this... In the meantime, looks like there's some merge conflicts again? I'll try to take a look in the next few days :) |
@hairyhenderson, thanks for the feedback and congrats! |
| EnvVar: "VULTR_OS", | ||
| Name: "vultr-os-id", | ||
| Usage: "Vultr operating system ID. Default: 159 (RancherOS)", | ||
| Value: 159, |
There was a problem hiding this comment.
So... While RancherOS is definitely cool, and is considered "supported" from docker-machine's point of view, the default for remote drivers (like Vultr) is Ubuntu (see https://github.com/docker/machine/blob/master/docs/index.md#specify-a-base-operating-systems). So IMO you should change the default to Ubuntu (probably 14.04 - i.e. 160), just to keep in line with what everyone expects and assumes.
There was a problem hiding this comment.
Not that I disagree, but...
RancherOS can be configured to look like Ubuntu :)
It's just a matter of what console container to use. Currently, RancherOS
does use ubuntu-console by default in cloud deployments.
Just my 2 cents.
On Tue, Jun 23, 2015, 23:41 Dave Henderson [email protected] wrote:
In drivers/vultr/vultr.go
#958 (comment):
- })
+}
+// GetCreateFlags registers the flags this driver adds to "docker hosts create"
+func GetCreateFlags() []cli.Flag {
- return []cli.Flag{
cli.StringFlag{EnvVar: "VULTR_API_KEY",Name: "vultr-api-key",Usage: "Vultr API key",},cli.IntFlag{EnvVar: "VULTR_OS",Name: "vultr-os-id",Usage: "Vultr operating system ID. Default: 159 (RancherOS)",Value: 159,So... While RancherOS is definitely cool, and is considered "supported"
from docker-machine's point of view, the default for remote drivers (like
Vultr) is Ubuntu (see
https://github.com/docker/machine/blob/master/docs/index.md#specify-a-base-operating-systems).
So IMO you should change the default to Ubuntu (probably 14.04 - i.e. 160),
just to keep in line with what everyone expects and assumes.—
Reply to this email directly or view it on GitHub
https://github.com/docker/machine/pull/958/files#r33076225.
There was a problem hiding this comment.
Yup, sure. But even though it might look like Ubuntu, it's not Ubuntu ;) I really think all of the remote drivers need to have the same default image.
There was a problem hiding this comment.
@hairyhenderson I have changed the default OS back to Ubuntu. Since the iPXE deployment of RancherOS is the better choice for Vultr (deploying Ubuntu takes up to 5 minutes) i have added instructions to the docs concerning the possibility to use RancherOS as base OS.
/cc @imikushin
|
@janeczku - so far this looks pretty good! Just the one thing about the default for |
|
@ehazlett @nathanleclaire you wanna chime in? i'd be happy to finally see this merged :) |
|
Thanks for your work @janeczku! I just tried out this fork with my Vultr account. It does not matter if I choose RancherOS or Ubuntu, Looking at my Vultr console, the servers have been created successfully. Also, I am able to login using the SSH keys created by docker-machine (in Any idea what's going wrong here? How to debug this? |
|
@djmaze thanks for your feedback. This is caused by recent changes to Vultr's API protocol. It is already fixed in the Vultr API client and i will push it to the machine driver later today. |
|
@djmaze this is now fixed. You can try the dev builds available here: https://github.com/janeczku/machine/releases/tag/v0.4.0-dev |
|
@janeczku Great, it works now. Thanks for the update! |
|
After creating multiple machines on Vulr, It seems rate limiting does not work as needed. |
|
@djmaze Thanks for your feedback. The rate limit error is due to the |
|
Excuse my limited Go knowledge. The tokenbucket library uses a mutex. Shouldn't this work across goroutines already? Ah I see, there is one driver instance per machine, so one bucket per host. That's unfortunate ;/ |
|
@djmaze You got that right. Vultr really needs to change their 1/sec rate limit policy to something more reasonable like x/minute or x/hour. |
|
So forgive me if this is stupid, but I just made the token bucket global: diff --git a/drivers/vultr/vultr.go b/drivers/vultr/vultr.go
index 11249b7..98a3b04 100644
--- a/drivers/vultr/vultr.go
+++ b/drivers/vultr/vultr.go
@@ -96,10 +96,12 @@ func GetCreateFlags() []cli.Flag {
}
}
+var vultrTokenBucket = tokenbucket.NewBucket(1*time.Second, 1)
+
func NewDriver(machineName string, storePath string, caCert string, privateKey string) (drivers.Driver, error) {
d := &Driver{MachineName: machineName, storePath: storePath, CaCertPath: caCert, PrivateKeyPath: privateKey, bucket: nil}
// throttle API queries according to 1/s rate-limit
- d.bucket = tokenbucket.NewBucket(1*time.Second, 1)
+ d.bucket = vultrTokenBucket
return d, nil
}This makes the Of course we can do better and initialize the bucket on first use instead (using a mutex). Apart from that, is there anything wrong with this change? |
|
Thats interesting! I would not have expected a single tokenbucket to work across multiple goroutines. It was my believe that when you have multiple receivers ( |
|
Confirmed working 1req/s API query throttling.
|
Co-Authored-By: Fabio Berchtold <[email protected]> Signed-off-by: Jan Broer <[email protected]>
|
Unfortunately, now I am not able to create machines based on RancherOS with the Vultr driver. This is the output when using With the default Ubuntu OS the installation works. It seems the API has changed again and Vultr is now returning |
|
Got it working again by making SCRIPTID an |
|
@djmaze Pushed fix to the Vultr API client repo. It was not enough to change the SCRIPTID field to integer since the API continues to return this field as a string in other calls. See here: https://github.com/JamesClonk/vultr/pull/7/files |
|
Hi , thanks for your efforts and persistence in submitting this driver. We are extremely excited that there is so much interest in Docker Machine and we really appreciate your interest. However, at this time it is proving to be extremely difficult for us to keep up with reviewing and testing each of these drivers for inclusion in the Machine core. We really want to switch to a more pluggable model, as well as polish up a few things about the driver model which need to be changed to ensure a smooth and sustainable future. Therefore, we will be moving to a plugin model for 0.5 and would love to have you involved in the design and development process. We are closing the outstanding driver PRs at this time, but please keep the code. We will stick closely to the current driver interface and you should be able to re-use a lot (if not all) of the existing driver along with the new plugin model. We will be moving all of the drivers which are merged directly into Machine today to the plugin model when it is available, so there will be no special treatment of those, and there will be documentation outlining the process of developing and using a Docker Machine driver plugin. With all of that being said, we want to apologize for the lack of feedback on your pull request. As contributors ourselves, we understand that being left in limbo is no fun. We would have liked to address this sooner, and in the future we will be more responsive around these kinds of issues. Once again, we thank you for the contribution and the tremendous support. Keep hacking strong! If you want to contribute to the design of the plugin model, we'd love to get your input on this issue where we will be planning it: |
Vultr driver updated to the new driver specs. Replaces PR #538 by @JamesClonk.
Signed-off-by: Jan Broer [email protected]