You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Andrea Turli <no...@github.com> on 2014/01/31 14:33:10 UTC
[jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created by
createNodesInGroup (#20)
You can merge this Pull Request by running:
git pull https://github.com/andreaturli/jclouds-labs-google fix/firewalls
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/20
-- Commit Summary --
* [JCLOUDS-442] reduces the firewalls created by createNodesInGroup
-- File Changes --
M google-compute-engine/pom.xml (5)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (169)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (4)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/strategy/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java (83)
A google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineComputeServiceLiveTest.java (101)
A google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapterLiveTest.java (141)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/internal/BaseGoogleComputeEngineApiLiveTest.java (11)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/20.patch
https://github.com/jclouds/jclouds-labs-google/pull/20.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -91,6 +91,21 @@
> <version>${jclouds.version}</version>
> <scope>test</scope>
> </dependency>
> + <!--
> + <exclusions>
> + <exclusion>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
> + <dependency>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + <version>0.9.0</version>
> + <scope>test</scope>
> + </dependency>
> + -->
> better to remove
Yes, please ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9469492
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
@demobox I think I've addressed all of your comments. Let's wait for buildhive and let me know if you want me to squash the commits before you merge it.
Shall we consider to port it also to master?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-34220599
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkState;
> +import static com.google.common.collect.Iterables.contains;
> +import static com.google.common.collect.Iterables.filter;
> +import static com.google.common.collect.Iterables.transform;
> +import static com.google.common.collect.Iterables.tryFind;
> +import static java.util.concurrent.TimeUnit.MILLISECONDS;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.CENTOS_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.DEBIAN_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_BOOT_DISK_SUFFIX;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_DELETE_BOOT_DISK_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_IMAGE_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_INTERVAL;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_TIMEOUT;
> +import static org.jclouds.googlecomputeengine.domain.Instance.NetworkInterface.AccessConfig.Type;
> +import static org.jclouds.googlecomputeengine.predicates.InstancePredicates.isBootDisk;
> +import static org.jclouds.util.Predicates2.retry;
Any reason for the import reordering?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468478
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
@danbroudy have you had the chance to look at it? I know the PR has been closed but I'm seeing issue with the current implementation. http://pastebin.com/3WxHWj2X
could you advice on that? thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-92890110
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #732](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/732/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-37302861
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> MILLISECONDS).apply(operation);
> - checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(), "Could not patch firewall, operation failed " + operation);
> + }
> + }
> +
> + private Set<Rule> createFirewallRulesFromInboundPorts(int[] inboundPorts) {
> + Set<Rule> rules = Sets.newLinkedHashSet();
Guess we need a sorted set here? Or why a LinkedHashSet?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358786
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> MILLISECONDS).apply(operation);
> checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
> + } else {
> + rules.addAll(firewall.getAllowed());
> + FirewallOptions firewallOptions = new FirewallOptions()
> + .name(firewallName)
> + .network(network.getSelfLink())
> + .allowedRules(rules)
> + .sourceTags(templateOptions.getTags())
> + .sourceRanges(of(DEFAULT_INTERNAL_NETWORK_RANGE, EXTERIOR_RANGE));
> + AtomicReference<Operation> operation = newReference(firewallApi.patch(firewallOptions.getName(), firewallOptions));
> + retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> + MILLISECONDS).apply(operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not patch firewall, operation failed" + operation);
I haven't checked in full detail, but would this work?
```
List... rules = ...
FirewallOptions firewallOptions = ... // without allowedRules
if (firewall == null) {
firewallOptions.allowedRules(rules)
...
} else {
rules.addAll(...)
firewallOptions.allowedRules(rules)
...
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468874
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> @@ -56,7 +57,7 @@ public FirewallOptions addAllowedRule(Firewall.Rule allowedRule) {
> /**
> * @see org.jclouds.googlecomputeengine.domain.Firewall#getAllowed()
> */
> - public FirewallOptions allowedRules(Set<Firewall.Rule> allowedRules) {
> + public FirewallOptions allowedRules(List<Firewall.Rule> allowedRules) {
This was an attempt, that I forgot to remove, sorry @demobox
I agree `Set` is better here, but there is a unit test testCreateNodeWhenNetworkNorFirewallExistDoesNotExist which fails randomly because of the order of the rules when I use Set. Any suggestions?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9470867
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by danbroudy <no...@github.com>.
> * @see org.jclouds.googlecomputeengine.features.FirewallApi#patch(String, org.jclouds.googlecomputeengine.options.FirewallOptions)
> */
> - private void getOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> - FirewallTagNamingConvention naming) {
> -
> + private void getAndUpdateOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> + String sharedResourceName) {
> + String firewallName = templateOptions.getNetworkName().or(sharedResourceName);
Ping. Seeing jclouds-labs-google was not promoted to core if we want to make these kinds of changes now is the time to do it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r28215641
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
Ouch, sorry you are right, but those failures are 2 expectedTests unrelated to the bouncycastle.
I'll fix that asap
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33820860
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Adrian Cole <no...@github.com>.
killing this as we are going to wildly refactor this codebase with auto. feel free to resubmit after!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-61285234
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -91,6 +91,21 @@
> <version>${jclouds.version}</version>
> <scope>test</scope>
> </dependency>
> + <!--
> + <exclusions>
> + <exclusion>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
> + <dependency>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + <version>0.9.0</version>
> + <scope>test</scope>
> + </dependency>
> + -->
I think we should be able to remove this bit [now](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=3db57e01b06d401da38b35850697812457df41cd)... ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468467
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #43](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/43/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33794238
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
There are also some new [Checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/49/org.apache.jclouds.labs$google-compute-engine/) - could you fix those? Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-37300953
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> @@ -91,6 +91,21 @@
> <version>${jclouds.version}</version>
> <scope>test</scope>
> </dependency>
> + <!--
> + <exclusions>
> + <exclusion>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
> + <dependency>
> + <groupId>net.schmizz</groupId>
> + <artifactId>sshj</artifactId>
> + <version>0.9.0</version>
> + <scope>test</scope>
> + </dependency>
> + -->
it's only commented out, better to remove
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468888
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> + FirewallOptions firewallOptions = new FirewallOptions()
> + .name(firewallName)
> + .network(network.getSelfLink())
> + .allowedRules(rules)
> + .sourceTags(templateOptions.getTags())
> + .sourceRanges(of(DEFAULT_INTERNAL_NETWORK_RANGE, EXTERIOR_RANGE));
> + AtomicReference<Operation> operation = newReference(firewallApi.patch(firewallOptions.getName(), firewallOptions));
> + retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> + MILLISECONDS).apply(operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not patch firewall, operation failed" + operation);
> + }
> + }
> +
> + private List<Firewall.Rule> createFirewallRulesFromInboundPorts(int[] inboundPorts) {
> + List<Firewall.Rule> rules = Lists.newArrayList();
> + for (Integer port : inboundPorts) {
`for (int port : inboundPorts)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468932
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by danbroudy <no...@github.com>.
Is this still an issue? We could rebase and reopen. Im happy to pick this up if people want me to.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-66388261
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
See discussion at [#273](https://github.com/jclouds/jclouds/pull/273#issuecomment-33970133)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33980122
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> * @see org.jclouds.googlecomputeengine.features.FirewallApi#patch(String, org.jclouds.googlecomputeengine.options.FirewallOptions)
> */
> - private void getOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> - FirewallTagNamingConvention naming) {
> -
> + private void getAndPatchOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
@demobox I see your point but here I wanted to highlight that the method is using `firewallApi.patch()` :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r10474752
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> MILLISECONDS).apply(operation);
> - checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(), "Could not patch firewall, operation failed " + operation);
> + }
> + }
> +
> + private Set<Rule> createFirewallRulesFromInboundPorts(int[] inboundPorts) {
[minor] static?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358767
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Adrian Cole <no...@github.com>.
Closed #20.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#event-186651478
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by danbroudy <no...@github.com>.
I agree this is still an issue! Im a little foggy on what is a problematic call, how Jclouds handles it before this PR and how it will handle it afterwards. Ill look into the code a little more.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-71746467
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> @@ -48,6 +48,17 @@
> <version>${jclouds.version}</version>
> </dependency>
> <dependency>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcpkix-jdk15on</artifactId>
> + <version>1.49</version>
> + <exclusions>
> + <exclusion>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcprov-jdk15on</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
> + <dependency>
Probably this is not needed but mostly depends on jclouds/jclouds#273
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9342518
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> @@ -70,7 +76,14 @@
>
>
> public BaseGoogleComputeEngineApiLiveTest() {
> - provider = "google-compute-engine";
> + this.provider = "google-compute-engine";
not sure why this happens, I'll revert as it seems more common on jclouds
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9469485
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> * @see org.jclouds.googlecomputeengine.features.FirewallApi#patch(String, org.jclouds.googlecomputeengine.options.FirewallOptions)
> */
> - private void getOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> - FirewallTagNamingConvention naming) {
> -
> + private void getAndUpdateOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> + String sharedResourceName) {
> + String firewallName = templateOptions.getNetworkName().or(sharedResourceName);
So if I understand correctly, we're removing the `FirewallTagNamingConvention` and replacing this with `templateOptions.getNetworkName()`, falling back to the group name if that is empty. That's not a backwards-compatible change, if I understand that correctly?
@abayer: Thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358585
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> No if I add that extra lib to the pom.xml my livetests are ok
Good. But there are still some unit test failures reported by BuildHive (even _with_ the new lib) - do you see those too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33820562
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkState;
> +import static com.google.common.collect.Iterables.contains;
> +import static com.google.common.collect.Iterables.filter;
> +import static com.google.common.collect.Iterables.transform;
> +import static com.google.common.collect.Iterables.tryFind;
> +import static java.util.concurrent.TimeUnit.MILLISECONDS;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.CENTOS_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.DEBIAN_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_BOOT_DISK_SUFFIX;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_DELETE_BOOT_DISK_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_IMAGE_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_INTERVAL;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_TIMEOUT;
> +import static org.jclouds.googlecomputeengine.domain.Instance.NetworkInterface.AccessConfig.Type;
> +import static org.jclouds.googlecomputeengine.predicates.InstancePredicates.isBootDisk;
> +import static org.jclouds.util.Predicates2.retry;
I think it depends on Intellij vs Eclipse :( Is it bad?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468942
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -459,7 +459,7 @@ public void testCreateNodeWhenNetworkNorFirewallExistDoesNotExist() throws RunNo
> .builder()
> .method("GET")
> .endpoint("https://www.googleapis" +
> - ".com/compute/v1/projects/myproject/global/firewalls/jclouds-test-port-22")
> + ".com/compute/v1/projects/myproject/global/firewalls/test")
Any reason for this change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358862
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs-google #654 UNSTABLE
Even with the Bouncycastle change there are some [test failures](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/654/testReport/). Are they related to the Bouncycastle thing, or..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33818053
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #44](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/44/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33795835
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
> @@ -48,6 +48,11 @@
> <version>${jclouds.version}</version>
> </dependency>
> <dependency>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcpkix-jdk15on</artifactId>
> + <version>1.49</version>
> + </dependency>
> + <dependency>
> <groupId>org.apache.jclouds.labs</groupId>
Probably this is not needed but mostly depends on https://github.com/jclouds/jclouds/pull/273
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9341996
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #48](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/48/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-34217430
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #668](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/668/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-34217149
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #654](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/654/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33795725
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -70,7 +76,14 @@
>
>
> public BaseGoogleComputeEngineApiLiveTest() {
> - provider = "google-compute-engine";
> + this.provider = "google-compute-engine";
Any reason for this change? What do we use in other ApiListTests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9469017
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #50](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/50/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-37303074
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #46](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/46/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33941480
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> * @see org.jclouds.googlecomputeengine.features.FirewallApi#patch(String, org.jclouds.googlecomputeengine.options.FirewallOptions)
> */
> - private void getOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
> - FirewallTagNamingConvention naming) {
> -
> + private void getAndPatchOrCreateFirewalls(GoogleComputeEngineTemplateOptions templateOptions, Network network,
Could we replace "Patch" with "Update" or "Modify" here? I always associate "Patch" with some kind of temporary fix ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r10474592
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by danbroudy <no...@github.com>.
Happy to help!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-93213658
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
- this commit maintains backwards-compatibility as it is only an internal implementation change.
- `launchCluster` is a good integration test but probably a specific test would be nice.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-37301381
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
hi @danbroudy I think it is still an issue. Basically the current code creates too many (identical) firewalls if I deploy multiple VMs in the same network. In fact, if the I ask for 10 nodes with specific inboundPorts, multiple firewalls with the same rules are created inside the network for the group of nodes.
This PR was an attempt to reduce the creation of the firewalls, given that there is a quota on that resource. In particular, FirewallTagNamingConvention doesn't look terribly helpful here and the PR was replacing it with templateOptions.getNetworkName(), falling back to the group name if that is empty.
Please give us your thoughts on that, if you have a better idea.
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-67072020
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by danbroudy <no...@github.com>.
> retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> MILLISECONDS).apply(operation);
> - checkState(!operation.get().getHttpError().isPresent(),"Could not create firewall, operation failed" + operation);
> + checkState(!operation.get().getHttpError().isPresent(), "Could not patch firewall, operation failed " + operation);
> + }
> + }
> +
> + private Set<Rule> createFirewallRulesFromInboundPorts(int[] inboundPorts) {
> + Set<Rule> rules = Sets.newLinkedHashSet();
> + for (int port : inboundPorts) {
> + rules.add(Rule.permitTcpRule(port));
This shouldn't add one rule per port. Rules can specify arrays of ranges. We need some logic to find the minimal number of rules for the given inbound ports. We should specify one rule for Tcp and one rule for Udp and each of those rules should enumerate all the ports.
using the ports form this pastebin as an example: http://pastebin.com/3WxHWj2X
[22, 4369, 6000-7999, 8087, 8093, 8098-8099, 8985] covers all the ports
```
"allowed": [
{
"IPProtocol": "tcp",
"ports":["22", "4369", "6000-7999", "8087", "8093", "8098-8099", "8985"],
},
{
"IPProtocol": "udp",
"ports": ["22", "4369", "6000-7999", "8087", "8093", "8098-8099", "8985"] ,
}
]
```
https://cloud.google.com/compute/docs/networking#firewalls_1 is a good resource
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r28387912
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> +import javax.annotation.Resource;
> +import javax.inject.Named;
> +import java.io.IOException;
> +import java.util.Set;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Test(groups = "live", testName = "GoogleComputeEngineComputeServiceLiveTest")
> +public class GoogleComputeEngineComputeServiceLiveTest extends BaseGoogleComputeEngineApiLiveTest {
> +
> + public static final String TEST_LAUNCH_CLUSTER = "experiment";
> + @Resource
[minor] Blank line before this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358822
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Ignasi Barrera <no...@github.com>.
Any update on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-43722792
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> + rules.addAll(firewall.getAllowed());
> + FirewallOptions firewallOptions = new FirewallOptions()
> + .name(firewallName)
> + .network(network.getSelfLink())
> + .allowedRules(rules)
> + .sourceTags(templateOptions.getTags())
> + .sourceRanges(of(DEFAULT_INTERNAL_NETWORK_RANGE, EXTERIOR_RANGE));
> + AtomicReference<Operation> operation = newReference(firewallApi.patch(firewallOptions.getName(), firewallOptions));
> + retry(operationDonePredicate, operationCompleteCheckTimeout, operationCompleteCheckInterval,
> + MILLISECONDS).apply(operation);
> + checkState(!operation.get().getHttpError().isPresent(),"Could not patch firewall, operation failed" + operation);
> + }
> + }
> +
> + private List<Firewall.Rule> createFirewallRulesFromInboundPorts(int[] inboundPorts) {
> + List<Firewall.Rule> rules = Lists.newArrayList();
[minor] static import Lists.newArrayList()?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468920
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #47](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/47/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-34157384
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkState;
> +import static com.google.common.collect.Iterables.contains;
> +import static com.google.common.collect.Iterables.filter;
> +import static com.google.common.collect.Iterables.transform;
> +import static com.google.common.collect.Iterables.tryFind;
> +import static java.util.concurrent.TimeUnit.MILLISECONDS;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.CENTOS_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.DEBIAN_PROJECT;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_BOOT_DISK_SUFFIX;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_DELETE_BOOT_DISK_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.GCE_IMAGE_METADATA_KEY;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_INTERVAL;
> +import static org.jclouds.googlecomputeengine.GoogleComputeEngineConstants.OPERATION_COMPLETE_TIMEOUT;
> +import static org.jclouds.googlecomputeengine.domain.Instance.NetworkInterface.AccessConfig.Type;
> +import static org.jclouds.googlecomputeengine.predicates.InstancePredicates.isBootDisk;
> +import static org.jclouds.util.Predicates2.retry;
Not bad, but it makes the diff unnecessarily big. It would be nice if it could be avoided, but I don't think we need to manually fix the PR for this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9473122
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
The builds look good, which is nice. Only one small comment about the method name. Some questions:
* Is the default behaviour backwards-compatible, or are we changing default behaviour with this commit? If so, do we need to document to users how to maintain backwards-compatible behaviour if they wish?
* Do we need a test to verify that the new logic works as expected? Or does the "launchCluster" test cover that case?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-37300854
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> - network.getSelfLink(),
> - firewallOptions));
> - operations.add(operation);
> - }
> - }
> -
> - for (AtomicReference<Operation> operation : operations) {
> + Firewall firewall = firewallApi.get(firewallName);
> + Set<Rule> rules = createFirewallRulesFromInboundPorts(templateOptions.getInboundPorts());
> + FirewallOptions firewallOptions = new FirewallOptions().name(firewallName).network(network.getSelfLink())
> + .sourceTags(templateOptions.getTags())
> + .sourceRanges(of(DEFAULT_INTERNAL_NETWORK_RANGE,
> + EXTERIOR_RANGE));
> + if (firewall == null) {
> + firewallOptions.allowedRules(rules);
> + AtomicReference<Operation> operation = newReference(firewallApi.createInNetwork(firewallOptions.getName(),
[minor] Formatting: move `newReference` down onto the next line and then put all the arguments on _one_line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358713
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #653](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/653/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33794234
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -56,7 +57,7 @@ public FirewallOptions addAllowedRule(Firewall.Rule allowedRule) {
> /**
> * @see org.jclouds.googlecomputeengine.domain.Firewall#getAllowed()
> */
> - public FirewallOptions allowedRules(Set<Firewall.Rule> allowedRules) {
> + public FirewallOptions allowedRules(List<Firewall.Rule> allowedRules) {
Worth mentioning in the Javadoc that duplicates will be ignored? And should the type be `Collection` rather than `List` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r9468984
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #49](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/49/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-34221076
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
@andreaturli Just getting back to this. I see this PR is against 1.7.x...could we start with master? Is this still an issue on master?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-39766904
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
No if I add that extra lib to the pom.xml my livetests are ok
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33819803
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
> @@ -28,4 +33,13 @@
> public GoogleComputeEngineSecurityGroupExtensionLiveTest() {
> provider = "google-compute-engine";
> }
> +
> + @Override
> + protected Properties setupProperties() {
> + Properties props = super.setupProperties();
> + setCredentialFromPemFile(props, provider + ".credential");
> + props.setProperty(TEMPLATE, "osFamily=CENTOS,locationId=us-central1-a,loginUser=jclouds");
Do we know which template this finds? Is this a special template we've prepared?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20/files#r11358906
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrew Phillips <no...@github.com>.
@andreaturli: which live test is failing? According to `mvn dependency:list`, this repo should only have a _test_ dependency on Bouncycastle:
```
C:\...\jclouds-labs-google>mvn dependency:list | findstr "bouncy"
[INFO] org.apache.jclouds.driver:jclouds-bouncycastle:jar:1.7.1-SNAPSHOT:test
[INFO] org.bouncycastle:bcprov-ext-jdk15on:jar:1.49:test
```
If we have an "undocumented dep" somewhere, that should indeed be fixed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-33822452
Re: [jclouds-labs-google] [JCLOUDS-442] reduces the firewalls created
by createNodesInGroup (#20)
Posted by Andrea Turli <no...@github.com>.
Thanks @danbroudy I hope to have sono the chance to fix that and hopefully also add a good logica to delete firewall rules once the VM gets deleted. I'd really like to have your thoughts once the PR is ready
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/20#issuecomment-93177897