/juju

environs: introduce AvailabilityZoneError #8109

Merged
merged 2 commits into from Nov 21, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@axw @jujubot @jameinel
@axw
Member

axw commented Nov 20, 2017 •
Edited 1 time

Description of change

Introduce the environs.AvailabilityZoneError
interface, and an accompanying helper function
IsAvailabilityZoneIndependent. Providers'
StartInstance methods may return an error
implementing environs.AvailabilityZoneError
to indicate that an error was not influenced
by the specified availability zone. This is
used by the provisioner and bootstrap code
to decide whether or not to attempt with an
alternative availability zone.

The existing ErrAvailabilityZoneFailed error
value has been removed, and all providers
have been updated to use the new interface
where appropriate. A helper function,
ZoneIndependentError, has been added to the
provider/common package which wraps a given
function such that it satisfies
environs.IsAvailabilityZoneIndependent.

QA steps

bootstrap
deploy ubuntu -n 3
(check units spread over AZs)
destroy-controller

repeat in a cloud with broken AZs, check that the provisioner retries after failing in one AZ

Documentation changes

None.

Bug reference

https://bugs.launchpad.net/juju/+bug/1732564
@axw
Member

axw commented Nov 20, 2017

providers obscure provisioning errors to conform to AZ distribution
environs/errors.go
+// to be specific to an availability zone by default, so that they can
+// be retried in another availability zone.
+func IsAvailabilityZoneIndependent(err error) bool {
+ if err, ok := err.(AvailabilityZoneError); ok {
@jameinel

jameinel Nov 20, 2017

Owner

Do you need to do both? I thought errors.Cause(err) returns err if its not a wrapped error

@axw

axw Nov 20, 2017

Member

No, this was due to a misguided change I made and backed out. Simplified, thanks.

provider/common/bootstrap.go
+ }
+ // This is the last zone in the list, error.
+ if len(zones) > 1 {
+ return nil, "", nil, errors.New("cannot start bootstrap instance in any availability zone")
@jameinel

jameinel Nov 20, 2017

Owner

if we have a list, is it helpful to the user to do something like:

"cannot start bootstrap instance in availability zones: %v", strings.Join(zones, ",")
?

@axw

axw Nov 20, 2017

Member

sure, done

provider/ec2/environ.go
+ if err != nil {
+ return nil, &common.StartInstanceError{
+ Err: errors.Trace(err),
+ ZoneIndependent: !errors.IsNotValid(err),
@jameinel

jameinel Nov 20, 2017

Owner

I find these hard to read. It is zone independent if it is not Not Valid?

Maybe a comment that explains the boolean logic in words?

@axw

axw Nov 20, 2017

Member

changed to

zoneSpecific := errors.IsNotValid(err)
...
ZoneIndependent: !zoneSpecific,

and also added a comment

provider/ec2/environ.go
- return nil, errors.Trace(err)
+ return nil, &common.StartInstanceError{
+ Err: errors.Trace(err),
+ ZoneIndependent: true,
@jameinel

jameinel Nov 20, 2017

Owner

Is the supported types a regional thing? (So while a given zone may not have an instance of a type, it still knows the type)

@axw

axw Nov 20, 2017

Member

Availability is dependent on region, but there's also capacity for AZs I'm pretty sure. We don't check that up front (not sure if you can?) - so there's no AZ specific error coming from that method call.

@jameinel

jameinel Nov 21, 2017

Owner

Don't these need to use the ZoneIndependentError function?

provider/gce/environ_broker.go
}
if err := validateAvailabilityZoneConsistency(args.AvailabilityZone, volumeAttachmentsZone); err != nil {
- return nil, errors.Wrap(errors.Trace(err), environs.ErrAvailabilityZoneFailed)
+ return nil, &common.StartInstanceError{
@jameinel

jameinel Nov 20, 2017

Owner

Should these actually be a Function because then we get proper Trace behavior?

@axw

axw Nov 20, 2017 •
Edited 1 time

Member

I don't think so. That would lead to source context being added wherever the common function is, which isn't helpful. Tracing should be done at the point where it's returned. Also, adding a function would make this even more boilerplatey, unless it were a function specific to AZs.

provider/gce/environ_broker.go
+func (env *environ) newRawInstance(args environs.StartInstanceParams, spec *instances.InstanceSpec) (_ *google.Instance, err error) {
+
+ zoneIndependent := true
+ defer func() {
@jameinel

jameinel Nov 20, 2017

Owner

I realize there are a number of returns here, but this feels a bit like action-at-a-distance that will ultimately be hard to understand why we're getting ZoneIndependent.

maybe its fine. Though I'd still like to keep Tracing information.
Is it possible to use something like errors.Wrap?

@axw

axw Nov 21, 2017

Member

Fair enough. I'll inline or use helpers where it feels appropriate.

provider/maas/environ.go
+ Err: err,
+ ZoneIndependent: true,
+ }
+ }
@jameinel

jameinel Nov 20, 2017

Owner

Why not just always wrap but pass in:
ZoneIndependent: zoneIndependent
?

@axw

axw Nov 21, 2017

Member

Just felt like unnecessary layering, but it's irrelevant now anyway, as I've dropped the defer.

@axw
environs: introduce AvailabilityZoneError 
Introduce the environs.AvailabilityZoneError
interface, and an accompanying helper function
IsAvailabilityZoneIndependent. Providers'
StartInstance methods may return an error
implementing environs.AvailabilityZoneError
to indicate that an error was not influenced
by the specified availability zone. This is
used by the provisioner and bootstrap code
to decide whether or not to attempt with an
alternative availability zone.

The existing ErrAvailabilityZoneFailed error
value has been removed, and all providers
have been updated to use the new interface
where appropriate. An error type,
StartInstanceError, has been added to the
provider/common package which implements the
interface.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
0c72398
@jameinel

jameinel approved these changes Nov 21, 2017
View changes

As long as we can get the correct traceback and StartInstanceError doesn't hide the underlying "Err" based traceback, LGTM.

provider/openstack/provider.go
- return nil, errors.New("missing controller UUID")
+func (e *Environ) StartInstance(args environs.StartInstanceParams) (_ *environs.StartInstanceResult, err error) {
+ zoneIndependentError := func(err error) error {
+ if err == nil {
@jameinel

jameinel Nov 21, 2017

Owner

Is there any reason why this can't just be common.ZoneIndependentError() ?

@jameinel

jameinel approved these changes Nov 21, 2017
View changes

You have a bare typecast and you missed the ec2 provider, but otherwise LGTM

provider/common/errors.go
+ return nil
+ }
+ wrapped := errors.Wrap(err, zoneIndependentError{err})
+ wrapped.(*errors.Err).SetLocation(1)
@jameinel

jameinel Nov 21, 2017

Owner

This doesn't seem safe (fmt.Errorf() won't return this type, etc)
Can we do:
if traceErr, ok := wrapped.(*errors.Err); ok {
traceErro.SetLocation(1)
}
That way we don't blind cast and get a panic()

@axw

axw Nov 21, 2017

Member

as mentioned on IRC: this is operating on the result of errors.Wrap, which is always an *errors.Err. there's a test, so if that changes then we'll catch it

provider/ec2/environ.go
- return nil, errors.Trace(err)
+ return nil, &common.StartInstanceError{
+ Err: errors.Trace(err),
+ ZoneIndependent: true,
@jameinel

jameinel Nov 20, 2017

Owner

Is the supported types a regional thing? (So while a given zone may not have an instance of a type, it still knows the type)

@axw

axw Nov 20, 2017

Member

Availability is dependent on region, but there's also capacity for AZs I'm pretty sure. We don't check that up front (not sure if you can?) - so there's no AZ specific error coming from that method call.

@jameinel

jameinel Nov 21, 2017

Owner

Don't these need to use the ZoneIndependentError function?

@axw
Member

axw commented Nov 21, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 21, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju
@jujubot
Contributor

jujubot commented Nov 21, 2017

Build failed: Tests failed
build url: http://ci.jujucharms.com/job/github-merge-juju/562
@axw
provider/common: ZoneIndependentError 
Remove StartInstanceError, and replace it with
the function ZoneIndependentError. This new
function wraps the given error such that it
will satisfy environs.IsAvailabilityZoneIndependent.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
e26c8c8
@axw
Member

axw commented Nov 21, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Nov 21, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

@jujubot jujubot merged commit 59d9b74 into juju:develop Nov 21, 2017
1 check passed

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment