Skip to content

Commit 18f5a89

Browse files
wallyworldwallyworld
committed
Tweak instance type matching so that it behaves sensibly
1 parent 70d7ae3 commit 18f5a89

File tree

2 files changed

+83
-39
lines changed

2 files changed

+83
-39
lines changed

‎environs/instances/instancetype.go

Lines changed: 34 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -74,53 +74,52 @@ func filterArches(src, filter []string) (dst []string) {
7474
// minMemoryHeuristic is the assumed minimum amount of memory (in MB) we prefer in order to run a server (1GB)
7575
const minMemoryHeuristic = 1024
7676

77+
// matchingTypesForConstraint returns instance types from allTypes which match cons.
78+
func matchingTypesForConstraint(allTypes []InstanceType, cons constraints.Value) []InstanceType {
79+
var matchingTypes []InstanceType
80+
for _, itype := range allTypes {
81+
itype, ok := itype.match(cons)
82+
if !ok {
83+
continue
84+
}
85+
matchingTypes = append(matchingTypes, itype)
86+
}
87+
return matchingTypes
88+
}
89+
7790
// getMatchingInstanceTypes returns all instance types matching ic.Constraints and available
7891
// in ic.Region, sorted by increasing region-specific cost (if known).
7992
func getMatchingInstanceTypes(ic *InstanceConstraint, allInstanceTypes []InstanceType) ([]InstanceType, error) {
80-
cons := ic.Constraints
8193
region := ic.Region
8294
var itypes []InstanceType
8395

84-
// Iterate over allInstanceTypes, finding matching ones.
85-
for _, itype := range allInstanceTypes {
86-
itype, ok := itype.match(cons)
87-
if !ok {
88-
continue
89-
}
90-
itypes = append(itypes, itype)
96+
// Rules used to select instance types:
97+
// - non memory constraints like cpu-cores etc are always honoured
98+
// (previously an instance type with largest available memory was returned even if it didn't
99+
// match on other constraints like cpu-cores and this is wrong).
100+
// - if no mem constraint specified, try opinionated default with enough mem to run a server.
101+
// - if no matches, try again without any memory constraint and return the instance
102+
// with the largest memory
103+
minMemCons := ic.Constraints
104+
minMem := uint64(minMemoryHeuristic)
105+
minMemCons.Mem = &minMem
106+
cons := ic.Constraints
107+
if ic.Constraints.Mem == nil {
108+
cons = minMemCons
91109
}
110+
itypes = matchingTypesForConstraint(allInstanceTypes, cons)
92111

93112
if len(itypes) == 0 {
94-
// No matching instance types were found, so the fallback is to:
95-
// 1. Sort by memory and find the smallest matching both the required architecture
96-
// and our own heuristic: minimum amount of memory required to run a realistic server, or
97-
// 2. Sort by memory in reverse order and return the largest one, which will hopefully work,
98-
// albeit not the best match
99-
archCons := constraints.Value{Arch: ic.Constraints.Arch}
100-
for _, itype := range allInstanceTypes {
101-
itype, ok := itype.match(archCons)
102-
if !ok {
103-
continue
104-
}
105-
itypes = append(itypes, itype)
106-
}
113+
cons := ic.Constraints
114+
cons.Mem = nil
115+
itypes = matchingTypesForConstraint(allInstanceTypes, cons)
107116
sort.Sort(byMemory(itypes))
108-
var fallbackType *InstanceType
109-
// 1. check for smallest instance type that can realistically run a server
110-
for _, itype := range itypes {
111-
if itype.Mem >= minMemoryHeuristic {
112-
itcopy := itype
113-
fallbackType = &itcopy
117+
for i, itype := range itypes {
118+
if (ic.Constraints.Mem == nil && itype.Mem >= minMemoryHeuristic) || i == len(itypes)-1 {
119+
itypes = []InstanceType{itype}
114120
break
115121
}
116122
}
117-
if fallbackType == nil && len(itypes) > 0 {
118-
// 2. just get the one with the largest memory
119-
fallbackType = &itypes[len(itypes)-1]
120-
}
121-
if fallbackType != nil {
122-
itypes = []InstanceType{*fallbackType}
123-
}
124123
}
125124
// If we have matching instance types, we can return those, sorted by cost.
126125
if len(itypes) > 0 {
@@ -129,7 +128,7 @@ func getMatchingInstanceTypes(ic *InstanceConstraint, allInstanceTypes []Instanc
129128
}
130129

131130
// No luck, so report the error.
132-
return nil, fmt.Errorf("no instance types in %s matching constraints %q", region, cons)
131+
return nil, fmt.Errorf("no instance types in %s matching constraints %q", region, ic.Constraints)
133132
}
134133

135134
// tagsMatch returns if the tags in wanted all exist in have.

‎environs/instances/instancetype_test.go

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,24 +140,66 @@ var getInstanceTypesTest = []struct {
140140
arches: []string{"arm"},
141141
},
142142
{
143-
about: "fallback instance type, enough memory for mongodb",
144-
cons: "mem=8G",
143+
about: "enough memory for mongodb if mem not specified",
144+
cons: "cpu-cores=4",
145145
itypesToUse: []InstanceType{
146+
{Id: "4", Name: "it-4", Arches: []string{"amd64"}, Mem: 2048, CpuCores: 4},
147+
{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 4},
148+
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256, CpuCores: 4},
149+
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4},
150+
},
151+
expectedItypes: []string{"it-3", "it-4"},
152+
},
153+
{
154+
about: "small mem specified, use that even though less than needed for mongodb",
155+
cons: "mem=300M",
156+
itypesToUse: []InstanceType{
157+
{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 2048},
158+
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
159+
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
160+
},
161+
expectedItypes: []string{"it-1", "it-3"},
162+
},
163+
{
164+
about: "mem specified but no mem matches, choose largest memory type with enough memory for mongdb",
165+
cons: "mem=8G arch=amd64",
166+
itypesToUse: []InstanceType{
167+
{Id: "4", Name: "it-4", Arches: []string{"arm"}, Mem: 8096},
146168
{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 4096},
147169
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 2048},
148170
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
149171
},
150-
expectedItypes: []string{"it-2"},
172+
expectedItypes: []string{"it-3"},
151173
},
152174
{
153-
about: "fallback instance type, not enough memory for mongodb",
175+
about: "mem specified but no mem matches, choose largest memory type",
154176
cons: "mem=4G",
155177
itypesToUse: []InstanceType{
156178
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
157179
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
158180
},
159181
expectedItypes: []string{"it-1"},
160182
},
183+
{
184+
about: "mem specified but no mem matches, choose largest memory type matching other constraints",
185+
cons: "mem=4G arch=amd64",
186+
itypesToUse: []InstanceType{
187+
{Id: "3", Name: "it-3", Arches: []string{"arm"}, Mem: 1024},
188+
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
189+
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
190+
},
191+
expectedItypes: []string{"it-1"},
192+
},
193+
{
194+
about: "largest mem available matching other constraints if mem not specified",
195+
cons: "cpu-cores=4",
196+
itypesToUse: []InstanceType{
197+
{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 2},
198+
{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256, CpuCores: 4},
199+
{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4},
200+
},
201+
expectedItypes: []string{"it-1"},
202+
},
161203
}
162204

163205
func constraint(region, cons string) *InstanceConstraint {
@@ -195,6 +237,9 @@ func (s *instanceTypeSuite) TestGetMatchingInstanceTypesErrors(c *gc.C) {
195237

196238
_, err = getMatchingInstanceTypes(constraint("test", "arch=i386 mem=8G"), instanceTypes)
197239
c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "arch=i386 mem=8192M"`)
240+
241+
_, err = getMatchingInstanceTypes(constraint("test", "cpu-cores=9000"), instanceTypes)
242+
c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "cpu-cores=9000"`)
198243
}
199244

200245
var instanceTypeMatchTests = []struct {

0 commit comments

Comments
 (0)