Skip to content

Commit afd2ce3

Browse files
benhoytbenhoyt
committed
Code review fixes; Add zone matcher support to worker/provisioner
1 parent 319707b commit afd2ce3

File tree

7 files changed

+82
-107
lines changed

7 files changed

+82
-107
lines changed

‎provider/common/availabilityzones.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,17 +205,18 @@ func DistributeInstances(
205205
return eligible, nil
206206
}
207207

208-
// LookupAvailabilityZone looks up an availability zone by name (or path) and
209-
// returns the zone object if found, nil and an error if not.
210-
func LookupAvailabilityZone(env ZonedEnviron, ctx context.ProviderCallContext, zones []AvailabilityZone, zone string) (AvailabilityZone, error) {
208+
// SelectAvailabilityZone looks up an availability zone by name (or path) and
209+
// returns the zone object if found, nil and an error if not found or if
210+
// multiple availability zones match.
211+
func SelectAvailabilityZone(env ZonedEnviron, zones []AvailabilityZone, zone string) (AvailabilityZone, error) {
211212
var matches []AvailabilityZone
212213
for _, z := range zones {
213214
if ZoneMatches(env, z.Name(), zone) {
214215
matches = append(matches, z)
215216
}
216217
}
217218
if len(matches) == 0 {
218-
return nil, errors.NotValidf("availability zone %q", zone)
219+
return nil, errors.NotFoundf("availability zone %q", zone)
219220
}
220221
if len(matches) > 1 {
221222
names := make([]string, len(matches))
@@ -233,11 +234,11 @@ func LookupAvailabilityZone(env ZonedEnviron, ctx context.ProviderCallContext, z
233234
func ValidateAvailabilityZone(env ZonedEnviron, ctx context.ProviderCallContext, zone string) error {
234235
zones, err := env.AvailabilityZones(ctx)
235236
if err != nil {
236-
return err
237+
return errors.Trace(err)
237238
}
238-
az, err := LookupAvailabilityZone(env, ctx, zones, zone)
239+
az, err := SelectAvailabilityZone(env, zones, zone)
239240
if err != nil {
240-
return err
241+
return errors.Trace(err)
241242
}
242243
if !az.Available() {
243244
return errors.Errorf("availability zone %q is unavailable", zone)

‎provider/vsphere/client.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type DialFunc func(_ context.Context, _ *url.URL, datacenter string) (Client, er
2121
// Client is an interface for interacting with the vSphere API.
2222
type Client interface {
2323
Close(context.Context) error
24-
ComputeResources(context.Context) ([]*mo.ComputeResource, []string, error)
24+
ComputeResources(context.Context) ([]vsphereclient.ComputeResource, error)
2525
ResourcePools(context.Context, string) ([]*object.ResourcePool, error)
2626
CreateVirtualMachine(context.Context, vsphereclient.CreateVirtualMachineParams) (*mo.VirtualMachine, error)
2727
Folders(ctx context.Context) (*object.DatacenterFolders, error)

‎provider/vsphere/environ_availzones.go

Lines changed: 33 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -18,29 +18,22 @@ import (
1818

1919
type vmwareAvailZone struct {
2020
r mo.ComputeResource
21-
rPath string
2221
pool *object.ResourcePool
2322
hostFolder string
2423
}
2524

26-
// Name implements common.AvailabilityZone
25+
// Name returns the "name" of the Vsphere availability zone, which is the path
26+
// relative to the datacenter's host folder.
2727
func (z *vmwareAvailZone) Name() string {
28-
// Strip "/DataCenter1/host/" prefix from compute resource or resource pool path
29-
return strings.TrimPrefix(z.path(), z.hostFolder+"/")
28+
// Strip "/DataCenter1/host/" prefix from resource pool path
29+
return strings.TrimPrefix(z.pool.InventoryPath, z.hostFolder+"/")
3030
}
3131

3232
// Available implements common.AvailabilityZone
3333
func (z *vmwareAvailZone) Available() bool {
3434
return true
3535
}
3636

37-
func (z *vmwareAvailZone) path() string {
38-
if z.pool == nil {
39-
return z.rPath
40-
}
41-
return z.pool.InventoryPath
42-
}
43-
4437
// AvailabilityZones is part of the common.ZonedEnviron interface.
4538
func (env *environ) AvailabilityZones(ctx context.ProviderCallContext) (zones []common.AvailabilityZone, err error) {
4639
err = env.withSession(ctx, func(env *sessionEnviron) error {
@@ -65,49 +58,45 @@ func (env *sessionEnviron) AvailabilityZones(ctx context.ProviderCallContext) ([
6558
folders.HostFolder.InventoryPath, folders.HostFolder.Name())
6659
hostFolder := folders.HostFolder.InventoryPath
6760

68-
computeResources, crPaths, err := env.client.ComputeResources(env.ctx)
61+
computeResources, err := env.client.ComputeResources(env.ctx)
6962
if err != nil {
7063
HandleCredentialError(err, env, ctx)
7164
return nil, errors.Trace(err)
7265
}
7366
var zones []common.AvailabilityZone
74-
for i, cr := range computeResources {
75-
if cr.Summary.GetComputeResourceSummary().EffectiveCpu == 0 {
76-
logger.Debugf("skipping empty compute resource %q", cr.Name)
67+
for _, cr := range computeResources {
68+
if cr.Resource.Summary.GetComputeResourceSummary().EffectiveCpu == 0 {
69+
logger.Debugf("skipping empty compute resource %q", cr.Resource.Name)
7770
continue
7871
}
7972

80-
// Add an availability zone for this compute resource directly, eg:
81-
// "/DataCenter1/host/Host1"
82-
zone := &vmwareAvailZone{
83-
r: *cr,
84-
rPath: crPaths[i],
85-
hostFolder: hostFolder,
86-
}
87-
logger.Tracef("zone from compute resource: %s (cr.Name=%q rPath=%q hostFolder=%q)",
88-
zone.Name(), zone.r.Name, zone.rPath, zone.hostFolder)
89-
zones = append(zones, zone)
90-
91-
// Then add an availability zone for each resource pool under this
92-
// compute resource, eg: "/DataCenter1/host/Host1/Resources"
93-
pools, err := env.client.ResourcePools(env.ctx, crPaths[i]+"/...")
73+
// Add an availability zone for each resource pool under this compute
74+
// resource, eg: "/DataCenter1/host/Host1/Resources"
75+
pools, err := env.client.ResourcePools(env.ctx, cr.Path+"/...")
9476
if err != nil {
9577
HandleCredentialError(err, env, ctx)
9678
return nil, errors.Trace(err)
9779
}
9880
for _, pool := range pools {
99-
zone = &vmwareAvailZone{
100-
r: *cr,
101-
rPath: crPaths[i],
81+
zone := &vmwareAvailZone{
82+
r: *cr.Resource,
10283
pool: pool,
10384
hostFolder: hostFolder,
10485
}
105-
logger.Tracef("zone from resource pool: %s (cr.Name=%q rPath=%q pool.InventoryPath=%q hostFolder=%q)",
106-
zone.Name(), zone.r.Name, zone.rPath, zone.pool.InventoryPath, zone.hostFolder)
86+
logger.Tracef("zone: %s (cr.Name=%q pool.InventoryPath=%q hostFolder=%q)",
87+
zone.Name(), zone.r.Name, zone.pool.InventoryPath, zone.hostFolder)
10788
zones = append(zones, zone)
10889
}
10990
}
11091

92+
if logger.IsDebugEnabled() {
93+
zoneNames := make([]string, len(zones))
94+
for i, zone := range zones {
95+
zoneNames[i] = zone.Name()
96+
}
97+
logger.Debugf("fetched availability zones: %q", zoneNames)
98+
}
99+
111100
env.zones = zones
112101
return env.zones, nil
113102
}
@@ -145,10 +134,6 @@ func (env *sessionEnviron) InstanceAvailabilityZoneNames(ctx context.ProviderCal
145134
vm := inst.(*environInstance).base
146135
for _, zone := range zones {
147136
pool := zone.(*vmwareAvailZone).pool
148-
if pool == nil {
149-
// Skip availability zones that aren't resource pools
150-
continue
151-
}
152137
if pool.Reference().Value == vm.ResourcePool.Value {
153138
results[i] = zone.Name()
154139
break
@@ -197,47 +182,25 @@ func (env *sessionEnviron) availZone(ctx context.ProviderCallContext, name strin
197182

198183
// ZoneMatches implements zoneMatcher interface to allow custom matching (see
199184
// provider/common.ZoneMatches). For a Vsphere "availability zone" (host,
200-
// cluster, or resource pool), allow match on absolute path, path relative to
201-
// host folder, or legacy resource pool match.
185+
// cluster, or resource pool), allow a match on the path relative to the host
186+
// folder, or a legacy resource pool match.
202187
func (env *environ) ZoneMatches(zone, constraint string) bool {
203-
return zoneMatches(zone, constraint)
204-
}
205-
206-
func zoneMatches(zone, constraint string) bool {
207-
// If they've specified an absolute path, strip the datacenter/host
208-
// segments; for example "/DataCenter1/host/Cluster1/Host1"
209-
// becomes "Cluster1/Host1". This allows the user to specify an
210-
// absolute path, like those from "govc find".
211-
//
212-
// TODO benhoyt: maybe we don't want absolute path matching at all,
213-
// because if there's a datacenter folder we don't know how deep it is,
214-
// so this will fail.
215-
if strings.HasPrefix(constraint, "/") {
216-
parts := strings.Split(constraint, "/")
217-
// Must be at least ["" "DataCenter1" "host" "Cluster1"]
218-
if len(parts) < 4 {
219-
return false
220-
}
221-
constraint = strings.Join(parts[3:], "/")
222-
}
223-
224188
// Allow match on full zone name (path without host folder prefix), for
225189
// example "Cluster1/Host1".
226190
if zone == constraint {
227191
return true
228192
}
229193

230-
// Otherwise, for resource pools, allow them to omit the "Resources"
231-
// part of the path (for backwards compatibility). For example, for pool
194+
// Otherwise allow them to omit the "Resources" part of the path (for
195+
// backwards compatibility). For example, for pool
232196
// "Host1/Resources/Parent/Child", allow a match on "Host1/Parent/Child".
233-
//
234-
// TODO: should we consider stripping "Resources" at any level? That may help with folders
235197
parts := strings.Split(zone, "/")
236-
if len(parts) > 2 && parts[1] == "Resources" {
237-
legacyZone := parts[0] + "/" + strings.Join(parts[2:], "/")
238-
if legacyZone == constraint {
239-
return true
198+
var partsWithoutResources []string
199+
for _, part := range parts {
200+
if part != "Resources" {
201+
partsWithoutResources = append(partsWithoutResources, part)
240202
}
241203
}
242-
return false
204+
legacyZone := strings.Join(partsWithoutResources, "/")
205+
return legacyZone == constraint
243206
}

‎provider/vsphere/environ_broker.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,7 @@ func (env *sessionEnviron) newRawInstance(
246246
}
247247

248248
createVMArgs.ComputeResource = &availZone.r
249-
if availZone.pool != nil {
250-
createVMArgs.ResourcePool = availZone.pool.Reference()
251-
}
249+
createVMArgs.ResourcePool = availZone.pool.Reference()
252250

253251
vm, err := env.client.CreateVirtualMachine(env.ctx, createVMArgs)
254252
if vsphereclient.IsExtendDiskError(err) {

‎provider/vsphere/environ_policy.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,11 @@ package vsphere
55

66
import (
77
"github.com/juju/errors"
8-
"github.com/juju/juju/provider/common"
9-
"github.com/juju/utils/arch"
10-
118
"github.com/juju/juju/core/constraints"
129
"github.com/juju/juju/environs"
1310
"github.com/juju/juju/environs/context"
11+
"github.com/juju/juju/provider/common"
12+
"github.com/juju/utils/arch"
1413
)
1514

1615
// PrecheckInstance is part of the environs.Environ interface.
@@ -48,9 +47,9 @@ func (env *sessionEnviron) checkZones(ctx context.ProviderCallContext, zones *[]
4847
return errors.Trace(err)
4948
}
5049
for _, zone := range *zones {
51-
_, err := common.LookupAvailabilityZone(env, ctx, foundZones, zone)
50+
_, err := common.SelectAvailabilityZone(env, foundZones, zone)
5251
if err != nil {
53-
return err
52+
return errors.Trace(err)
5453
}
5554
}
5655
return nil

‎provider/vsphere/internal/vsphereclient/client.go

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ func IsExtendDiskError(err error) bool {
4444
return ok
4545
}
4646

47+
// ComputeResource stores an mo.ComputeResource along with its full path
48+
type ComputeResource struct {
49+
Resource *mo.ComputeResource
50+
Path string
51+
}
52+
4753
// Client encapsulates a vSphere client, exposing the subset of
4854
// functionality that we require in the Juju provider.
4955
type Client struct {
@@ -235,51 +241,55 @@ func (c *Client) VirtualMachines(ctx context.Context, path string) ([]*mo.Virtua
235241

236242
// ComputeResources returns a slice of all compute resources in the datacenter,
237243
// along with a slice of each compute resource's full path.
238-
func (c *Client) ComputeResources(ctx context.Context) ([]*mo.ComputeResource, []string, error) {
244+
func (c *Client) ComputeResources(ctx context.Context) ([]ComputeResource, error) {
239245
_, datacenter, err := c.finder(ctx)
240246
if err != nil {
241-
return nil, nil, errors.Trace(err)
247+
return nil, errors.Trace(err)
242248
}
243249
folders, err := datacenter.Folders(ctx)
244250
if err != nil {
245-
return nil, nil, errors.Trace(err)
251+
return nil, errors.Trace(err)
246252
}
247253

248254
return c.computeResourcesFromRef(ctx, folders.HostFolder.Reference(), folders.HostFolder.InventoryPath)
249255
}
250256

251257
// computeResourcesFromRef returns a slice of compute resources under the given
252258
// reference (folder), recursively including folders.
253-
func (c *Client) computeResourcesFromRef(ctx context.Context, ref types.ManagedObjectReference, path string) ([]*mo.ComputeResource, []string, error) {
259+
func (c *Client) computeResourcesFromRef(ctx context.Context, ref types.ManagedObjectReference, path string) ([]ComputeResource, error) {
254260
es, err := c.lister(ref).List(ctx)
255261
if err != nil {
256-
return nil, nil, errors.Trace(err)
262+
return nil, errors.Trace(err)
257263
}
258264

259-
var crs []*mo.ComputeResource
260-
var crPaths []string
265+
var crs []ComputeResource
261266
for _, e := range es {
262267
switch o := e.Object.(type) {
263268
case mo.ClusterComputeResource:
264-
crs = append(crs, &o.ComputeResource)
265-
crPaths = append(crPaths, path+"/"+o.ComputeResource.Name)
266-
c.logger.Tracef("added cluster crPath %q", crPaths[len(crPaths)-1])
269+
cr := ComputeResource{
270+
Resource: &o.ComputeResource,
271+
Path: path + "/" + o.ComputeResource.Name,
272+
}
273+
crs = append(crs, cr)
274+
c.logger.Tracef("added cluster crPath %q", cr.Path)
267275
case mo.ComputeResource:
268-
crs = append(crs, &o)
269-
crPaths = append(crPaths, path+"/"+o.Name)
270-
c.logger.Tracef("added host crPath %q", crPaths[len(crPaths)-1])
276+
cr := ComputeResource{
277+
Resource: &o,
278+
Path: path + "/" + o.Name,
279+
}
280+
crs = append(crs, cr)
281+
c.logger.Tracef("added host crPath %q", cr.Path)
271282
case mo.Folder:
272-
subCrs, subPaths, err := c.computeResourcesFromRef(ctx, o.Reference(), path+"/"+o.Name)
283+
subCrs, err := c.computeResourcesFromRef(ctx, o.Reference(), path+"/"+o.Name)
273284
if err != nil {
274-
return nil, nil, errors.Trace(err)
285+
return nil, errors.Trace(err)
275286
}
276287
crs = append(crs, subCrs...)
277-
crPaths = append(crPaths, subPaths...)
278288
c.logger.Tracef("added %d compute resources from %q",
279289
len(subCrs), path+"/"+o.Name)
280290
}
281291
}
282-
return crs, crPaths, nil
292+
return crs, nil
283293
}
284294

285295
// Folders returns the datacenter's folders object.

‎worker/provisioner/provisioner_task.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -767,12 +767,12 @@ type AvailabilityZoneMachine struct {
767767

768768
// MatchesConstraints against an AZ. If the constraints specifies Zones, make sure
769769
// this AZ matches a listed ZoneName.
770-
func (az *AvailabilityZoneMachine) MatchesConstraints(cons constraints.Value) bool {
770+
func (az *AvailabilityZoneMachine) MatchesConstraints(zonedEnv providercommon.ZonedEnviron, cons constraints.Value) bool {
771771
if !cons.HasZones() {
772772
return true
773773
}
774774
for _, zone := range *cons.Zones {
775-
if az.ZoneName == zone {
775+
if providercommon.ZoneMatches(zonedEnv, az.ZoneName, zone) {
776776
return true
777777
}
778778
}
@@ -879,9 +879,11 @@ func (task *provisionerTask) machineAvailabilityZoneDistribution(
879879
zoneMap = azMachineFilterSort(task.populateDistributionGroupZoneMap(distGroupMachineIds))
880880
}
881881

882+
zonedEnv, _ := task.broker.(providercommon.ZonedEnviron)
883+
882884
sort.Sort(zoneMap)
883885
for _, zoneMachines := range zoneMap {
884-
if !zoneMachines.MatchesConstraints(cons) {
886+
if !zoneMachines.MatchesConstraints(zonedEnv, cons) {
885887
task.logger.Debugf("machine %s does not match az %s: constraints do not match",
886888
machineId, zoneMachines.ZoneName)
887889
continue
@@ -1274,10 +1276,12 @@ func (task *provisionerTask) markMachineFailedInAZ(machine apiprovisioner.Machin
12741276
}
12751277
}
12761278

1279+
zonedEnv, _ := task.broker.(providercommon.ZonedEnviron)
1280+
12771281
// Check if there are any zones left to try (that also match constraints).
12781282
zoneMap := azMachineFilterSort(task.availabilityZoneMachines)
12791283
for _, zoneMachines := range zoneMap {
1280-
if zoneMachines.MatchesConstraints(cons) &&
1284+
if zoneMachines.MatchesConstraints(zonedEnv, cons) &&
12811285
!zoneMachines.FailedMachineIds.Contains(machine.Id()) &&
12821286
!zoneMachines.ExcludedMachineIds.Contains(machine.Id()) {
12831287
return true, nil

0 commit comments

Comments
 (0)