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