-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
I like the idea of a generic provisioner - that'll make life easier as others come along. But just be aware of the changes happening in #1090. |
It seems #1090 could benefit from this refactor too. Some of that code that that PR had to copy and paste is now in the GenericProvisioner. |
@ibuildthecloud yeah - maybe a separate PR with the refactor would be easier to merge first? /cc @ehazlett |
I agree. I can certainly do that. |
@hairyhenderson @ehazlett I created #1098 which this PR is based on. #1098 is the GenericProvisioner. |
Cool. I'll have to review this when I get a chance. Thanks for going the extra mile with the GenericProvisioner refactor. One thing this makes me think about is, we really need to change the |
@ibuildthecloud awesome! i was wanting to add this in as well :) Also, huge +1 to the Generic Provisioner! Reviewing :) |
Looking awesome! Tested with #1098 merged. Works well. Only warning i received was this during create:
Perhaps we should override the storage driver in the RancherOS provisioner config? |
return b.DownloadBoot2Docker(latestReleaseUrl) | ||
} | ||
|
||
func (b *B2dUtils) DownloadBoot2Docker(latestReleaseUrl string) error { |
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.
So this name is kind of a misnomer in this PR, huh? Since it looks like it gets used to get RancherOS ISO too.
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.
How about naming method "DownloadISOFromURL". Also the more misleading part is the message below that says "Downloading latest boot2docker release to ". I can change that to "Downloading %s to %s ", latestReleaseUrl, machineName
@ehazlett I wasn't sure what the behavior you wanted here, so I put a warning. I notice in the RHEL drive you just silently set it to devicemapper, so I can do that same. I didn't want to fail because then the default invocation will always fail being that the default is aufs. Should I just silently set it overlay? |
I think I'd prefer a warning in both cases. There's also the possibility of what I mentioned here (#1090 (comment)) but it's a little more drawn out so warnings seem fine to me in the near-term. |
Just a heads up, I'm troubleshooting one issue with this provisioner. w/ VirtualBox I randomly get |
cc @sthulb |
Okay, the ssh error is a bug with RancherOS, not the provisioner. We'll fix on our side. |
@nathanleclaire @sthulb I change my mind. I think this might be an issue in the VirtualBox driver. At https://github.com/docker/machine/blob/master/drivers/virtualbox/virtualbox.go#L396 GetIP() is called but SSH might not be available at that time. There seems to be WaitForSSH() func that libmachine/host.go calls but that is after this call to GetIP. Seems like a WaitForSSH() should be called before GetIP(). |
Ah yes, this is why the virtualbox driver previously did its own waiting on SSH, in addition to the generic method, but we may have taken it out by accident. It should definitely be added back in, I suppose in the |
@ibuildthecloud @nathanleclaire i'm not sure I agree with the warnings on the storage provider. since we don't have a "recommended" how can we warn? To me, whatever is the best for the system should be specified and then potentially warned against if you specify an alternate. |
I think the issue is mainly around, what happens if the user manually specifies |
@nathanleclaire i see -- thanks for clarification. I think I would rather have the provisioners set their recommended and then set the default to Thoughts? |
I like the approach of defaulting to "". But if the user picks a driver that is unsupported we should probably fail as opposed to warning. |
Yeah, failing seems more like the right thing to do here - I have a feeling that even if we log a warning, it's likely to get overlooked. Hopefully users confident enough to set the storage driver will understand that we have their best interests at heart. |
I created #1109 to default the storage driver to "". Also I updated this PR to match that behavior. |
Nice - reviewing this PR is on my immediate todo list. Longer term, we should start a conversation about how we want to orient the VBox driver - personally, I don't think forcing you guys to provide an ISO in the format that our hardcoded-to-boot2docker driver expects is the right approach (I'd prefer the opposite - to force our code to be more general), and I'd like something more flexible to allow users to have Debian etc. VMs as well. |
Reviewed / tested -- works great. LGTM |
This needs #1108 before merge |
#1108 merged |
Signed-off-by: Darren Shepherd <darren@rancher.com>
Signed-off-by: Darren Shepherd <darren@rancher.com>
Signed-off-by: Darren Shepherd <darren@rancher.com>
RancherOS provisioner, tested with VirtualBox and AWS Signed-off-by: Darren Shepherd <darren@rancher.com>
@nathanleclaire I rebased this onto #1143. Now this PR should just run as is. |
I'm getting sporadic errors on removal of a running RancherOS VM: $ ./docker-machine_darwin-amd64 rm rancher
Error removing machine rancher: exit status 1
There was an error removing a machine. To force remove it, pass the -f option. Warning: this might leave it running on the provider. Currently trying to investigate whether this is related to the Virtualbox driver or not. |
Naturally as soon as I post that comment they vanish on me. Trying to dupe now. |
@nathanleclaire I see that error on machines that fail to create. for example if I get a panic, I do ctrl-c, or i manually delete in vbox gui. |
Bingo
Now to try and figure out if it also happens with boot2docker. |
Yeah, these are all finishing creation successfully though (ostensibly at least). Maybe there's a race causing issues. |
@nathanleclaire do you think those random |
Which wait for stopped? |
In virtualbox: https://github.com/docker/machine/blob/master/drivers/virtualbox/virtualbox.go#L437 -- i had to add this for Windows. |
The issue I described seems not easily dupe'able and I suspect either an issue with my VirtualBox setup, or something not directly related to this PR. Tested and I'm in -- LGTM 👍 |
Are there still a few outstanding PRs which need to get merged in before this one? |
Ah, also, I'd like to get something like https://github.com/docker/machine/pull/1090/files#diff-1a523bd9fa0dbf998008b37579210e12R1299 in either this or a followup PR depending on the order in which things resolve. |
@nathanleclaire I don't know of any other outstanding PRs. I can add docs, but it seem like it will overlap with the Red hat provisioner. Should I create a PR based on that one or want until red hat is merged? |
I'd wait, I mostly just a want a commitment to do so once that's in / outlining the basic structure a little bit - I'm ready to merge this whenever you are @ehazlett. |
Support RancherOS, Fixes #992
🎉 |
This allows the DHCP client to correctly report the hostname of the system the the DHCP server. Previously all boot2docker instances would report a static value of 'box'. Fixes docker#1096
This PR adds a RancherOS provisioner. As RancherOS is only officially supported on VirtualBox and AWS right now, this provisioner has been tested with those configurations. To try VirtualBox just do the following:
./docker-machine create -d virtualbox --virtualbox-boot2docker-url https://github.com/rancherio/os/releases/download/v0.3.1-rc2/machine-rancheros.iso dev
The way this provisioner works is two fold. First, we changed RancherOS to look and behave a lot like boot2docker. This makes it so that RancherOS ISO can be used as a drop in replacement for b2d.
Second, the provisioner code largely follows the approach of the Ubuntu provisioner. Instead of duplicating all the code I did a simple refactor of the Ubuntu provisioner and created a Generic provisioner in which I could share a lot of the code between the two provisioners.
Refs #905