Initial support to wire up spaces support for Azure #11983
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wallyworld
commented
Sep 10, 2020
| @@ -713,54 +720,20 @@ func (env *azureEnviron) createVirtualMachine( | |||
| vmDependsOn = append(vmDependsOn, availabilitySetId) | |||
| } | |||
|
|
|||
| // Controller and non-controller machines are assigned to separate | |||
There was a problem hiding this comment.
moved to networkInfoForInstance
wallyworld
commented
Sep 10, 2020
Comment on lines
+181
to
+224
| // No space constraints so use the default network and subnet. | ||
| if !constraints.HasSpaces() { | ||
| return vnetID, []string{subnetID}, nil | ||
| } | ||
|
|
||
| // Attempt to filter the subnet IDs for the configured availability zone. | ||
| // If there is no configured zone, consider all subnet IDs. | ||
| az := availabilityZone | ||
| subnetIDsForZone := make([][]network.Id, len(subnetsToZones)) | ||
| for i, nic := range subnetsToZones { | ||
| var subnetIDs []network.Id | ||
| if az != "" { | ||
| var err error | ||
| if subnetIDs, err = network.FindSubnetIDsForAvailabilityZone(az, nic); err != nil { | ||
| return "", nil, errors.Annotatef(err, "getting subnets in zone %q", az) | ||
| } | ||
| if len(subnetIDs) == 0 { | ||
| return "", nil, errors.Errorf("availability zone %q has no subnets satisfying space constraints", az) | ||
| } | ||
| } else { | ||
| for subnetID := range nic { | ||
| subnetIDs = append(subnetIDs, subnetID) | ||
| } | ||
| } | ||
|
|
||
| // Filter out any fan networks. | ||
| subnetIDsForZone[i] = network.FilterInFanNetwork(subnetIDs) | ||
| } | ||
| logger.Debugf("found subnet ids for zone: %#v", subnetIDsForZone) | ||
|
|
||
| /// For each list of subnet IDs that satisfy space and zone constraints, | ||
| // choose a single one at random. | ||
| subnetIDForZone := make([]network.Id, len(subnetIDsForZone)) | ||
| for i, subnetIDs := range subnetIDsForZone { | ||
| if len(subnetIDs) == 1 { | ||
| subnetIDForZone[i] = subnetIDs[0] | ||
| continue | ||
| } | ||
|
|
||
| subnetIDForZone[i] = subnetIDs[rand.Intn(len(subnetIDs))] | ||
| } | ||
| if len(subnetIDForZone) == 0 { | ||
| return "", nil, errors.NotFoundf("subnet for constraint %q", constraints.String()) | ||
| } |
There was a problem hiding this comment.
Largely copied from openstack provider
ycliuhw
approved these changes
Sep 10, 2020
|
|
manadart
reviewed
Sep 21, 2020
| @@ -295,7 +295,7 @@ func FindSubnetIDsForAvailabilityZone(zoneName string, subnetsToZones map[Id][]s | |||
| matchingSubnetIDs := set.NewStrings() | |||
| for subnetID, zones := range subnetsToZones { | |||
| zonesSet := set.NewStrings(zones...) | |||
| if zonesSet.Contains(zoneName) { | |||
| if zonesSet.Size() == 0 && zoneName == "" || zonesSet.Contains(zoneName) { | |||
There was a problem hiding this comment.
I am concerned with the special casing we have introduced here, particularly given that we do not call this with an empty zone name (see subnetsForZone in the Azure provider).
manadart
reviewed
Sep 21, 2020
| @@ -486,8 +487,21 @@ func (api *ProvisionerAPI) subnetsAndZonesForSpace(machineID string, spaceName s | |||
|
|
|||
| zones := subnet.AvailabilityZones() | |||
| if len(zones) == 0 { | |||
There was a problem hiding this comment.
Here too. Might it not be better to indicate whether AZs confine subnet availability via a method on NetworkingEnviron, which would allow us to design out this special casing?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Initial spaces support is added to the Azure provider. The main changes are:
Drive by fix for the firewaller worker and handling not provisioned errors.
TODO: model network security group for custom networks
QA steps
create a resource group (test-rg) and make a virtual network (test-vn) with some subnets (cidr1, cidr2)
juju bootstrap azure --no-default-model
juju add-model --config network="test-rg/test-vn"
Add some spaces to the model
juju add-space foo cidr1
juju add-space bar cidr2
Add a machine with space constraints
juju add-machine --constraints="spaces=foo,bar"
Use the Azure portal to check that machine is deployed with 2 network interfaces, one for each subnet
Bug reference
https://bugs.launchpad.net/juju/+bug/1891650
https://bugs.launchpad.net/juju/+bug/1892490