You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by Graeme-Miller <gi...@git.apache.org> on 2017/06/26 11:30:36 UTC
[GitHub] brooklyn-server pull request #745: Azure ARM default network fixes from PR s...
GitHub user Graeme-Miller opened a pull request:
https://github.com/apache/brooklyn-server/pull/745
Azure ARM default network fixes from PR suggestions
Important Note: Azure ARM doesn't currently work with the master of Brooklyn. Currently, after a Azure ARM VM is created, JClouds attempts to list all of the nodes in the resource group but cannot parse the result due to a missing property (vhd for DataDisk). I have tested with the latest JClouds code in master and we do not have this issue.
However, we are currently on the latest released version of JClouds (2.0.1) and we cannot pull in the code in mater until there is another JClouds release. As such, the code that has been written for default network has been written against the code in JClouds master and will work when we upgrade to the next version of JClouds. If you need working Azure ARM support, and default networking build JClouds master locally, and build brooklyn against it.
---------------------
Various fixes:
- IpOptions were being added incorrectly to templateOptions. They were being set as a map, when they should have been a list of maps
- When checking templateOptions for existing config, look for 'networks' rather than NETWORK_NAME config (which resolves to 'networkName'). Still use NETWORK_NAME config when checking in brooklyn config
- Check that a region has been set in brooklyn config (config.get(CLOUD_REGION_ID)) if not, don't try and create a default network. It was suggested that I could move the default network creation to later in JCloudsLocation after the templateOptions have been built. This would mean that a default region would have been added to the templateOptions if none was specified in config. I attempted to implement this, but ran into an issue. The AzureTemplateOptions in JClouds 2.0.1 is different to the AzureTemplateOptions in JClouds master. As such, it is not possible to amend the AzureTemplateOptions from Brooklyn code, as we would not be able to modify the ipOptions (a concept that only exists in JClouds master). As such, I have put the null check in for the time being. After the next JClouds release, I will come back and fix this
- Check for the situation where a default network exists, but a default subnet does not. In this situation create a default subnet only
------------------------------
@neykov pointed out there could be issues if we are trying to create multiple VM's at the same time due to the these VMs all trying to create the default network/subnet. I have investigated and I don't think this will be an issue. The default network creation is done in the obtainOnce method of JCloudsLocation. If there were a situation where multiple VMs tried to create the network at the same time, then all but one would fail. Brooklyn will by default will then attempt to get a VM again, and this time it will succeed. To test this theory I spooled up three AMPs, and at the same time kicked off a deployment of 5 VMs on each AMP (15 total) to try and force this situation. Only 3/15 came up correctly, but the failures were quota issues which means they got passed network creation.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Graeme-Miller/brooklyn-server fix_azure_default_network
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/745.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #745
----
commit 923f452b18b415c39cd7b4f71768953091132f3f
Author: graeme.miller <gr...@cloudsoftcorp.com>
Date: 2017-06-23T11:53:41Z
Azure ARM default network fixes from PR suggestions
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
* re azure jclouds 2.0.1 not having enough functionality - the changes here (and in previous PR) will create the default network, but will not assign it to the nodes because `ipOptions` doesn't exist in `templateOptions`. That's actually fine because 2.0.1 will create a single network for all nodes. The behaviour this PR fixes is introduced post `2.0.1`. That's why I suggest to do the changes against jclouds master and mark the PR as `WIP` until Brooklyn upgrades to a new jclouds release.
* re quota failures - not waiting for the `createOrUpdate` operation to complete will result in failures which look much like quota exceeded failures you are seeing. The API calls are async and can return before the resource is fully created. Attempts to use the resource in other operations will fail. That's why its important to:
1. block for the operation to complete
2. do an effort to `createOrUpdate` the resource only once (or as little as possible) to minimize the risk of unblocking too early while a subsequent call re-creates the resource, putting it back in the `UPDATING` state.
Depending on the region and instance type the quota is between 10-30 instances. So I'd expect at least 10 instances to come up fine, ideally all of them on regions that have the limit increased.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #745: Azure ARM default network fixes from PR s...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/745#discussion_r124042939
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreator.java ---
@@ -64,57 +68,71 @@ public static void createDefaultNetworkAndAddToTemplateOptionsIfRequired(Compute
LOG.debug("azure.arm.default.network.enabled is disabled, not creating default network");
return;
}
-
+ String location = config.get(CLOUD_REGION_ID);
+ if(StringUtils.isEmpty(location)) {
+ LOG.debug("No region information, so cannot create a default network");
+ return;
+ }
Map<String, Object> templateOptions = config.get(TEMPLATE_OPTIONS);
+
//Only create a default network if we haven't specified a network name (in template options or config) or ip options
if (config.containsKey(NETWORK_NAME)) {
- LOG.debug("Network config specified when provisioning Azure machine. Not creating default network");
+ LOG.debug("Network config [{}] specified when provisioning Azure machine. Not creating default network", NETWORK_NAME.getName());
return;
}
- if (templateOptions != null && (templateOptions.containsKey(NETWORK_NAME.getName()) || templateOptions.containsKey("ipOptions"))) {
+ if (templateOptions != null && (templateOptions.containsKey("networks") || templateOptions.containsKey("ipOptions"))) {
LOG.debug("Network config specified when provisioning Azure machine. Not creating default network");
return;
}
AzureComputeApi api = computeService.getContext().unwrapApi(AzureComputeApi.class);
- String location = config.get(CLOUD_REGION_ID);
String resourceGroupName = DEFAULT_RESOURCE_GROUP_PREFIX + "-" + location;
String vnetName = DEFAULT_NETWORK_NAME_PREFIX + "-" + location;
String subnetName = DEFAULT_SUBNET_NAME_PREFIX + "-" + location;
+ SubnetApi subnetApi = api.getSubnetApi(resourceGroupName, vnetName);
+ VirtualNetworkApi virtualNetworkApi = api.getVirtualNetworkApi(resourceGroupName);
+
//Check if default already exists
- Subnet preexistingSubnet = api.getSubnetApi(resourceGroupName, vnetName).get(subnetName);
+ Subnet preexistingSubnet = subnetApi.get(subnetName);
if(preexistingSubnet != null){
LOG.info("Using pre-existing default Azure network [{}] and subnet [{}] when provisioning machine",
vnetName, subnetName);
updateTemplateOptions(config, preexistingSubnet);
return;
}
+ createResourceGroupIfNeeded(api, resourceGroupName, location);
- LOG.info("Network config not specified when provisioning Azure machine, and default network/subnet does not exists. "
- + "Creating network [{}] and subnet [{}], and updating template options", vnetName, subnetName);
+ Subnet.SubnetProperties subnetProperties = Subnet.SubnetProperties.builder().addressPrefix(DEFAULT_SUBNET_ADDRESS_PREFIX).build();
- createResourceGroupIfNeeded(api, resourceGroupName, location);
+ //Creating network + subnet if network doesn't exist
+ if(virtualNetworkApi.get(vnetName) == null) {
+ LOG.info("Network config not specified when provisioning Azure machine, and default network/subnet does not exists. "
+ + "Creating network [{}] and subnet [{}], and updating template options", vnetName, subnetName);
- //Setup properties for creating subnet/network
- Subnet subnet = Subnet.create(subnetName, null, null,
- Subnet.SubnetProperties.builder().addressPrefix(DEFAULT_SUBNET_ADDRESS_PREFIX).build());
+ Subnet subnet = Subnet.create(subnetName, null, null, subnetProperties);
- VirtualNetwork.VirtualNetworkProperties virtualNetworkProperties = VirtualNetwork.VirtualNetworkProperties
- .builder().addressSpace(VirtualNetwork.AddressSpace.create(Arrays.asList(DEFAULT_VNET_ADDRESS_PREFIX)))
- .subnets(Arrays.asList(subnet)).build();
+ VirtualNetwork.VirtualNetworkProperties virtualNetworkProperties = VirtualNetwork.VirtualNetworkProperties
+ .builder().addressSpace(VirtualNetwork.AddressSpace.create(Arrays.asList(DEFAULT_VNET_ADDRESS_PREFIX)))
+ .subnets(Arrays.asList(subnet)).build();
+ virtualNetworkApi.createOrUpdate(vnetName, location, virtualNetworkProperties);
+ } else {
+ LOG.info("Network config not specified when provisioning Azure machine, and default subnet does not exists. "
+ + "Creating subnet [{}] on network [{}], and updating template options", subnetName, vnetName);
- //Create network
- api.getVirtualNetworkApi(resourceGroupName).createOrUpdate(vnetName, location, virtualNetworkProperties);
+ //Just creating the subnet
+ subnetApi.createOrUpdate(subnetName, subnetProperties);
--- End diff --
Need to wait for the `createOrUpdate` operation to complete (here and above). The call is async. Can be done by polling the `properties.provisioningState()` returned by the `get` call on the resource.
If not done, any operations using the resource will fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
Thanks @Graeme-Miller, merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
Tested with a location with no virtual network set but with overrideAuthenticateSudo set to true.
Tested with a specified virtual network.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
Fine by me to merge the PR as it already improves on the existing functionality. Can you create another PR which targets post 2.0.1 jclouds, handling the default region case and mark it `WIP` so we can merge it once we have a new jclouds release.
Note that because of my second point in above comment this will actually break deployments for anyone relying on the functionality. Happy to pair to discuss how it can be improved on.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #745: Azure ARM default network fixes from PR s...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/745#discussion_r124042243
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreator.java ---
@@ -64,57 +68,71 @@ public static void createDefaultNetworkAndAddToTemplateOptionsIfRequired(Compute
LOG.debug("azure.arm.default.network.enabled is disabled, not creating default network");
return;
}
-
+ String location = config.get(CLOUD_REGION_ID);
+ if(StringUtils.isEmpty(location)) {
+ LOG.debug("No region information, so cannot create a default network");
+ return;
+ }
Map<String, Object> templateOptions = config.get(TEMPLATE_OPTIONS);
+
//Only create a default network if we haven't specified a network name (in template options or config) or ip options
if (config.containsKey(NETWORK_NAME)) {
- LOG.debug("Network config specified when provisioning Azure machine. Not creating default network");
+ LOG.debug("Network config [{}] specified when provisioning Azure machine. Not creating default network", NETWORK_NAME.getName());
return;
}
- if (templateOptions != null && (templateOptions.containsKey(NETWORK_NAME.getName()) || templateOptions.containsKey("ipOptions"))) {
+ if (templateOptions != null && (templateOptions.containsKey("networks") || templateOptions.containsKey("ipOptions"))) {
LOG.debug("Network config specified when provisioning Azure machine. Not creating default network");
return;
}
AzureComputeApi api = computeService.getContext().unwrapApi(AzureComputeApi.class);
- String location = config.get(CLOUD_REGION_ID);
String resourceGroupName = DEFAULT_RESOURCE_GROUP_PREFIX + "-" + location;
String vnetName = DEFAULT_NETWORK_NAME_PREFIX + "-" + location;
String subnetName = DEFAULT_SUBNET_NAME_PREFIX + "-" + location;
+ SubnetApi subnetApi = api.getSubnetApi(resourceGroupName, vnetName);
+ VirtualNetworkApi virtualNetworkApi = api.getVirtualNetworkApi(resourceGroupName);
+
//Check if default already exists
- Subnet preexistingSubnet = api.getSubnetApi(resourceGroupName, vnetName).get(subnetName);
+ Subnet preexistingSubnet = subnetApi.get(subnetName);
if(preexistingSubnet != null){
LOG.info("Using pre-existing default Azure network [{}] and subnet [{}] when provisioning machine",
vnetName, subnetName);
updateTemplateOptions(config, preexistingSubnet);
return;
}
+ createResourceGroupIfNeeded(api, resourceGroupName, location);
- LOG.info("Network config not specified when provisioning Azure machine, and default network/subnet does not exists. "
- + "Creating network [{}] and subnet [{}], and updating template options", vnetName, subnetName);
+ Subnet.SubnetProperties subnetProperties = Subnet.SubnetProperties.builder().addressPrefix(DEFAULT_SUBNET_ADDRESS_PREFIX).build();
- createResourceGroupIfNeeded(api, resourceGroupName, location);
+ //Creating network + subnet if network doesn't exist
+ if(virtualNetworkApi.get(vnetName) == null) {
+ LOG.info("Network config not specified when provisioning Azure machine, and default network/subnet does not exists. "
+ + "Creating network [{}] and subnet [{}], and updating template options", vnetName, subnetName);
- //Setup properties for creating subnet/network
- Subnet subnet = Subnet.create(subnetName, null, null,
- Subnet.SubnetProperties.builder().addressPrefix(DEFAULT_SUBNET_ADDRESS_PREFIX).build());
+ Subnet subnet = Subnet.create(subnetName, null, null, subnetProperties);
- VirtualNetwork.VirtualNetworkProperties virtualNetworkProperties = VirtualNetwork.VirtualNetworkProperties
- .builder().addressSpace(VirtualNetwork.AddressSpace.create(Arrays.asList(DEFAULT_VNET_ADDRESS_PREFIX)))
- .subnets(Arrays.asList(subnet)).build();
+ VirtualNetwork.VirtualNetworkProperties virtualNetworkProperties = VirtualNetwork.VirtualNetworkProperties
+ .builder().addressSpace(VirtualNetwork.AddressSpace.create(Arrays.asList(DEFAULT_VNET_ADDRESS_PREFIX)))
+ .subnets(Arrays.asList(subnet)).build();
+ virtualNetworkApi.createOrUpdate(vnetName, location, virtualNetworkProperties);
+ } else {
+ LOG.info("Network config not specified when provisioning Azure machine, and default subnet does not exists. "
+ + "Creating subnet [{}] on network [{}], and updating template options", subnetName, vnetName);
- //Create network
- api.getVirtualNetworkApi(resourceGroupName).createOrUpdate(vnetName, location, virtualNetworkProperties);
+ //Just creating the subnet
+ subnetApi.createOrUpdate(subnetName, subnetProperties);
+ }
+
+ //Get created subnet
Subnet createdSubnet = api.getSubnetApi(resourceGroupName, vnetName).get(subnetName);
--- End diff --
You already have `subnetApi` available here, could reuse it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
@neykov I have rebased
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
@Graeme-Miller Brooklyn has upgraded to jclouds 2.0.2, which opens the door to supporting default region as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
@neykov my test failed again, but with a better rate. 8/15 came up.
This doesn't surprise me as the failures are all for exceeding the cpu quota limit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
@neykov that's a good point about the default networking functionality being different in 2.0.1. I'd still recommend merging this PR as it is useful for downstream projects that use the latest JClouds.
Not sure I 100% follow your comment about quota failures. However, if it is acceptable I will research and fix in a separate PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #745: Azure ARM default network fixes from PR s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/745
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #745: Azure ARM default network fixes from PR suggesti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/745
LGTM with the "waiting to complete" changes.
@Graeme-Miller have you retried your test, has the failure rate improved? If no failure this is good to merge. Otherwise we need to look for further improvements to prevent deployment errors.
Prefer rebasing of PR instead merging from master, especially for small ones, but not a blocker for merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---