Skip to content

Commit 98a2022

Browse files
jujubotjujubot
authored
Merge pull request juju#12609 from SimonRichardson/ec2-fix-panic
juju#12609 This was a fun one to locate. There are two issues here: 1. Using a slice with holes always leads to bad expectations 2. Expecting an interface to be backed by one instance seems bad The following PR ensures that we collect the zones from both `amzInstance` and `sdkInstance`. As we seem to be transitioning between libraries we need to ensure that we correctly handle those locations. There aren't any unit tests in here so I did add some to at least cover us for those scenarios. ---- I wish there were a better lint guard around this as it's bad practice to just convert the type without checking. ## QA steps It expects that you have setup the acceptance tests from the README. ```sh $ ./assess_model_migration.py aws $GOPATH/bin/juju /tmp/artifacts ```
2 parents fdd1696 + ed72236 commit 98a2022

21 files changed

+197
-82
lines changed

‎provider/common/availabilityzones.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ type ZonedEnviron interface {
2323
// InstanceAvailabilityZoneNames returns the names of the availability
2424
// zones for the specified instances. The error returned follows the same
2525
// rules as Environ.Instances.
26-
InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error)
26+
InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error)
2727

2828
// DeriveAvailabilityZones attempts to derive availability zones from
2929
// the specified StartInstanceParams.
@@ -114,8 +114,8 @@ func AvailabilityZoneAllocations(
114114
return nil, nil
115115
}
116116

117-
for i, id := range group {
118-
zone := instanceZones[i]
117+
for _, id := range group {
118+
zone := instanceZones[id]
119119
if zone == "" {
120120
continue
121121
}

‎provider/common/availabilityzones_test.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,14 @@ func (s *AvailabilityZoneSuite) SetUpSuite(c *gc.C) {
5353

5454
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsAllRunningInstances(c *gc.C) {
5555
var called int
56-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
56+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
5757
c.Assert(ids, gc.DeepEquals, []instance.Id{"inst0", "inst1", "inst2"})
5858
called++
59-
return []string{"az0", "az1", "az2"}, nil
59+
return map[instance.Id]string{
60+
"inst0": "az0",
61+
"inst1": "az1",
62+
"inst2": "az2",
63+
}, nil
6064
})
6165
zoneInstances, err := common.AvailabilityZoneAllocations(&s.env, s.callCtx, nil)
6266
c.Assert(called, gc.Equals, 1)
@@ -84,10 +88,10 @@ func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsAllRunningInstanc
8488

8589
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsPartialInstances(c *gc.C) {
8690
var called int
87-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
91+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
8892
c.Assert(ids, gc.DeepEquals, []instance.Id{"nichts", "inst1", "null", "inst2"})
8993
called++
90-
return []string{"", "az1", "", "az1"}, environs.ErrPartialInstances
94+
return map[instance.Id]string{"inst1": "az1", "inst2": "az1"}, environs.ErrPartialInstances
9195
})
9296
zoneInstances, err := common.AvailabilityZoneAllocations(&s.env, s.callCtx, []instance.Id{"nichts", "inst1", "null", "inst2"})
9397
c.Assert(called, gc.Equals, 1)
@@ -104,7 +108,7 @@ func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsPartialInstances(
104108
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsInstanceAvailabilityZonesErrors(c *gc.C) {
105109
returnErr := fmt.Errorf("whatever")
106110
var called int
107-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
111+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
108112
called++
109113
return nil, returnErr
110114
})
@@ -116,7 +120,7 @@ func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsInstanceAvailabil
116120

117121
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsInstanceAvailabilityZonesNoInstances(c *gc.C) {
118122
var called int
119-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
123+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
120124
called++
121125
return nil, environs.ErrNoInstances
122126
})
@@ -128,10 +132,10 @@ func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsInstanceAvailabil
128132

129133
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsNoZones(c *gc.C) {
130134
var calls []string
131-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
135+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
132136
c.Assert(ids, gc.DeepEquals, []instance.Id{"inst0", "inst1", "inst2"})
133137
calls = append(calls, "InstanceAvailabilityZoneNames")
134-
return []string{"", "", ""}, nil
138+
return make(map[instance.Id]string, 3), nil
135139
})
136140
s.PatchValue(&s.env.availabilityZones, func(context.ProviderCallContext) (network.AvailabilityZones, error) {
137141
calls = append(calls, "AvailabilityZones")
@@ -145,10 +149,10 @@ func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsNoZones(c *gc.C)
145149

146150
func (s *AvailabilityZoneSuite) TestAvailabilityZoneAllocationsErrors(c *gc.C) {
147151
var calls []string
148-
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
152+
s.PatchValue(&s.env.instanceAvailabilityZoneNames, func(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
149153
c.Assert(ids, gc.DeepEquals, []instance.Id{"inst0", "inst1", "inst2"})
150154
calls = append(calls, "InstanceAvailabilityZoneNames")
151-
return []string{"", "", ""}, nil
155+
return make(map[instance.Id]string, 3), nil
152156
})
153157
resultErr := fmt.Errorf("u can haz no az")
154158
s.PatchValue(&s.env.availabilityZones, func(context.ProviderCallContext) (network.AvailabilityZones, error) {

‎provider/common/mock_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func (env *mockEnviron) StorageProvider(t jujustorage.ProviderType) (jujustorage
9898
}
9999

100100
type availabilityZonesFunc func(context.ProviderCallContext) (network.AvailabilityZones, error)
101-
type instanceAvailabilityZoneNamesFunc func(context.ProviderCallContext, []instance.Id) ([]string, error)
101+
type instanceAvailabilityZoneNamesFunc func(context.ProviderCallContext, []instance.Id) (map[instance.Id]string, error)
102102
type deriveAvailabilityZonesFunc func(context.ProviderCallContext, environs.StartInstanceParams) ([]string, error)
103103

104104
type mockZonedEnviron struct {
@@ -112,7 +112,7 @@ func (env *mockZonedEnviron) AvailabilityZones(ctx context.ProviderCallContext)
112112
return env.availabilityZones(ctx)
113113
}
114114

115-
func (env *mockZonedEnviron) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
115+
func (env *mockZonedEnviron) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
116116
return env.instanceAvailabilityZoneNames(ctx, ids)
117117
}
118118

‎provider/common/mocks/zoned_environ.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎provider/dummy/environs.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,7 +1520,7 @@ func (env *environ) AvailabilityZones(ctx context.ProviderCallContext) (corenetw
15201520
}
15211521

15221522
// InstanceAvailabilityZoneNames implements environs.ZonedEnviron.
1523-
func (env *environ) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
1523+
func (env *environ) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
15241524
if err := env.checkBroken("InstanceAvailabilityZoneNames"); err != nil {
15251525
return nil, errors.NotSupportedf("instance availability zones")
15261526
}
@@ -1530,17 +1530,17 @@ func (env *environ) InstanceAvailabilityZoneNames(ctx context.ProviderCallContex
15301530
}
15311531
azMaxIndex := len(availabilityZones) - 1
15321532
azIndex := 0
1533-
returnValue := make([]string, len(ids))
1534-
for i := range ids {
1533+
returnValue := make(map[instance.Id]string, 0)
1534+
for _, id := range ids {
15351535
if availabilityZones[azIndex].Available() {
1536-
returnValue[i] = availabilityZones[azIndex].Name()
1536+
returnValue[id] = availabilityZones[azIndex].Name()
15371537
} else {
15381538
// Based on knowledge of how the AZs are setup above
15391539
// in AvailabilityZones()
1540-
azIndex += 1
1541-
returnValue[i] = availabilityZones[azIndex].Name()
1540+
azIndex++
1541+
returnValue[id] = availabilityZones[azIndex].Name()
15421542
}
1543-
azIndex += 1
1543+
azIndex++
15441544
if azIndex == azMaxIndex {
15451545
azIndex = 0
15461546
}

‎provider/ec2/environ.go

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -266,19 +266,38 @@ func (e *environ) AvailabilityZones(ctx context.ProviderCallContext) (corenetwor
266266

267267
// InstanceAvailabilityZoneNames returns the availability zone names for each
268268
// of the specified instances.
269-
func (e *environ) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) ([]string, error) {
269+
func (e *environ) InstanceAvailabilityZoneNames(ctx context.ProviderCallContext, ids []instance.Id) (map[instance.Id]string, error) {
270270
instances, err := e.Instances(ctx, ids)
271271
if err != nil && err != environs.ErrPartialInstances {
272-
return nil, err
272+
return nil, errors.Trace(err)
273273
}
274-
zones := make([]string, len(instances))
275-
for i, inst := range instances {
274+
275+
return gatherAvailabilityZones(instances), nil
276+
}
277+
278+
// AvailabilityZoner defines a institute interface for getting an az from an
279+
// instance.
280+
type AvailabilityZoner interface {
281+
AvailabilityZone() (string, bool)
282+
}
283+
284+
func gatherAvailabilityZones(instances []instances.Instance) map[instance.Id]string {
285+
zones := make(map[instance.Id]string, 0)
286+
for _, inst := range instances {
276287
if inst == nil {
277288
continue
278289
}
279-
zones[i] = inst.(*amzInstance).AvailZone
290+
t, ok := inst.(AvailabilityZoner)
291+
if !ok {
292+
continue
293+
}
294+
az, ok := t.AvailabilityZone()
295+
if !ok {
296+
continue
297+
}
298+
zones[inst.Id()] = az
280299
}
281-
return zones, err
300+
return zones
282301
}
283302

284303
// DeriveAvailabilityZones is part of the common.ZonedEnviron interface.
@@ -321,7 +340,7 @@ func (e *environ) parsePlacement(ctx context.ProviderCallContext, placement stri
321340
matcher := CreateSubnetMatcher(value)
322341
// Get all known subnets, look for a match
323342
allSubnets := []string{}
324-
subnetResp, vpcId, err := e.subnetsForVPC(ctx)
343+
subnetResp, vpcID, err := e.subnetsForVPC(ctx)
325344
if err != nil {
326345
return nil, errors.Trace(err)
327346
}
@@ -346,7 +365,7 @@ func (e *environ) parsePlacement(ctx context.ProviderCallContext, placement stri
346365
logger.Debugf("found a matching subnet (%v) but couldn't find the AZ", subnet)
347366
}
348367
}
349-
logger.Debugf("searched for subnet %q, did not find it in all subnets %v for vpc-id %q", value, allSubnets, vpcId)
368+
logger.Debugf("searched for subnet %q, did not find it in all subnets %v for vpc-id %q", value, allSubnets, vpcID)
350369
}
351370
return nil, fmt.Errorf("unknown placement directive: %v", placement)
352371
}
@@ -955,7 +974,7 @@ func tagResources(e *amzec2.EC2, ctx context.ProviderCallContext, tags map[strin
955974
}
956975
ec2Tags := make([]amzec2.Tag, 0, len(tags))
957976
for k, v := range tags {
958-
ec2Tags = append(ec2Tags, amzec2.Tag{k, v})
977+
ec2Tags = append(ec2Tags, amzec2.Tag{Key: k, Value: v})
959978
}
960979
var err error
961980
for a := shortAttempt.Start(); a.Next(); {
@@ -971,7 +990,7 @@ func tagRootDisk(e *amzec2.EC2, ctx context.ProviderCallContext, tags map[string
971990
if len(tags) == 0 {
972991
return nil
973992
}
974-
findVolumeId := func(inst *amzec2.Instance) string {
993+
findVolumeID := func(inst *amzec2.Instance) string {
975994
for _, m := range inst.BlockDeviceMappings {
976995
if m.DeviceName != inst.RootDeviceName {
977996
continue
@@ -982,13 +1001,13 @@ func tagRootDisk(e *amzec2.EC2, ctx context.ProviderCallContext, tags map[string
9821001
}
9831002
// Wait until the instance has an associated EBS volume in the
9841003
// block-device-mapping.
985-
volumeId := findVolumeId(inst)
1004+
volumeID := findVolumeID(inst)
9861005
// TODO(katco): 2016-08-09: lp:1611427
9871006
waitRootDiskAttempt := utils.AttemptStrategy{
9881007
Total: 5 * time.Minute,
9891008
Delay: 5 * time.Second,
9901009
}
991-
for a := waitRootDiskAttempt.Start(); volumeId == "" && a.Next(); {
1010+
for a := waitRootDiskAttempt.Start(); volumeID == "" && a.Next(); {
9921011
resp, err := e.Instances([]string{inst.InstanceId}, nil)
9931012
if err != nil {
9941013
err = errors.Annotate(maybeConvertCredentialError(err, ctx), "cannot fetch instance information")
@@ -1001,13 +1020,13 @@ func tagRootDisk(e *amzec2.EC2, ctx context.ProviderCallContext, tags map[string
10011020
}
10021021
if len(resp.Reservations) > 0 && len(resp.Reservations[0].Instances) > 0 {
10031022
inst = &resp.Reservations[0].Instances[0]
1004-
volumeId = findVolumeId(inst)
1023+
volumeID = findVolumeID(inst)
10051024
}
10061025
}
1007-
if volumeId == "" {
1026+
if volumeID == "" {
10081027
return errors.New("timed out waiting for EBS volume to be associated")
10091028
}
1010-
return tagResources(e, ctx, tags, volumeId)
1029+
return tagResources(e, ctx, tags, volumeID)
10111030
}
10121031

10131032
var runInstances = _runInstances

‎provider/ec2/environ_test.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,21 @@ package ec2
66
import (
77
"strings"
88

9+
"github.com/aws/aws-sdk-go/service/ec2"
910
gomock "github.com/golang/mock/gomock"
1011
"github.com/juju/errors"
1112
jc "github.com/juju/testing/checkers"
1213
amzec2 "gopkg.in/amz.v3/ec2"
1314
gc "gopkg.in/check.v1"
1415

1516
"github.com/juju/juju/core/constraints"
17+
"github.com/juju/juju/core/instance"
1618
"github.com/juju/juju/core/network"
1719
"github.com/juju/juju/core/network/firewall"
1820
"github.com/juju/juju/environs"
1921
"github.com/juju/juju/environs/config"
2022
"github.com/juju/juju/environs/context"
23+
"github.com/juju/juju/environs/instances"
2124
"github.com/juju/juju/environs/simplestreams"
2225
)
2326

@@ -407,3 +410,47 @@ func (*Suite) TestGetValidSubnetZoneMapIntersectionSelectsCorrectIndex(c *gc.C)
407410
c.Assert(err, jc.ErrorIsNil)
408411
c.Check(subnetZones, gc.DeepEquals, allSubnetZones[1])
409412
}
413+
414+
func (*Suite) TestGatherNilAZ(c *gc.C) {
415+
az := gatherAvailabilityZones(nil)
416+
c.Assert(az, gc.HasLen, 0)
417+
}
418+
419+
func (*Suite) TestGatherEmptyAZ(c *gc.C) {
420+
instances := []instances.Instance{}
421+
az := gatherAvailabilityZones(instances)
422+
c.Assert(az, gc.HasLen, 0)
423+
}
424+
425+
func (*Suite) TestGatherAZ(c *gc.C) {
426+
instances := []instances.Instance{
427+
&amzInstance{
428+
Instance: &amzec2.Instance{
429+
InstanceId: "id1",
430+
AvailZone: "aaa",
431+
},
432+
},
433+
&sdkInstance{
434+
i: &ec2.Instance{
435+
InstanceId: ptrString("id2"),
436+
Placement: &ec2.Placement{
437+
AvailabilityZone: ptrString("bbb"),
438+
},
439+
},
440+
},
441+
&sdkInstance{
442+
i: &ec2.Instance{
443+
InstanceId: ptrString("id3"),
444+
},
445+
},
446+
}
447+
az := gatherAvailabilityZones(instances)
448+
c.Assert(az, gc.DeepEquals, map[instance.Id]string{
449+
"id1": "aaa",
450+
"id2": "bbb",
451+
})
452+
}
453+
454+
func ptrString(s string) *string {
455+
return &s
456+
}

‎provider/ec2/instance.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,14 @@ func (inst *amzInstance) Id() instance.Id {
3737
return instance.Id(inst.InstanceId)
3838
}
3939

40+
// AvailabilityZone returns the underlying az for an instance.
41+
func (inst *amzInstance) AvailabilityZone() (string, bool) {
42+
if inst.Instance == nil {
43+
return "", false
44+
}
45+
return inst.Instance.AvailZone, true
46+
}
47+
4048
// Status returns the status of this EC2 instance.
4149
func (inst *amzInstance) Status(ctx context.ProviderCallContext) instance.Status {
4250
// pending | running | shutting-down | terminated | stopping | stopped
@@ -145,6 +153,15 @@ func (inst *sdkInstance) Id() instance.Id {
145153
return instance.Id(*inst.i.InstanceId)
146154
}
147155

156+
// AvailabilityZone returns the underlying az for an instance.
157+
func (inst *sdkInstance) AvailabilityZone() (string, bool) {
158+
if inst.i == nil || inst.i.Placement == nil ||
159+
inst.i.Placement.AvailabilityZone == nil {
160+
return "", false
161+
}
162+
return *inst.i.Placement.AvailabilityZone, true
163+
}
164+
148165
// Status returns the status of this EC2 instance.
149166
func (inst *sdkInstance) Status(ctx context.ProviderCallContext) instance.Status {
150167
if inst.i.State == nil || inst.i.State.Name == nil {

0 commit comments

Comments
 (0)