You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2015/10/15 15:29:43 UTC
[GitHub] incubator-brooklyn pull request: Update Tomcat's webapp.url sensor...
GitHub user bostko opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/959
Update Tomcat's webapp.url sensor to show the public hostname
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/incubator-brooklyn tomcat_public_webapp_url
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/959.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 #959
----
commit 0461a66ea5082ab398ac5a1c907453517bf08391
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-10-15T13:29:11Z
Update Tomcat's webapp.url sensor to show the public hostname
----
---
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: Update Tomcat's webapp.url sensor...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-153782392
Thank you all for the discussion!
I made the changes and now I am testing.
---
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: Update Tomcat's webapp.url sensor...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-154130795
looks good!
---
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: Update Tomcat's webapp.url sensor...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#discussion_r42172681
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/webapp/JavaWebAppSshDriver.java ---
@@ -93,11 +93,11 @@ protected String inferRootUrl() {
if (isProtocolEnabled("https")) {
Integer port = getHttpsPort();
checkNotNull(port, "HTTPS_PORT sensors not set; is an acceptable port available?");
- return String.format("https://%s:%s/", getSubnetHostname(), port);
+ return String.format("https://%s:%s/", getHostname(), port);
--- End diff --
@grkvlt will this cause a problem from Clocker?
The problem is it depends on the context for whether you want the subnet hostname/ip or the public hostname/ip. We don't know at this point how the main.uri sensor is going to be used.
---
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: Update Tomcat's webapp.url sensor...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-151561676
Don't fully agree @ahgittin, `main.uri` should be the externally accessible URL, where sensors will poll for values, will be given to users to connect to. Whether you need an external or internal url *between* entities is known at composition time only. If doing it in yaml you'll want to override the default sensor. Java group entities usually assume local network and directly use the subnet sensor (see [this](https://github.com/apache/incubator-brooklyn/blob/master/software/nosql/src/main/java/org/apache/brooklyn/entity/nosql/riak/RiakClusterImpl.java#L231-L232) for example).
So until we have a principled way to handle this I'd say keep the public IP.
---
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: Update Tomcat's webapp.url sensor...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-151822681
@neykov i am persuaded by your logic. +1 to your and @grkvlt 's suggestion.
@bostko disregard my previous comment (to close this) but please can you apply `BrooklynAccessUtils.getBrooklynAccessibleAddress(...)` to get the public host and port -- and confirm it works in both GCE (and clocker w calico!) if possible -- then we can merge this?
---
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: Update Tomcat's webapp.url sensor...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#discussion_r42565010
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/webapp/JavaWebAppSshDriver.java ---
@@ -93,11 +93,11 @@ protected String inferRootUrl() {
if (isProtocolEnabled("https")) {
Integer port = getHttpsPort();
checkNotNull(port, "HTTPS_PORT sensors not set; is an acceptable port available?");
- return String.format("https://%s:%s/", getSubnetHostname(), port);
+ return String.format("https://%s:%s/", getHostname(), port);
--- End diff --
@neykov @aledsage I think that `BrooklynAccessUtils` is actually the right choice to get the host and port for the `main.uri` URL which will therefore work with Clocker. Note that the current `getSubnetHostname()` call will return the SDN provided IP address, so with e.g. Calico defaults in Clocker you would get 50.0.0.2 as the address, and that is only accessible from other containers, not from Brookln. And `getHostname()` will return the Docker host IP address, so the port will then be wrong, due to forwarding.
---
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: Update Tomcat's webapp.url sensor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/959
---
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: Update Tomcat's webapp.url sensor...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-151554727
Close this, as per @neykov 's comments.
First requirement to me is that a load-balancer or other app in the same network (e.g. in docker) can use the URL sensor, it will want the local network ie the subnet one which is probably the wisest choice here.
Cross/public-network access is the special case which needs better semantics or topology to have a principled way to handle for generic entities and networks. /cc @grkvlt @aledsage
---
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: Update Tomcat's webapp.url sensor...
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/959#discussion_r42492268
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/webapp/JavaWebAppSshDriver.java ---
@@ -93,11 +93,11 @@ protected String inferRootUrl() {
if (isProtocolEnabled("https")) {
Integer port = getHttpsPort();
checkNotNull(port, "HTTPS_PORT sensors not set; is an acceptable port available?");
- return String.format("https://%s:%s/", getSubnetHostname(), port);
+ return String.format("https://%s:%s/", getHostname(), port);
--- End diff --
This was already changed from `getHostname` to `getSubnetHostname` to work in clocker env, so probably not safe to just change it back. Given that the value of "main.uri" (the value set here) is the URL we see in the summary tab @bostko's change seems like what the user would expect. So
* +1 for the change, but better use `BrooklynAccessUtils.getBrooklynAccessibleAddress` to be even more location independent
* we should figure out how the change fails in the clocker case (would be nice to add a test case) and figure out an alternative solution
* previously changed in https://github.com/apache/incubator-brooklyn/pull/554, some interesting comments there
@bostko Could you give some more details as to what is the underlying problem leading to this change.
---
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: Update Tomcat's webapp.url sensor...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#issuecomment-153767829
@bostko ping
---
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: Update Tomcat's webapp.url sensor...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/959#discussion_r42492781
--- Diff: software/webapp/src/main/java/org/apache/brooklyn/entity/webapp/JavaWebAppSshDriver.java ---
@@ -93,11 +93,11 @@ protected String inferRootUrl() {
if (isProtocolEnabled("https")) {
Integer port = getHttpsPort();
checkNotNull(port, "HTTPS_PORT sensors not set; is an acceptable port available?");
- return String.format("https://%s:%s/", getSubnetHostname(), port);
+ return String.format("https://%s:%s/", getHostname(), port);
--- End diff --
It fixes the main.uri for GCE.
---
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.
---