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
