Fail container provisioning if MAAS can't allocate an address for it #8084

Merged
merged 1 commit into from Nov 15, 2017

Conversation

Projects
None yet
4 participants
Member

wupeka commented Nov 15, 2017

Description of change

When setting up a container on MAAS with more than one interface and the non-primary interface can't get an address from MAAS we should fail instead of just issuing a warning.

QA steps

Create a machine in juju on MAAS with interfaces in two spaces, reserve (using MAAS GUI) all addresses in -second- space, try to create a container on this machine with interfaces in those two spaces - it should fail with a proper message.

Bug reference

http://pad.lv/1725356

Contributor

jujubot commented Nov 15, 2017

Can one of the admins verify this patch?

Looks good (other than some small tweaks)!

provider/maas/devices.go
@@ -351,6 +351,7 @@ func (env *maasEnviron) createAndPopulateDevice(params deviceCreatorParams) (gom
for i, iface := range interface_set {
names[i] = iface.Name()
}
+ device.Delete()
@babbageclunk

babbageclunk Nov 15, 2017

Member

It'd be better to do this (and the others) in a deferred function, otherwise there's a risk that someone else adds another error path but forgets the device.Delete().

Something like this after the CreateDevice call

defer func() {
    if err != nil {
        device.Delete()
    }
}()

(You'll need to name the error return.)

- c.Assert(args, gc.HasLen, 1)
- return args[0]
+func getArgs(c *gc.C, calls []testing.StubCall, callNum, argNum int) interface{} {
+ c.Assert(len(calls) >= callNum, jc.IsTrue)
@babbageclunk

babbageclunk Nov 15, 2017

Member

If you use c.Assert(len(calls), gc.Not(jc.LessThan), callNum) here then if it fails it'll show us how many calls there were.

Member

wupeka commented Nov 15, 2017

$$merge$$

Contributor

jujubot commented Nov 15, 2017

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

@@ -343,6 +343,11 @@ func (env *maasEnviron) createAndPopulateDevice(params deviceCreatorParams) (gom
if err != nil {
return nil, errors.Trace(err)
}
+ defer func() {
+ if err != nil {
@babbageclunk

babbageclunk Nov 15, 2017

Member

The err here needs to refer to the error return value rather than just the err local variable. Otherwise the delete won't be done for the error return on line 419, because the function-local-scope err will be nil, even though the if-scope err is non-nil.

The fix is to name the error return err and change the line that calls CreateDevice not to create a local. Then the err in the defer will be the error return and it will catch all errors returned from the func.

@jameinel

jameinel Nov 16, 2017

Owner

this seems to have landed while you were making this comment. should we wait to mark the bug fixed until this is addressed?

@jujubot jujubot merged commit 4cd9d2e into juju:develop Nov 15, 2017

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