Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uses externally accessible address for main uri of controller #1168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nakomis
Copy link
Contributor

@nakomis nakomis commented Jan 21, 2016

Previously, if deploying to BYON AWS instances, the internal IP address was being displayed in the main.uri for the ControlledDynamicWebAppCluster

@@ -283,7 +283,7 @@ protected String inferUrl(boolean requireManagementAccessible) {
}

protected String inferUrl() {
return inferUrl(false);
return inferUrl();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infinite loop?

@nakomis nakomis force-pushed the fix/abstract-controller-main-ui branch from 978d33d to 5971885 Compare January 21, 2016 16:46
@nakomis nakomis force-pushed the fix/abstract-controller-main-ui branch from 5971885 to c91d7eb Compare January 21, 2016 16:47
@nakomis
Copy link
Contributor Author

nakomis commented Jan 21, 2016

@sjcorbett Good catch, had pushed some code I hadn't intended to

@nakomis
Copy link
Contributor Author

nakomis commented Jan 22, 2016

Test failure appears unrelated, and JmxFeedTest passes locally

@ahgittin
Copy link
Contributor

@nakomis agree failure unrelated, have snuck an attempt to fix this in at #1171 btw

this behaviour change seems reasonable as we normally want a load-balancer to be accessible. however this might not always be the case and it does set up further inconsistencies in when main_uri is the subnet address vs main address. @grkvlt in particular in clocker is this going to cause an issue, i.e. might it combine the public IP with the private port? /cc @aledsage

we should set up some guidelines about when public v private ip's are used, and how to configure

as a quick win if we need to the inferUrl(true/false) call in this PR could be configurable via a config key, e.g. PREFER_ADVERTISING_PUBLIC_IP. but that feels ugly.

if it doesn't cause an issue in clocker i'm happy to merge and live with inconsistency for now, but let's be thinking about how to cleanly handle the public/private ambiguity!

@ahgittin
Copy link
Contributor

(plus another test failure fix at #1174)

@nakomis nakomis closed this Jan 29, 2016
@nakomis nakomis reopened this Jan 29, 2016
@ahgittin
Copy link
Contributor

ugly failure this time. not one i've come across. probably nondet. @nakomix you're not having luck!

org.apache.brooklyn.AssemblyTest.checkBrooklynCoreFeature

Failing for the past 1 build (Since Unstable#2472 )
Took 5.5 sec.
Error Message

gave up waiting for service org.apache.karaf.features.BootFinished
Stacktrace

org.ops4j.pax.swissbox.tracker.ServiceLookupException: gave up waiting for service org.apache.karaf.features.BootFinished
    at org.ops4j.pax.swissbox.tracker.ServiceLookup.getService(ServiceLookup.java:199)
    at org.ops4j.pax.swissbox.tracker.ServiceLookup.getService(ServiceLookup.java:136)
    at org.ops4j.pax.exam.inject.internal.ServiceInjector.injectField(ServiceInjector.java:89)
    at org.ops4j.pax.exam.inject.internal.ServiceInjector.injectDeclaredFields(ServiceInjector.java:69)
    at org.ops4j.pax.exam.inject.internal.ServiceInjector.injectFields(ServiceInjector.java:61)
    at org.ops4j.pax.exam.invoker.junit.internal.ContainerTestRunner.createTest(ContainerTestRunner.java:61)
    at org.junit.runners.BlockJUnit4ClassRunner$1.runReflectiveCall(BlockJUnit4ClassRunner.java:266)

@ahgittin
Copy link
Contributor

@nakomis could you spearhead the others incl @sjcorbett @aledsage @grkvlt to determine what we want to do wrt public/private?

i don't think we can merge this until we've got some guidelines.

@nakomis
Copy link
Contributor Author

nakomis commented Feb 19, 2016

/bump @sjcorbett @grkvlt @aledsage Any input on @ahgittin's comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants