You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2015/05/01 19:43:17 UTC
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
GitHub user sjcorbett opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/623
Improvements to JcloudsLocationSecurityGroupCustomizer
* Class is no longer final. Useful methods are exposed to subclasses.
* 0.0.0.0/0 used for CIDR for SSH so that Brooklyn failing over doesn't break its SSH access to machines.
* Incorporates SecurityGroupDefinitions.
* Adds ICMP permission to shared group.
* Predicate<Exception> can be provided to indicate that failed requests may be retried.
* Includes AwsExceptionRetryPredicate to retry InvalidGroup.InUse, DependencyViolation and RequestLimitExceeded errors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/incubator-brooklyn security-group-customiser
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/623.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #623
----
commit 64ae942b4296119b468fa2522ea53142e6851070
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-05-01T17:42:59Z
Improvements to JcloudsLocationSecurityGroupCustomizer
* Class is no longer final. Useful methods are exposed to subclasses.
* 0.0.0.0/0 used for CIDR for SSH so that Brooklyn failing over doesn't
break its SSH access to machines.
* Incorporates SecurityGroupDefinitions.
* Adds ICMP permission to shared group.
* Predicate<Exception> can be provided to indicate that failed requests
may be retried.
* Includes AwsExceptionRetryPredicate to retry InvalidGroup.InUse,
DependencyViolation and RequestLimitExceeded errors.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#discussion_r30515696
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -77,19 +84,31 @@ public JcloudsLocationSecurityGroupCustomizer load(final String appContext) thro
/** Caches the base security group that should be shared between all instances in the same Jclouds location */
private final Cache<Location, SecurityGroup> sharedGroupCache = CacheBuilder.newBuilder().build();
+
/** Caches security groups unique to instances */
private final Cache<String, SecurityGroup> uniqueGroupCache = CacheBuilder.newBuilder().build();
+ /** The context for this location customizer. */
private final String applicationId;
- private final Supplier<Cidr> brooklynCidrSupplier;
- JcloudsLocationSecurityGroupCustomizer(String applicationId) {
- this(applicationId, new LocalhostExternalIpCidrSupplier());
+ /** The CIDR for addresses that may SSH to machines. */
+ private Supplier<Cidr> sshCidrSupplier;
+
+ /**
+ * A predicate indicating whether the customiser can retry a request to add a security group
+ * or a rule after an throwable is thrown.
+ */
+ private Predicate<Exception> isExceptionRetryable = Predicates.alwaysFalse();
+
+ protected JcloudsLocationSecurityGroupCustomizer(String applicationId) {
+ // Would be better to restrict with something like LocalhostExternalIpCidrSupplier, but
+ // we risk making machines inaccessible from Brooklyn when HA fails over.
+ this(applicationId, Suppliers.ofInstance(new Cidr("0.0.0.0/0")));
--- End diff --
use `Cidr.UNIVERSAL`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#discussion_r30515800
--- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -366,6 +426,62 @@ void clearSecurityGroupCaches() {
}
/**
+ * Runs the given callable. Repeats until the operation succeeds or {@link #isExceptionRetryable} indicates
+ * that the request cannot be retried.
+ */
+ protected <T> T runOperationWithRetry(Callable<T> operation) {
--- End diff --
could use `Repeater` -- it has `backoff` support
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#issuecomment-98185582
Credit to @richardcloudsoft for the first version of the retry logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#issuecomment-103090466
LGTM. haven't run tests but i believe you that they work. minor comments, no need for them to block merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/623
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#issuecomment-104607177
@ahgittin Thanks. I will incorporate comments in the future.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Improvements to JcloudsLocationSe...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/623#issuecomment-104597163
@sjcorbett if there are no objections i'm going to merge -- comments are small enough they can be addressed in a new PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---