Skip to content

Commit 495b905

Browse files
wallyworldwallyworld
committed
Fix default image/instance behaviour
1 parent c0a89e0 commit 495b905

File tree

14 files changed

+375
-223
lines changed

14 files changed

+375
-223
lines changed

‎environs/ec2/config.go

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,19 +12,23 @@ import (
1212

1313
var configChecker = schema.StrictFieldMap(
1414
schema.Fields{
15-
"access-key": schema.String(),
16-
"secret-key": schema.String(),
17-
"region": schema.String(),
18-
"control-bucket": schema.String(),
19-
"public-bucket": schema.String(),
20-
"public-bucket-region": schema.String(),
15+
"access-key": schema.String(),
16+
"secret-key": schema.String(),
17+
"region": schema.String(),
18+
"control-bucket": schema.String(),
19+
"public-bucket": schema.String(),
20+
"public-bucket-region": schema.String(),
21+
"default-image-id": schema.String(),
22+
"default-instance-type": schema.String(),
2123
},
2224
schema.Defaults{
23-
"access-key": "",
24-
"secret-key": "",
25-
"region": "us-east-1",
26-
"public-bucket": "juju-dist",
27-
"public-bucket-region": "us-east-1",
25+
"access-key": "",
26+
"secret-key": "",
27+
"region": "us-east-1",
28+
"public-bucket": "juju-dist",
29+
"public-bucket-region": "us-east-1",
30+
"default-image-id": "",
31+
"default-instance-type": "",
2832
},
2933
)
3034

@@ -57,6 +61,14 @@ func (c *environConfig) secretKey() string {
5761
return c.attrs["secret-key"].(string)
5862
}
5963

64+
func (c *environConfig) defaultImageId() string {
65+
return c.attrs["default-image-id"].(string)
66+
}
67+
68+
func (c *environConfig) defaultInstanceType() string {
69+
return c.attrs["default-instance-type"].(string)
70+
}
71+
6072
func (p environProvider) newConfig(cfg *config.Config) (*environConfig, error) {
6173
valid, err := p.Validate(cfg, nil)
6274
if err != nil {

‎environs/ec2/config_test.go

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,18 @@ var testAuth = aws.Auth{"gopher", "long teeth"}
3838
// when mutated by the mutate function, or that the parse matches the
3939
// given error.
4040
type configTest struct {
41-
config attrs
42-
change attrs
43-
region string
44-
cbucket string
45-
pbucket string
46-
pbucketRegion string
47-
accessKey string
48-
secretKey string
49-
firewallMode config.FirewallMode
50-
err string
41+
config attrs
42+
change attrs
43+
region string
44+
cbucket string
45+
pbucket string
46+
pbucketRegion string
47+
accessKey string
48+
secretKey string
49+
defaultImageId string
50+
defaultInstanceType string
51+
firewallMode config.FirewallMode
52+
err string
5153
}
5254

5355
type attrs map[string]interface{}
@@ -125,6 +127,12 @@ func (t configTest) check(c *C) {
125127
if t.firewallMode != "" {
126128
c.Assert(ecfg.FirewallMode(), Equals, t.firewallMode)
127129
}
130+
if t.defaultImageId != "" {
131+
c.Assert(ecfg.defaultImageId(), Equals, t.defaultImageId)
132+
}
133+
if t.defaultInstanceType != "" {
134+
c.Assert(ecfg.defaultInstanceType(), Equals, t.defaultInstanceType)
135+
}
128136

129137
// check storage buckets are configured correctly
130138
env := e.(*environ)
@@ -245,6 +253,16 @@ var configTests = []configTest{
245253
config: attrs{
246254
"admin-secret": "Futumpsh",
247255
},
256+
}, {
257+
config: attrs{
258+
"default-image-id": "image-id",
259+
},
260+
defaultImageId: "image-id",
261+
}, {
262+
config: attrs{
263+
"default-instance-type": "instance-type",
264+
},
265+
defaultInstanceType: "instance-type",
248266
}, {
249267
config: attrs{},
250268
firewallMode: config.FwInstance,

‎environs/ec2/ec2.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -427,11 +427,13 @@ func (e *environ) startInstance(scfg *startInstanceParams) (environs.Instance, e
427427
return nil, err
428428
}
429429
spec, err := findInstanceSpec(baseURLs, &instances.InstanceConstraint{
430-
Region: e.ecfg().region(),
431-
Series: scfg.series,
432-
Arches: arches,
433-
Constraints: scfg.constraints,
434-
Storage: &storage,
430+
Region: e.ecfg().region(),
431+
Series: scfg.series,
432+
Arches: arches,
433+
Constraints: scfg.constraints,
434+
DefaultInstanceType: e.ecfg().defaultInstanceType(),
435+
DefaultImageId: e.ecfg().defaultImageId(),
436+
Storage: &storage,
435437
})
436438
if err != nil {
437439
return nil, err

‎environs/ec2/export_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,12 @@ var TestImagesData = []jujutest.FileContent{
365365
"virt": "pv",
366366
"region": "test",
367367
"id": "ami-02000034"
368+
},
369+
"test2pe": {
370+
"root_store": "ebs",
371+
"virt": "pv",
372+
"region": "test",
373+
"id": "ami-02000035"
368374
}
369375
},
370376
"pubname": "ubuntu-raring-13.04-i386-server-20121218",

‎environs/ec2/image_test.go

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,13 @@ func (s *specSuite) TearDownSuite(c *C) {
4747
}
4848

4949
var findInstanceSpecTests = []struct {
50-
series string
51-
arches []string
52-
cons string
53-
itype string
54-
image string
50+
series string
51+
arches []string
52+
cons string
53+
defaultItype string
54+
defaultImage string
55+
itype string
56+
image string
5557
}{
5658
{
5759
series: "precise",
@@ -117,6 +119,18 @@ var findInstanceSpecTests = []struct {
117119
cons: "arch=amd64",
118120
itype: "cc1.4xlarge",
119121
image: "ami-01000035",
122+
}, {
123+
series: "precise",
124+
arches: both,
125+
defaultItype: "m1.medium",
126+
itype: "m1.medium",
127+
image: "ami-00000033",
128+
}, {
129+
series: "raring",
130+
arches: both,
131+
itype: "m1.small",
132+
defaultImage: "ami-02000035",
133+
image: "ami-02000035",
120134
},
121135
}
122136

@@ -125,11 +139,13 @@ func (s *specSuite) TestFindInstanceSpec(c *C) {
125139
c.Logf("test %d", i)
126140
storage := ebsStorage
127141
spec, err := findInstanceSpec([]string{"test:"}, &instances.InstanceConstraint{
128-
Region: "test",
129-
Series: t.series,
130-
Arches: t.arches,
131-
Constraints: constraints.MustParse(t.cons),
132-
Storage: &storage,
142+
Region: "test",
143+
Series: t.series,
144+
Arches: t.arches,
145+
Constraints: constraints.MustParse(t.cons),
146+
DefaultInstanceType: t.defaultItype,
147+
DefaultImageId: t.defaultImage,
148+
Storage: &storage,
133149
})
134150
c.Assert(err, IsNil)
135151
c.Check(spec.InstanceTypeName, Equals, t.itype)
@@ -138,10 +154,12 @@ func (s *specSuite) TestFindInstanceSpec(c *C) {
138154
}
139155

140156
var findInstanceSpecErrorTests = []struct {
141-
series string
142-
arches []string
143-
cons string
144-
err string
157+
series string
158+
arches []string
159+
cons string
160+
defaultInstanceType string
161+
defaultImageId string
162+
err string
145163
}{
146164
{
147165
series: "bad",
@@ -150,7 +168,17 @@ var findInstanceSpecErrorTests = []struct {
150168
}, {
151169
series: "precise",
152170
arches: []string{"arm"},
153-
err: `no "precise" images in test with arches \[arm\], and no default specified`,
171+
err: `no "precise" images in test with arches \[arm\], and no override specified`,
172+
}, {
173+
series: "precise",
174+
arches: both,
175+
defaultInstanceType: "bad.type",
176+
err: `invalid default instance type name "bad.type"`,
177+
}, {
178+
series: "precise",
179+
arches: both,
180+
defaultImageId: "bad",
181+
err: `invalid default image id "bad"`,
154182
}, {
155183
series: "raring",
156184
arches: both,
@@ -163,10 +191,12 @@ func (s *specSuite) TestFindInstanceSpecErrors(c *C) {
163191
for i, t := range findInstanceSpecErrorTests {
164192
c.Logf("test %d", i)
165193
_, err := findInstanceSpec([]string{"test:"}, &instances.InstanceConstraint{
166-
Region: "test",
167-
Series: t.series,
168-
Arches: t.arches,
169-
Constraints: constraints.MustParse(t.cons),
194+
Region: "test",
195+
Series: t.series,
196+
Arches: t.arches,
197+
Constraints: constraints.MustParse(t.cons),
198+
DefaultInstanceType: t.defaultInstanceType,
199+
DefaultImageId: t.defaultImageId,
170200
})
171201
c.Check(err, ErrorMatches, t.err)
172202
}

‎environs/instances/image.go

Lines changed: 19 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,9 @@ type InstanceConstraint struct {
1616
Series string
1717
Arches []string
1818
Constraints constraints.Value
19-
DefaultInstanceType string // the default instance type to use if none matches the constraints
20-
DefaultImageId string // the default image to use if none matches the constraints
19+
DefaultInstanceType string // the instance type to use if multiple matching ones are found
20+
DefaultImageId string // the image to use if multiple matching ones are found
21+
OverrideImageId string // ignore any known, published images, use this image
2122
// Optional filtering criteria not supported by all providers. These attributes are not specified
2223
// by the user as a constraint but rather passed in by the provider implementation to restrict the
2324
// choice of available images.
@@ -31,72 +32,44 @@ type InstanceSpec struct {
3132
Image Image
3233
}
3334

34-
// minMemoryHeuristic is the assumed minimum amount of memory (in MB) we prefer in order to run a server (1GB)
35-
const minMemoryHeuristic = 1024
36-
3735
// FindInstanceSpec returns an InstanceSpec satisfying the supplied InstanceConstraint.
38-
// r has been set up to read from a file containing Ubuntu cloud guest images availability data. A query
39-
// interface for EC2 images is exposed at http://cloud-images.ubuntu.com/query. Other cloud providers may
40-
// provide similar files for their own images. e.g. the Openstack provider has been configured to look for
41-
// cloud image availability files in the cloud's control and public storage containers.
42-
// For more information on the image availability file format, see https://help.ubuntu.com/community/UEC/Images.
36+
// possibleImages contains a list of images matching the InstanceConstraint.
4337
// allInstanceTypes provides information on every known available instance type (name, memory, cpu cores etc) on
44-
// which instances can be run.
38+
// which instances can be run. The InstanceConstraint is used to filter allInstanceTypes and then a suitable image
39+
// compatible with the matching instance types is returned.
4540
func FindInstanceSpec(possibleImages []Image, ic *InstanceConstraint, allInstanceTypes []InstanceType) (*InstanceSpec, error) {
4641
matchingTypes, err := getMatchingInstanceTypes(ic, allInstanceTypes)
4742
if err != nil {
48-
// There are no instance types matching the supplied constraints. If the user has specifically
49-
// asked for a nominated default instance type to be used as a fallback and that is invalid, we
50-
// report the error. Otherwise we continue to look for an instance type that we can use as a last resort.
51-
if len(allInstanceTypes) == 0 || ic.DefaultInstanceType != "" {
52-
return nil, err
53-
}
54-
// No matching instance types were found, so the fallback is to:
55-
// 1. Sort by memory and find the smallest matching both the required architecture
56-
// and our own heuristic: minimum amount of memory required to run a realistic server, or
57-
// 2. Sort by memory in reverse order and return the largest one, which will hopefully work,
58-
// albeit not the best match
59-
60-
archCons := &InstanceConstraint{Arches: ic.Arches}
61-
fallbackTypes, fberr := getMatchingInstanceTypes(archCons, allInstanceTypes)
62-
// If there's an error getting the fallback instance, return the original error.
63-
if fberr != nil {
64-
return nil, err
65-
}
66-
sort.Sort(byMemory(fallbackTypes))
67-
// 1. check for smallest instance type that can realistically run a server
68-
for _, itype := range fallbackTypes {
69-
if itype.Mem >= minMemoryHeuristic {
70-
matchingTypes = []InstanceType{itype}
71-
break
72-
}
73-
}
74-
if len(matchingTypes) == 0 {
75-
// 2. just get the one with the largest memory
76-
matchingTypes = []InstanceType{fallbackTypes[len(fallbackTypes)-1]}
77-
}
43+
return nil, err
7844
}
7945

8046
sort.Sort(byArch(possibleImages))
8147
for _, itype := range matchingTypes {
48+
typeMatch := false
8249
for _, image := range possibleImages {
8350
if image.match(itype) {
84-
return &InstanceSpec{itype.Id, itype.Name, image}, nil
51+
typeMatch = true
52+
if ic.DefaultImageId == "" || ic.DefaultImageId == image.Id {
53+
return &InstanceSpec{itype.Id, itype.Name, image}, nil
54+
}
8555
}
8656
}
57+
if typeMatch && ic.DefaultImageId != "" {
58+
return nil, fmt.Errorf("invalid default image id %q", ic.DefaultImageId)
59+
}
8760
}
88-
// if no matching image is found for whatever reason, use the default if one is specified.
89-
if ic.DefaultImageId != "" && len(matchingTypes) > 0 {
61+
// if no matching image is found for whatever reason, use the override if one is specified.
62+
if ic.OverrideImageId != "" && len(matchingTypes) > 0 {
9063
spec := &InstanceSpec{
9164
InstanceTypeId: matchingTypes[0].Id,
9265
InstanceTypeName: matchingTypes[0].Name,
93-
Image: Image{Id: ic.DefaultImageId, Arch: ic.Arches[0]},
66+
Image: Image{Id: ic.OverrideImageId, Arch: ic.Arches[0]},
9467
}
9568
return spec, nil
9669
}
9770

9871
if len(possibleImages) == 0 || len(matchingTypes) == 0 {
99-
return nil, fmt.Errorf("no %q images in %s with arches %s, and no default specified",
72+
return nil, fmt.Errorf("no %q images in %s with arches %s, and no override specified",
10073
ic.Series, ic.Region, ic.Arches)
10174
}
10275

@@ -107,15 +80,6 @@ func FindInstanceSpec(possibleImages []Image, ic *InstanceConstraint, allInstanc
10780
return nil, fmt.Errorf("no %q images in %s matching instance types %v", ic.Series, ic.Region, names)
10881
}
10982

110-
//byMemory is used to sort a slice of instance types by the amount of RAM they have.
111-
type byMemory []InstanceType
112-
113-
func (s byMemory) Len() int { return len(s) }
114-
func (s byMemory) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
115-
func (s byMemory) Less(i, j int) bool {
116-
return s[i].Mem < s[j].Mem
117-
}
118-
11983
// Image holds the attributes that vary amongst relevant images for
12084
// a given series in a given region.
12185
type Image struct {

0 commit comments

Comments
 (0)