You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2015/06/25 13:21:28 UTC
[GitHub] incubator-brooklyn pull request: Fixes issue where port was not be...
GitHub user nakomis opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/717
Fixes issue where port was not being opened if port range was overridden
The following YAML was failing to open the port (8080 or 9090) in the AWS security group
```
location: aws-ec2:eu-west-1
services:
- type: brooklyn.entity.webapp.jboss.JBoss7Server
brooklyn.config:
http.port: 9090+
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/incubator-brooklyn fix/open-ports-with-range-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/717.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 #717
----
commit b320104ffcf59a2787048893e2647327d8b10517
Author: Martin Harris <gi...@nakomis.com>
Date: 2015-06-25T11:09:32Z
Fixes issue where port was not being opened if port range was overridden
----
---
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: Fixes issue where port was not be...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115290661
The previous implementation had better behaviour where it would resolve only port config keys. You can keep the old behaviour, while introducing a new case where the value is `PortRange` and doesn't need resolving. Also for the case where the value is already available, but wrapped in a future, you can use ValueResolver with a 0 timeout.
Also the `SameServerDriverLifecycleEffectorTasks` doesn't use `getRaw`.
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33246801
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -456,6 +456,9 @@ protected final void callStartHooks() {}
Object value = getConfig(k);
if (value instanceof Integer){
ports.add((Integer)value);
+ } else if (PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
Why isn't this check picked up by line 452, would it be better to fix that?
---
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: Fixes issue where port was not be...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115686933
Thanks all! :+1:
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115240572
LGTM
---
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: Fixes issue where port was not be...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/717
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115234746
ok. But still better to pick it up on all keys that are a `PortRange`, not just those ending `.port`, right?
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33248952
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -456,6 +456,9 @@ protected final void callStartHooks() {}
Object value = getConfig(k);
if (value instanceof Integer){
ports.add((Integer)value);
+ } else if (PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
ok. But still better to pick it up on all keys that are a PortRange, not just those ending .port, right?
---
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: Fixes issue where port was not be...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115650032
@nakomis @neykov why not just `TypeCoercions(getRaw(PORT_KEY), PortRange.class)` ?
this should convert `int` to `PortRange` and then iterate, which might be slightly wasteful but worth it as it's simpler.
also @nakomis note uses of `Tasks.resolving(...)` -- resolving config is normally non-blocking but if blocking is a problem you can do `Tasks.resolving(<raw config>).timeout(ValueResolver.REAL_QUICK_WAIT)...`
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115674373
I think it's ready to be reviewed again
---
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: Fixes issue where port was not be...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115639792
I agree that the changes are useful, probably didn't explain clearly what I mean in the previous comment.
The current implementation will not resolve the value even if we know that it is a port range (from the config key type). Something in the spirit of your initial commit will keep existing behaviour, while still getting us the benefit from your changes:
```
if config key type is port range {
getConfig(k) - resolve value, we know it's a port range
} else if config key contains ".port" {
getConfig(k) - resolve value
} else {
v = getConfigRaw(k) - doesn't resolve
if (v instanceof PortRange) {
}
}
```
Not important for this PR, but in future we could go one step further and try to resolve the value with a 0 or a very short timeout so even more cases are covered.
wdyt?
---
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: Fixes issue where port was not be...
Posted by rdowner <gi...@git.apache.org>.
Github user rdowner commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115670512
@nakomis what's next for this PR? Are you still adding changes, or do you think it's ready and it should be reviewed again? Thanks :smile:
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115263858
The failing tests are related to these changes. The call to getRequiredOpenPorts blocks until all config is resolvable. Will switch to config().getRaw(k)
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115239089
Comments incorporated
---
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: Fixes issue where port was not be...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115676632
Looks great, can be merged.
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33250565
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -449,11 +449,14 @@ protected final void callStartHooks() {}
configKeys.addAll(getEntityType().getConfigKeys());
for (ConfigKey<?> k: configKeys) {
+ Object value = getConfig(k);
if (PortRange.class.isAssignableFrom(k.getType())) {
PortRange p = (PortRange)getConfig(k);
if (p != null && !p.isEmpty()) ports.add(p.iterator().next());
+ } else if (value != null && PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
nitpick: add the first if condition as an or to the second to remove duplicate code
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33248227
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -456,6 +456,9 @@ protected final void callStartHooks() {}
Object value = getConfig(k);
if (value instanceof Integer){
ports.add((Integer)value);
+ } else if (PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
452 checks the class of the key's type, then checks assignability of the value from getConfig. If you use `entityspec.configure(Attributes.HTTP_PORT, "9999+")` then it will be picked up on line 452, however if you use `entityspec.configure("http.port"), which is effectively what happens when you put "http.port" in the YAML, then it creates a new `ConfigKey<Object>` which is not picked up on line 452
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33251476
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -449,11 +449,14 @@ protected final void callStartHooks() {}
configKeys.addAll(getEntityType().getConfigKeys());
for (ConfigKey<?> k: configKeys) {
+ Object value = getConfig(k);
if (PortRange.class.isAssignableFrom(k.getType())) {
PortRange p = (PortRange)getConfig(k);
if (p != null && !p.isEmpty()) ports.add(p.iterator().next());
+ } else if (value != null && PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
Thinking about it, I'm not sure that the first if condition is needed at all. If the key's type is assignable to PortRange, then the value must be too (or null)
---
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: Fixes issue where port was not be...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33250131
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -456,6 +456,9 @@ protected final void callStartHooks() {}
Object value = getConfig(k);
if (value instanceof Integer){
ports.add((Integer)value);
+ } else if (PortRange.class.isAssignableFrom(value.getClass())) {
+ PortRange p = (PortRange)value;
+ if (p != null && !p.isEmpty()) ports.add(p.iterator().next());
--- End diff --
This won't work if there's a port collision, which will cause the corresponding port attribute to be incremented until a free port is found. In that case the entity will use the free port, but the firewall will open the initial port instead. Mostly a problem for `SameServerEntity`, `localhost` location. As you've copied the existing logic I am fine with keeping it as is, but worth adding a comment.
There's identical logic in `SameServerDriverLifecycleEffectorTasks` so better update it there as well.
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115648528
Right, I'd misunderstood your previous comment. I'll make the changes as you suggested, and update `SameServerDriverLifecycleEffectorTasks` too
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115283889
Ok, comments incorporated, and tests are passing. I've had to switch to `config().getRaw(k)`, but I've added a comment to explain why
---
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: Fixes issue where port was not be...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115680421
LGTM too
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#issuecomment-115575034
Using `http.port: 9999+` in the YAML (akin to using `entity.configure("http.port", "9999+")` as opposed to `entity.configure(Attributes.HTTP_PORT, "9999+")`, like I used in the unit test that I added) will create a `ConfigKey<Object>` and the previous implementation did recognise it as a port range. To determine that it is a port range, you must either resolve the value (which causes the thread to hang if any of them are futures etc, breaking `SoftwareProcessEntityLatchTest`), or use `getRaw`
I'd intended to update `SameServerDriverLifecycleEffectorTasks` to use `getRaw`, but overlooked that. If you agree the right approach is to use `getRaw`, I'll update `SameServerDriverLifecycleEffectorTasks`
---
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: Fixes issue where port was not be...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/717#discussion_r33249808
--- Diff: software/base/src/main/java/brooklyn/entity/basic/SoftwareProcessImpl.java ---
@@ -456,6 +456,9 @@ protected final void callStartHooks() {}
Object value = getConfig(k);
if (value instanceof Integer){
ports.add((Integer)value);
+ } else if (PortRange.class.isAssignableFrom(value.getClass())) {
--- End diff --
Ah, good point. My `else if` is in the wrong place
---
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.
---