Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
Closed #1202. -- 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/1202#event-3586273351
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
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/1202#issuecomment-663855703
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
@danielestevez any plans ti implement the cleanup improvements in this PR? If not feel free to close it and open a new one. -- 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/1202#issuecomment-400203295
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
This PR is about fixing the cleanup. As discussed previously that should be better achieved by tagging the implicit security groups jclouds creates. I'm ok at using this PR to implent this, or to close it and open a follow-up one. Up to you @danielestevez! And thanks for all the recent patches! -- 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/1202#issuecomment-391516887
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
This simple validator should work https://github.com/jclouds/jclouds/pull/1213 and we could close 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/1202#issuecomment-391501389
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
Thanks, @danielestevez. I've just added a comment to your commit. -- 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/1202#issuecomment-389865682
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
About this, i started to implement a custom validation for ARM here extending current interfaces https://github.com/danielestevez/jclouds/commits/groupNamingValidation Still not sure about the implementation, and specially naming but there it is if you want to take a look -- 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/1202#issuecomment-389671765
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
The DNS one is just a naming convention known to work for most providers. Being restrictive works fine for portability but conflicts when you're trying to use it with pre-existing stuff. In this case I'd say it makes sense to have a custom one for Azure, since it is only used to name things: vms, security groups, networks, etc, and all them should follow the same naming conventions... and if not, we'll find out! :) -- 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/1202#issuecomment-387073926
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
For the first point, i supposed it made sense to delete only those "implicit" orphaned groups created by jclouds, but it's true it'd delete groups from the subscription so adding a tag would make this process indeed more robust. For the validator, i thought there was maybe a reason to use a valid DNS ruleset so i did'nt want to change this as a first fix. If you can confirm there is not such reason i will add a new Azure-compatible validator and run the tests again! -- 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/1202#issuecomment-387071085
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
H I think it is still not the right fix. And I see two different issues
here:
1. We shouldn't rely on naming validation to identify something as "created by
jclouds".
2. The current validator does not match the Azure naming restrictions, so we'd
better change it.
For the second point, we could replace [the default
validator](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/internal/FormatSharedNamesAndAppendUniqueStringToThoseWhichRepeat.java#L79)
by a more relaxed implementation that better reflects Azure naming
restrictions. Once that validator is created we should be able to configure it
by binding it in the HTTP API module as:
```java
bind(new TypeLiteral>()
{}).to(CustomNamingValidator.class).in(Scopes.SINGLETON);
```
Once we have a proper naming validator in place for Azure, we should fix how we
identify the security groups we automatically created. We could do this in a
similar way to what we do with the availability sets: when we create these
implicit security groups
[here](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/loaders/CreateSecurityGroupIfNeeded.java#L91),
we could add a tag (say, `jclouds-`) to *mark* the
group as an auto-generated one. Then the cleanup strategy, apart from getting
the security group (it won't fail now as we have a proepr validator) will check
if it is has the tag, and decide if it has to be deleted or not based on that.
I think this approach is much more robust and we should go for something like
this.
--
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/1202#issuecomment-387002426
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
> I think this change is not safe Thanks for catching this, @nacx! Are we perhaps missing a unit test somewhere to ensure the cleanup method behaves as you described? -- 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/1202#issuecomment-386777891
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
Checked again: The problem happens when trying to security groups from nodes not created by jclouds and therefore, not respecting naming convention (using Upper case breaks [DnsNameValidator](https://github.com/danielestevez/jclouds/blob/9cdd53b0b7a87fa26a77b9ce370882f2a9cc7d71/core/src/main/java/org/jclouds/predicates/validators/DnsNameValidator.java#L55)) Now these security groups already existing in subscription will be ignored and only jclouds "implicit" created groups will be removed, as the original intent was. -- 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/1202#issuecomment-386639937
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
Ok! I'll take a look at it again. When i tried it it was trying to remove a security group already existing in the user subscription, maybe the problem is there -- 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/1202#issuecomment-386596221
Re: [jclouds/jclouds] Removes check for group name when deleting (#1202)
I think this change is not safe. The group name the method [gets passed in](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeService.java#L120) is **not** the name of a security group. It is the name of the *jclouds group* (that "logical" group jclouds configures when calling `computeService.createNodesInGroup`. That cleanup method is intended just to remove the security groups that are created automatically by jclouds (not directly by the user), when nodes are provisioned without specifying security groups, but using the `templateOptions.inboundPorts`. Those "implicit" securoty groups to open the configured ports are created [here](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CreateResourcesThenCreateNodes.java#L188-L194). Their name is derived from the "group" configured when creating nodes, and the cleanup method attemps to rebuild that name when deleting all orphaned security groups for that node. It should only attempt to delete those "implicit" security groups, so if there is a failure there, we should handle it differently, making sure we just try to delete those groups. Makes sense? -- 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/1202#issuecomment-386527745
[jclouds/jclouds] Removes check for group name when deleting (#1202)
This currently fails if you use a securityGroup with Upper case letters in your subscription. Naming check shouldn't be needed in a delete operation. You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds/pull/1202 -- Commit Summary -- * Removes check for group name when deleting -- File Changes -- M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CleanupResources.java (5) -- Patch Links -- https://github.com/jclouds/jclouds/pull/1202.patch https://github.com/jclouds/jclouds/pull/1202.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/1202
