Fix/azure resource removal #1240

nakomis · 2018-09-12T12:09:18Z

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

…tempting to delete group

nacx

Thanks, @nakomis! And many thanks for taking care of adding new tests. It's highly appreciated!

nacx · 2018-09-18T15:37:40Z

...ute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzurePredicatesModule.java

@@ -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.

nacx · 2018-09-18T15:43:27Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

+                  }
+                  deleted &= ipDeleted;
+               } catch (Exception ex) {
+                  logger.warn(">> Error deleting ip %s", ip);


Mark deleted = false too?

Also: log the exception?

nacx · 2018-09-18T15:49:33Z

...ute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzurePredicatesModule.java

@@ -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).

nacx · 2018-09-18T15:51:25Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

+         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.

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

nacx · 2018-09-18T15:53:29Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

@@ -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?

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

Sadly I can confirm this is the case. Even Azure Portal seems to have this type of eventual inconsistencies causing random failures

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 may be tempted to accidentally change this in the future.

nacx · 2018-09-18T15:55:02Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

+            } 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.

nacx · 2018-09-18T15:56:23Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

-      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.

See comment above about NetworkSecurityGroups / ResourceGroups

nacx · 2018-09-18T15:59:52Z

...pute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceLiveTest.java

+   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));

nacx · 2018-09-18T16:00:19Z

...pute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceLiveTest.java

+      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

nacx · 2018-09-18T16:04:02Z

...te-arm/src/test/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResourcesTest.java

+
+        // 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.

demobox · 2018-09-19T01:24:43Z

Thanks, @nakomis! And many thanks for taking care of adding new tests. It's highly appreciated!

+1 to that - many thanks!

demobox · 2018-09-19T01:28:13Z

...ute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzurePredicatesModule.java

+            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?

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

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?

demobox · 2018-09-19T01:32:34Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

+               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

demobox · 2018-09-19T01:33:06Z

...ompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java

+         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?

demobox · 2018-09-19T01:34:41Z

...te-arm/src/test/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResourcesTest.java

+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?

nakomis · 2018-11-21T13:24:54Z

Hi, thanks for the feedback. I've addressed a couple of the comments above, and pushed a couple of changes to address the others

nacx · 2018-11-23T09:19:05Z

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?

gaul · 2019-01-28T19:09:11Z

@nakomis does this pull request need anything to move forward?

gaul · 2020-07-25T13:30:18Z

Please reopen against apache/jclouds if this is still relevant.

nakomis added 2 commits September 12, 2018 13:07

Adds CleanupResourcesTest

fe4ff11

Waits for Azure resources to be removed from resource group before at…

1f79ca2

…tempting to delete group

nacx requested review from danielestevez and nacx September 12, 2018 12:13

nacx reviewed Sep 18, 2018

View reviewed changes

demobox reviewed Sep 19, 2018

View reviewed changes

gaul closed this Jul 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/azure resource removal #1240

Fix/azure resource removal #1240

nakomis commented Sep 12, 2018

nacx left a comment

nacx Sep 18, 2018

nacx Sep 18, 2018

demobox Sep 19, 2018

nacx Sep 18, 2018

nacx Sep 18, 2018

nakomis Nov 21, 2018

nacx Sep 18, 2018

nakomis Nov 21, 2018

danielestevez Nov 21, 2018

nacx Nov 23, 2018 •

edited

nacx Sep 18, 2018

nacx Sep 18, 2018

nakomis Nov 21, 2018

nacx Sep 18, 2018

nacx Sep 18, 2018

nacx Sep 18, 2018

demobox commented Sep 19, 2018

demobox Sep 19, 2018

danielestevez Sep 26, 2018

demobox Sep 26, 2018

demobox Sep 19, 2018

demobox Sep 19, 2018

demobox Sep 19, 2018

nakomis commented Nov 21, 2018

nacx commented Nov 23, 2018

gaul commented Jan 28, 2019

gaul commented Jul 25, 2020

Fix/azure resource removal #1240

Fix/azure resource removal #1240

Conversation

nakomis commented Sep 12, 2018

nacx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nacx Nov 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demobox commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nakomis commented Nov 21, 2018

nacx commented Nov 23, 2018

gaul commented Jan 28, 2019

gaul commented Jul 25, 2020

nacx Nov 23, 2018 •

edited