You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Martin Harris <no...@github.com> on 2018/09/12 12:09:22 UTC
[jclouds/jclouds] Fix/azure resource removal (#1240)
Fixes an issue where Azure ARM resource groups are not deleted when a node is destroyed. This fix resolves the following Jiras
https://issues.apache.org/jira/browse/JCLOUDS-1330
https://issues.apache.org/jira/browse/JCLOUDS-1331
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1240
-- Commit Summary --
* Adds CleanupResourcesTest
* Waits for Azure resources to be removed from resource group before attempting to delete group
-- File Changes --
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzurePredicatesModule.java (35)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java (105)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CreateResourcesThenCreateNodes.java (4)
M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/config/AzureComputeProperties.java (2)
M providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceLiveTest.java (47)
A providers/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResourcesTest.java (314)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1240.patch
https://github.com/jclouds/jclouds/pull/1240.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> @@ -110,6 +122,27 @@ protected VirtualMachineInStatePredicateFactory provideNodeSuspendedPredicate(fi
timeouts.nodeTerminated, pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
}
+ @Provides
+ @Named(TIMEOUT_RESOURCE_REMOVED)
+ protected Predicate<IdReference> provideResourceRemovedPredicate(final AzureComputeApi api, final ComputeServiceConstants.Timeouts timeouts,
+ final PollPeriod pollPeriod) {
+ long timeout = timeouts.nodeTerminated;
+ return retry(new Predicate<IdReference>() {
+ @Override
+ public boolean apply(final IdReference input) {
+ List<org.jclouds.azurecompute.arm.domain.Resource> attachedResources = api.getResourceGroupApi().resources(input.resourceGroup());
+ Optional<org.jclouds.azurecompute.arm.domain.Resource> resourceInGroup = Iterables.tryFind(attachedResources, new Predicate<org.jclouds.azurecompute.arm.domain.Resource>() {
+ @Override
+ public boolean apply(org.jclouds.azurecompute.arm.domain.Resource resource) {
+ return resource.id().equalsIgnoreCase(input.id());
Curious about this - in which cases do we expect IDs to have different cases? Is there some documentation we could like to in a comment here to explain why we need a case-insensitive comparison here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#pullrequestreview-156628024
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -171,9 +216,12 @@ public boolean cleanupSecurityGroupIfOrphaned(String resourceGroup, String group
logger.debug(">> deleting orphaned security group %s from %s...", name, resourceGroup);
try {
deleted = resourceDeleted.apply(sgapi.delete(name));
+ resourceRemoved.apply(IdReference.create(securityGroup.id()));
Then, please add a comment in places where we are using this, to make explicit this is done to deal with eventual consistency issues. Otherwise, we might be tempted to accidentally change this in the future.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r235879058
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> }
}
return deleted;
}
+ private void cleanupVirtualNetworks(String resourceGroupName) {
+ for (VirtualNetwork virtualNetwork : api.getVirtualNetworkApi(resourceGroupName).list()) {
+ if (virtualNetwork.tags() != null && virtualNetwork.tags().containsKey("jclouds")) {
+ try {
+ // Virtual networks cannot be deleted if there are any resources attached. It does not seem to be possible
+ // to list devices connected to a virtual network
+ // https://docs.microsoft.com/en-us/azure/virtual-network/manage-virtual-network#delete-a-virtual-network
+ // We also check the tags to ensure that it's jclouds-created
+ api.getVirtualNetworkApi(resourceGroupName).delete(virtualNetwork.name());
+ resourceRemoved.apply(IdReference.create(virtualNetwork.id()));
+ } catch (IllegalArgumentException e) {
+ if (e.getMessage().contains("InUseSubnetCannotBeDeleted")) {
+ // Can be ignored as virtualNetwork is in use
Curious about this comment - does it mean we _can_ or we _have to_ try not to delete it? Given that the intent here is to delete, as Ignasi points out, should the log message be a `warn` instead?
If so, we can probably remove the comment, as the log message says pretty much the same thing
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#pullrequestreview-156628711
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> @@ -121,7 +131,17 @@ public boolean cleanupVirtualMachineNICs(VirtualMachine virtualMachine) {
PublicIPAddress ip = api.getPublicIPAddressApi(publicIpResourceGroup).get(publicIpName);
if (ip.tags() != null && Boolean.parseBoolean(ip.tags().get(AUTOGENERATED_IP_KEY))) {
logger.debug(">> deleting public ip %s...", publicIpName);
- deleted &= api.getPublicIPAddressApi(publicIpResourceGroup).delete(publicIpName);
+ try {
+ boolean ipDeleted = api.getPublicIPAddressApi(publicIpResourceGroup).delete(publicIpName);
+ if (!ipDeleted) {
+ logger.warn(">> ip not deleted %s...", ip);
+ }
+ deleted &= ipDeleted;
+ } catch (Exception ex) {
+ logger.warn(">> Error deleting ip %s", ip);
Also: log the exception?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r218643836
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Martin Harris <no...@github.com>.
Hi, thanks for the feedback. I've addressed a couple of the comments above, and pushed a couple of changes to address the others
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#issuecomment-440660829
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> @@ -110,6 +122,27 @@ protected VirtualMachineInStatePredicateFactory provideNodeSuspendedPredicate(fi
timeouts.nodeTerminated, pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
}
+ @Provides
+ @Named(TIMEOUT_RESOURCE_REMOVED)
+ protected Predicate<IdReference> provideResourceRemovedPredicate(final AzureComputeApi api, final ComputeServiceConstants.Timeouts timeouts,
+ final PollPeriod pollPeriod) {
+ long timeout = timeouts.nodeTerminated;
+ return retry(new Predicate<IdReference>() {
+ @Override
+ public boolean apply(final IdReference input) {
+ List<org.jclouds.azurecompute.arm.domain.Resource> attachedResources = api.getResourceGroupApi().resources(input.resourceGroup());
+ Optional<org.jclouds.azurecompute.arm.domain.Resource> resourceInGroup = Iterables.tryFind(attachedResources, new Predicate<org.jclouds.azurecompute.arm.domain.Resource>() {
+ @Override
+ public boolean apply(org.jclouds.azurecompute.arm.domain.Resource resource) {
+ return resource.id().equalsIgnoreCase(input.id());
> So in short, yes, we should always use equalsIgnoreCase for safety
Thanks for explaining, @danielestevez! In that case, would it make sense to add a test for that as part of this PR?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r220679968
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Gaul <no...@github.com>.
Please reopen against apache/jclouds if this is still relevant.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#issuecomment-663855829
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -171,9 +216,12 @@ public boolean cleanupSecurityGroupIfOrphaned(String resourceGroup, String group
logger.debug(">> deleting orphaned security group %s from %s...", name, resourceGroup);
try {
deleted = resourceDeleted.apply(sgapi.delete(name));
+ resourceRemoved.apply(IdReference.create(securityGroup.id()));
Sadly I can confirm this is the case. Even Azure Portal seems to have this type of eventual inconsistencies causing random failures
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r235555027
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
> Thanks, @nakomis! And many thanks for taking care of adding new tests. It's highly appreciated!
+1 to that - many thanks!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#issuecomment-422614385
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> logger.debug(">> the resource group %s is empty. Deleting...", group);
- deleted = resourceDeleted.apply(api.getResourceGroupApi().delete(group));
+ URI uri = api.getResourceGroupApi().delete(group);
+ deleted = uri == null || resourceDeleted.apply(uri);
+ } else {
+ logger.debug(">> the resource group %s contains %s. Not deleting...", group, attachedResources);
Similar comment as above: should this be `logger.warn`?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#pullrequestreview-156628784
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> +import org.jclouds.azurecompute.arm.features.DiskApi;
+import org.jclouds.azurecompute.arm.features.NetworkInterfaceCardApi;
+import org.jclouds.azurecompute.arm.features.NetworkSecurityGroupApi;
+import org.jclouds.azurecompute.arm.features.PublicIPAddressApi;
+import org.jclouds.azurecompute.arm.features.ResourceGroupApi;
+import org.jclouds.compute.functions.GroupNamingConvention;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+
+@Test(groups = "unit", testName = "CleanupResourcesTest")
+public class CleanupResourcesTest {
If the case-insensitive resource ID comparison logic is important, do we need a test for that?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#pullrequestreview-156629019
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> @@ -110,6 +122,27 @@ protected VirtualMachineInStatePredicateFactory provideNodeSuspendedPredicate(fi
timeouts.nodeTerminated, pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
}
+ @Provides
+ @Named(TIMEOUT_RESOURCE_REMOVED)
+ protected Predicate<IdReference> provideResourceRemovedPredicate(final AzureComputeApi api, final ComputeServiceConstants.Timeouts timeouts,
+ final PollPeriod pollPeriod) {
+ long timeout = timeouts.nodeTerminated;
+ return retry(new Predicate<IdReference>() {
+ @Override
+ public boolean apply(final IdReference input) {
+ List<org.jclouds.azurecompute.arm.domain.Resource> attachedResources = api.getResourceGroupApi().resources(input.resourceGroup());
+ Optional<org.jclouds.azurecompute.arm.domain.Resource> resourceInGroup = Iterables.tryFind(attachedResources, new Predicate<org.jclouds.azurecompute.arm.domain.Resource>() {
+ @Override
+ public boolean apply(org.jclouds.azurecompute.arm.domain.Resource resource) {
+ return resource.id().equalsIgnoreCase(input.id());
We should expect different cases.... everywhere :( Azure ARM is case insensitive and our provider should adapt to this.
There are currently lots of places where this could fail since we use equals or Map keys.
I had this trouble here: https://stackoverflow.com/questions/50068324/azure-arm-api-returns-locations-with-inconsistent-case/50977265#50977265 and it seems to happen to others too https://stackoverflow.com/questions/48561304/resource-group-name-case-insensitive-in-disk-ids
So in short, yes, we should always use equalsIgnoreCase for safety
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r220581590
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Martin Harris <no...@github.com>.
nakomis commented on this pull request.
> @@ -171,9 +216,12 @@ public boolean cleanupSecurityGroupIfOrphaned(String resourceGroup, String group
logger.debug(">> deleting orphaned security group %s from %s...", name, resourceGroup);
try {
deleted = resourceDeleted.apply(sgapi.delete(name));
+ resourceRemoved.apply(IdReference.create(securityGroup.id()));
Unfortunately, yes, we do need to wait. The issue that this PR is addressing is that even through a resource is deleted and (in this case) the NetworkSecuityGroup API is showing that group has been deleted, it is still showing up in the ResourceGroup. This appears to be an "eventual consistency" issue, so we need to wait until the resource no longer shows as a member of the resource group
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r235384528
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Gaul <no...@github.com>.
@nakomis does this pull request need anything to move forward?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#issuecomment-458261013
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Andrew Gaul <no...@github.com>.
Closed #1240.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#event-3586274056
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks, @nakomis! And many thanks for taking care of adding new tests. It's highly appreciated!
> @@ -98,7 +109,8 @@ protected VirtualMachineInStatePredicateFactory provideVirtualMachineRunningPred
@Named(TIMEOUT_RESOURCE_DELETED)
protected Predicate<URI> provideResourceDeletedPredicate(final AzureComputeApi api,
final ComputeServiceConstants.Timeouts timeouts, final PollPeriod pollPeriod) {
- return retry(new ActionDonePredicate(api), timeouts.nodeTerminated, pollPeriod.pollInitialPeriod,
+ long timeout = timeouts.nodeTerminated;
Undo this tiny change.
> @@ -121,7 +131,17 @@ public boolean cleanupVirtualMachineNICs(VirtualMachine virtualMachine) {
PublicIPAddress ip = api.getPublicIPAddressApi(publicIpResourceGroup).get(publicIpName);
if (ip.tags() != null && Boolean.parseBoolean(ip.tags().get(AUTOGENERATED_IP_KEY))) {
logger.debug(">> deleting public ip %s...", publicIpName);
- deleted &= api.getPublicIPAddressApi(publicIpResourceGroup).delete(publicIpName);
+ try {
+ boolean ipDeleted = api.getPublicIPAddressApi(publicIpResourceGroup).delete(publicIpName);
+ if (!ipDeleted) {
+ logger.warn(">> ip not deleted %s...", ip);
+ }
+ deleted &= ipDeleted;
+ } catch (Exception ex) {
+ logger.warn(">> Error deleting ip %s", ip);
Mark `deleted = false` too?
> @@ -110,6 +122,27 @@ protected VirtualMachineInStatePredicateFactory provideNodeSuspendedPredicate(fi
timeouts.nodeTerminated, pollPeriod.pollInitialPeriod, pollPeriod.pollMaxPeriod);
}
+ @Provides
+ @Named(TIMEOUT_RESOURCE_REMOVED)
+ protected Predicate<IdReference> provideResourceRemovedPredicate(final AzureComputeApi api, final ComputeServiceConstants.Timeouts timeouts,
The name of this method is quite confusing. I'd suggest renaming to `InResourceGroup` or similar (even if you have to change its semantics and return presence instead of absence).
> + // Wait for 404 on the disk api
+ Predicate<IdReference> diskDeleted = new Predicate<IdReference>() {
+ @Override
+ public boolean apply(IdReference input) {
+ return api.getDiskApi(input.resourceGroup()).get(input.name()) == null;
+ }
+ };
+
+ Predicate<IdReference> filter = retry(diskDeleted, 1200, 5, 15, SECONDS);
+ Set<IdReference> nonDeleted = Sets.filter(deleteDisks, filter);
+
+ if (!nonDeleted.isEmpty()) {
+ logger.warn(">> disks not deleted: %s", Joiner.on(',').join(nonDeleted));
+ }
+
+ Set<IdReference> disksNotInRg = Sets.filter(deleteDisks, resourceRemoved);
This predicate can be expensive and is just used to print a log. Put these lines inside a `log.IsWarnEnabed` guard to save these calls if logging is disabled.
> @@ -171,9 +216,12 @@ public boolean cleanupSecurityGroupIfOrphaned(String resourceGroup, String group
logger.debug(">> deleting orphaned security group %s from %s...", name, resourceGroup);
try {
deleted = resourceDeleted.apply(sgapi.delete(name));
+ resourceRemoved.apply(IdReference.create(securityGroup.id()));
Do we really need to wait for this? Getting all resources in an RG looks like a expensive call? Is there any better call we could make instead?
> }
}
return deleted;
}
+ private void cleanupVirtualNetworks(String resourceGroupName) {
+ for (VirtualNetwork virtualNetwork : api.getVirtualNetworkApi(resourceGroupName).list()) {
+ if (virtualNetwork.tags() != null && virtualNetwork.tags().containsKey("jclouds")) {
+ try {
+ // Virtual networks cannot be deleted if there are any resources attached. It does not seem to be possible
+ // to list devices connected to a virtual network
+ // https://docs.microsoft.com/en-us/azure/virtual-network/manage-virtual-network#delete-a-virtual-network
+ // We also check the tags to ensure that it's jclouds-created
+ api.getVirtualNetworkApi(resourceGroupName).delete(virtualNetwork.name());
+ resourceRemoved.apply(IdReference.create(virtualNetwork.id()));
Same here. Better use the `get` virtual network call instead of the resource group one.
> }
}
return deleted;
}
+ private void cleanupVirtualNetworks(String resourceGroupName) {
+ for (VirtualNetwork virtualNetwork : api.getVirtualNetworkApi(resourceGroupName).list()) {
+ if (virtualNetwork.tags() != null && virtualNetwork.tags().containsKey("jclouds")) {
+ try {
+ // Virtual networks cannot be deleted if there are any resources attached. It does not seem to be possible
+ // to list devices connected to a virtual network
+ // https://docs.microsoft.com/en-us/azure/virtual-network/manage-virtual-network#delete-a-virtual-network
+ // We also check the tags to ensure that it's jclouds-created
+ api.getVirtualNetworkApi(resourceGroupName).delete(virtualNetwork.name());
+ resourceRemoved.apply(IdReference.create(virtualNetwork.id()));
+ } catch (IllegalArgumentException e) {
+ if (e.getMessage().contains("InUseSubnetCannotBeDeleted")) {
+ // Can be ignored as virtualNetwork is in use
+ logger.debug("Cannot delete virtual network %s as it is in use", virtualNetwork);
Better change to `warn`.
> @@ -231,7 +309,10 @@ private static boolean isOrphanedJcloudsAvailabilitySet(AvailabilitySet availabi
}
private boolean deleteVirtualMachine(String group, VirtualMachine virtualMachine) {
- return resourceDeleted.apply(api.getVirtualMachineApi(group).delete(virtualMachine.name()));
+ URI uri = api.getVirtualMachineApi(group).delete(virtualMachine.name());
+ boolean deleted = uri == null || resourceDeleted.apply(uri);
+ resourceRemoved.apply(IdReference.create(virtualMachine.id()));
Same here. Prefer the VM api get method.
> @@ -65,6 +72,20 @@ public void initializeContext() {
super.initializeContext();
resourceDeleted = context.utils().injector().getInstance(Key.get(new TypeLiteral<Predicate<URI>>() {
}, Names.named(TIMEOUT_RESOURCE_DELETED)));
+ resourceRemoved = context.utils().injector().getInstance(Key.get(new TypeLiteral<Predicate<IdReference>>() {
+ }, Names.named(TIMEOUT_RESOURCE_REMOVED)));
+ }
+
+ @Test(groups = "live", enabled = true)
+ public void testResourceGroupDeletedOnDestroy() throws Exception {
+ template = buildTemplate(templateBuilder());
+ nodes = newTreeSet(client.createNodesInGroup(group, 1, template));
+ NodeMetadata node = nodes.first();
nit: `NodeMetadata node = Iterables.getOnlyElement(client.createNodesInGroup(group, 1, template));`
> @@ -65,6 +72,20 @@ public void initializeContext() {
super.initializeContext();
resourceDeleted = context.utils().injector().getInstance(Key.get(new TypeLiteral<Predicate<URI>>() {
}, Names.named(TIMEOUT_RESOURCE_DELETED)));
+ resourceRemoved = context.utils().injector().getInstance(Key.get(new TypeLiteral<Predicate<IdReference>>() {
+ }, Names.named(TIMEOUT_RESOURCE_REMOVED)));
+ }
+
+ @Test(groups = "live", enabled = true)
+ public void testResourceGroupDeletedOnDestroy() throws Exception {
+ template = buildTemplate(templateBuilder());
+ nodes = newTreeSet(client.createNodesInGroup(group, 1, template));
+ NodeMetadata node = nodes.first();
+ client.destroyNode(node.getId());
+ List<Resource> resources = view.unwrapApi(AzureComputeApi.class).getResourceGroupApi().resources(resourceGroupName);
+ if (!resources.isEmpty()) {
+ Assert.fail("Resource group was not empty, contained " + resources);
nit: static import
> +
+ expect(diskApi.delete("myOsDisk")).andReturn(URI.create("http://myDeletionUri"));
+ expect(diskApi.delete("myDataDisk")).andReturn(URI.create("http://myDeletionUri"));
+ // Simulate a delay in disk deletion
+ expect(diskApi.get("myDataDisk")).andReturn(disk).times(3);
+ expect(diskApi.get("myDataDisk")).andReturn(null).times(2); // An additional call is made when filtering the set
+ expect(diskApi.get("myOsDisk")).andReturn(null);
+
+ replay(api, diskApi);
+
+ // Do call
+ cleanupResources(api).cleanupManagedDisks(vm);
+
+ // Verify deleted resource group
+ verify(api, diskApi);
+ }
Now that you're adding this (thanks!) add a method to verify that the virtual networks are deleted too.
Also add a test to verify that the resources that _don't_ have the jclouds tags (availability sets, networks, etc), are not deleted.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#pullrequestreview-156433617
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Ignasi Barrera <no...@github.com>.
> Hi, thanks for the feedback. I've addressed a couple of the comments above, and pushed a couple of changes to address the others
Looks like the changes are note pushed?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#issuecomment-441188820
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Martin Harris <no...@github.com>.
nakomis commented on this pull request.
> + // Wait for 404 on the disk api
+ Predicate<IdReference> diskDeleted = new Predicate<IdReference>() {
+ @Override
+ public boolean apply(IdReference input) {
+ return api.getDiskApi(input.resourceGroup()).get(input.name()) == null;
+ }
+ };
+
+ Predicate<IdReference> filter = retry(diskDeleted, 1200, 5, 15, SECONDS);
+ Set<IdReference> nonDeleted = Sets.filter(deleteDisks, filter);
+
+ if (!nonDeleted.isEmpty()) {
+ logger.warn(">> disks not deleted: %s", Joiner.on(',').join(nonDeleted));
+ }
+
+ Set<IdReference> disksNotInRg = Sets.filter(deleteDisks, resourceRemoved);
Hi @nacx The call to the predicate causes the method to block until the resource has been removed from the resource group (in addition to being deleted), so it is always necessary to call it
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r235383608
Re: [jclouds/jclouds] Fix/azure resource removal (#1240)
Posted by Martin Harris <no...@github.com>.
nakomis commented on this pull request.
> @@ -231,7 +309,10 @@ private static boolean isOrphanedJcloudsAvailabilitySet(AvailabilitySet availabi
}
private boolean deleteVirtualMachine(String group, VirtualMachine virtualMachine) {
- return resourceDeleted.apply(api.getVirtualMachineApi(group).delete(virtualMachine.name()));
+ URI uri = api.getVirtualMachineApi(group).delete(virtualMachine.name());
+ boolean deleted = uri == null || resourceDeleted.apply(uri);
+ resourceRemoved.apply(IdReference.create(virtualMachine.id()));
See comment above about NetworkSecurityGroups / ResourceGroups
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1240#discussion_r235384767