Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Provider vsphere bootstrap zone #8007
Conversation
|
"going through all providers" do you mean going through all zones? |
yup, fixed description |
| + var result *environs.StartInstanceResult | ||
| + for i, zone := range zones { | ||
| + if zone != "" { | ||
| + ctx.Infof("Attempting to create instance in availability zone %q", zone) |
wallyworld
Nov 1, 2017
Owner
I am not sure we need this at Info level. We reduced the amount of bootstrap logging. I think it would be sufficient to log with the hardware info a bit later on what zone was used.
wallyworld
approved these changes
Nov 1, 2017
The core bootstrap logic looks ok. I think the provider updates are ok too but am not as sure about those just with a code read. But CI will soon tell us.
| + if errors.IsNotImplemented(err) { | ||
| + // No zone support, so just call StartInstance with | ||
| + // a blank StartInstanceParams.AvailabilityZone. | ||
| + zones = []string{""} |
jameinel
Nov 1, 2017
Owner
Is it better to start with a list containing a single empty entry rather than an empty list? The latter sounds much more like "pick anything" than the former (to me).
| return nil, "", nil, errors.Annotate(err, "cannot start bootstrap instance") | ||
| } | ||
| + var result *environs.StartInstanceResult |
jameinel
Nov 1, 2017
Owner
Zone selection feels like it should be part of StartInstance rather than part of Bootstrap, shouldn't it?
We need similar logic for every instance that we deploy, including load balancing, support for provider networks that only exist in particular zones, etc.
I'm a little concerned that pulling it into Bootstrap handles one case, but we have to re-implement this for all the times we go to provision an instance.
axw
Nov 1, 2017
Member
It was part of StartInstance, but that doesn't work well with provisioner parallelisation. We have two places where we provision instances: bootstrap, and the provisioner. The provisioner was updated to pass in the availability zone, but the work to update bootstrap was deferred. That's this PR.
| + | ||
| + // Attempt creating the instance in each of the availability | ||
| + // zones, unless the args imply a specific zone. | ||
| + zone, err := zonedEnviron.DeriveAvailabilityZone(args) |
jameinel
Nov 1, 2017
Owner
shouldn't Derive be allowed to say there are potentially a couple of zones that might work? (I'm thinking things like spaces where you define a space that does span zones, but it might not span all possible zones.)
Similarly, we've talked about being able to do application level constraints where you specify a subset of the zones.
Something like:
juju deploy postgres --constraints zones=us-east-1a,us-east-1b,us-east-1c
This is more relevant on MAAS where zones are not equivalent. Some of that is abuse of the Zone/Region terminology, but it is a matter of course that people do things like "create a zone grouping some machines together for a specific purpose", and they want to be able to include a range.
| -// connection and in one of the provided zones. | ||
| -func (gce *Connection) AddInstance(spec InstanceSpec, zones ...string) (*Instance, error) { | ||
| +// connection and in the provided zone. | ||
| +func (gce *Connection) AddInstance(spec InstanceSpec, zone string) (*Instance, error) { |
axw
Nov 1, 2017
Member
Maybe you could expand on why?
If we later decide that StartInstance should take a list of zones, then we can put the logic in there to iterate through them. I don't think that logic belongs here.
| - Tools: tools, | ||
| - Constraints: cons, | ||
| - Placement: "zone=z1", | ||
| + AvailabilityZone: "z1", |
jameinel
Nov 1, 2017
Owner
Generally mandating AvailabilityZone as a single value feels like its going the wrong way.
axw
added some commits
Nov 1, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
axw commentedNov 1, 2017
•
Edited 1 time
-
axw
Nov 1, 2017
Description of change
The vsphere provider was failing to bootstrap due to recent provisioner changes. The vsphere provider was essentially expecting a zone to be passed in, whereas the other providers were still going through all zones to cover bootstrap.
We update provider/common.Bootstrap to attempt all "available" availability zones, if no zone is specified, or derivable. Not all providers have been updated yet to remove the now-redundant code to support bootstrap.
QA steps
(should go through each of the zones until it finds one that works)
(should attempt only zone foo)
ditto for gce and openstack providers
Documentation changes
None.
Bug reference
None.