You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2014/11/21 12:52:29 UTC

[GitHub] incubator-brooklyn pull request: DO NOT MERGE: Fix various 2014112...

GitHub user aledsage opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/353

    DO NOT MERGE: Fix various 20141120

    This is currently for review only, and for sharing with @Nakomis
    I'm happy with the first two commits; but not the "TODO Hack" commit!

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/aledsage/incubator-brooklyn fix/various-20141120

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/353.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 #353
    
----
commit 0ae2f1255afe827cf1a6df0b1ab186cae6944bd0
Author: Aled Sage <al...@gmail.com>
Date:   2014-11-21T11:46:13Z

    Adds sizeHistoryTest, and TimeWindowedList.builder
    
    - Also supports taking Duration, rather than long

commit 3e970b407faade5a0741d24d20f6047646813ffc
Author: Aled Sage <al...@gmail.com>
Date:   2014-11-21T11:48:22Z

    Adds PortForwardManager.getId()
    
    - And PortForwardManagerAuthority.hasOwningEntity()

commit 810755b09666f9016c1406cd35b4b7d38257369e
Author: Aled Sage <al...@gmail.com>
Date:   2014-11-21T11:50:55Z

    TODO Hack for PortForwardManagerAuthority starting port
    
    - The PortForwardManagerAuthority can hand out ports. However, it
      just uses an internal counter for this.
    - This adds support for configuring the starting port number.

----


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21028368
  
    --- Diff: core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java ---
    @@ -0,0 +1,83 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.access;
    +
    +import org.testng.Assert;
    +import org.testng.annotations.AfterMethod;
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.entity.basic.Entities;
    +import brooklyn.location.Location;
    +import brooklyn.management.internal.LocalManagementContext;
    +import brooklyn.test.entity.LocalManagementContextForTests;
    +
    +public class PortForwardManagerLocationResolverTest {
    +
    +    private LocalManagementContext managementContext;
    +
    +    @BeforeMethod(alwaysRun=true)
    +    public void setUp() throws Exception {
    +        managementContext = LocalManagementContextForTests.newInstance();
    +    }
    +    
    +    @AfterMethod(alwaysRun=true)
    +    public void tearDown() throws Exception {
    +        if (managementContext != null) Entities.destroyAll(managementContext);
    +    }
    +    
    +    @Test
    +    public void testReturnsSameInstanceBasedOnScope() {
    +        Location global1 = resolve("portForwardManager()"); // defaults to global
    +        Location global2 = resolve("portForwardManager()");
    +        Location global3 = resolve("portForwardManager(scope=global)");
    +        assertSame(global1, global2);
    +        assertSame(global1, global3);
    +        
    +        Location a1 = resolve("portForwardManager(scope=a)");
    +        Location a2 = resolve("portForwardManager(scope=a)");
    +        assertSame(a1, a2);
    +        assertNotSame(global1, a1);
    +        
    +        Location b1 = resolve("portForwardManager(scope=b)");
    +        assertNotSame(global1, b1);
    +        assertNotSame(a1, b1);
    +    }
    +
    +    private Location resolve(String val) {
    +        Location l = managementContext.getLocationRegistry().resolve(val);
    +        Assert.assertNotNull(l);
    +        return l;
    +    }
    +    
    +    private void assertSame(Location loc1, Location loc2) {
    +        Assert.assertNotNull(loc1);
    +        Assert.assertTrue(loc1 instanceof PortForwardManager, "loc1="+loc1);
    +        Assert.assertSame(loc1, loc2);
    +    }
    +    
    +    private void assertNotSame(Location loc1, Location loc2) {
    +        Assert.assertNotNull(loc1);
    +        Assert.assertNotNull(loc2);
    +        Assert.assertTrue(loc2 instanceof PortForwardManager, "loc1="+loc1);
    --- End diff --
    
    Should assert on `loc1`.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21028151
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    + * handle a unique identifier for it.
    + * 
    + * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
    + * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
    + * 
      * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
      * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
      * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
      */
     @Beta
    -public interface PortForwardManager {
    +public interface PortForwardManager extends Location {
     
    -    /**
    -     * Reserves a unique public port on the given publicIpId.
    -     * <p>
    -     * Often followed by {@link #associate(String, int, Location, int)}
    -     * to enable {@link #lookup(Location, int)}.
    -     */
    -    public int acquirePublicPort(String publicIpId);
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    +            "scope",
    --- End diff --
    
    Just "scope" rather than in the "brooklyn.portForwardManager" namespace?


---
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: Fix various 20141120 (for PortFor...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#issuecomment-64888428
  
    Latest commit incorporate comments from @sjcorbett up to the one about "Should assert on loc1" from 14 minutes ago. Except for where I've replied explicitly to the comment.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027812
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -123,19 +159,90 @@
          * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
          * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
          * e.g. if the location is not known ahead of time.
    +     * 
    +     * @deprecated Use {@link #associate(String, HostAndPort, Location, int)}
          */
    +    @Deprecated
         public void associate(String publicIpId, int publicPort, Location l, int privatePort);
     
    -    /** Returns the subset of port mappings associated with a given location. */
    -    public Collection<PortMapping> getLocationPublicIpIds(Location l);
    -        
    -    /** Returns the mapping to a given private port, or null if none. */
    -    public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort);
    +    /**
    +     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
    +     * <p>
    +     * Conceivably this may have to be access-location specific.
    +     * 
    +     * @deprecated Use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
    +     */
    +    @Deprecated
    +    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress);
     
         /**
    -     * Returns true if this implementation is a client which is immutable/safe for serialization
    -     * i.e. it delegates to something on an entity or location elsewhere.
    +     * Returns a recorded public hostname or address.
    +     * 
    +     * @deprecated Use {@link #lookup(String, int)} or {@link #lookup(Location, int)}
          */
    -    public boolean isClient();
    +    @Deprecated
    +    public String getPublicIpHostname(String publicIpId);
         
    +    /**
    +     * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
    +     * 
    +     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
    --- End diff --
    
    `forgetPortMapping(Location, int)` doesn't exist. `forgetPortMapping(Location)` does.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027713
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -115,7 +99,59 @@
          * @see #recordPublicIpHostname(String, String)
          */
         public HostAndPort lookup(Location l, int privatePort);
    +
    +    /**
    +     * Returns the public endpoint (host and port) for use contacting the given endpoint.
    +     * 
    +     * Expects a previous call to {@link #associate(String, HostAndPort, int)}, to register
    +     * the endpoint.
    +     * 
    +     * Will return null if there has not been a public endpoint associated with this pairing.
    +     */
    +    public HostAndPort lookup(String publicIpId, int privatePort);
    +
    +    /** 
    +     * Clears the given port mapping, returning the true if there was a match.
    --- End diff --
    
    "the true"


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027748
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    + * handle a unique identifier for it.
    + * 
    + * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
    + * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
    + * 
      * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
      * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
      * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
      */
     @Beta
    -public interface PortForwardManager {
    +public interface PortForwardManager extends Location {
     
    -    /**
    -     * Reserves a unique public port on the given publicIpId.
    -     * <p>
    -     * Often followed by {@link #associate(String, int, Location, int)}
    -     * to enable {@link #lookup(Location, int)}.
    -     */
    -    public int acquirePublicPort(String publicIpId);
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    +            "scope",
    +            "The scope that this applies to, defaulting to global",
    +            "global");
     
    -    /** Returns old mapping if it existed, null if it is new. */
    -    public PortMapping acquirePublicPortExplicit(String publicIpId, int port);
    +    @Beta
    +    public static final ConfigKey<Integer> PORT_FORWARD_MANAGER_STARTING_PORT = ConfigKeys.newIntegerConfigKey(
    +            "brooklyn.portForwardManager.startingPort",
    +            "The starting port for assigning port numbers, such as for DNAT",
    +            11000);
     
    -    /** Returns the port mapping for a given publicIpId and public port. */
    -    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort);
    +    public String getScope();
     
    -    /** Returns the subset of port mappings associated with a given public IP ID. */
    -    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId);
    -
    -    /** Clears the given port mapping, returning the mapping if there was one. */
    -    public PortMapping forgetPortMapping(String publicIpId, int publicPort);
    -    
    -    /** @see #forgetPortMapping(String, int) */
    -    public boolean forgetPortMapping(PortMapping m);
    -
    -    // -----------------
    -    
         /**
    -     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
    +     * Reserves a unique public port on the given publicIpId.
          * <p>
    -     * Conceivably this may have to be access-location specific.
    +     * Often followed by {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
    +     * to enable {@link #lookup(String, int)} or {@link #lookup(Location, int)} respectively.
          */
    -    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress);
    -
    -    /** Returns a recorded public hostname or address. */
    -    public String getPublicIpHostname(String publicIpId);
    -    
    -    /** Clears a previous call to {@link #recordPublicIpHostname(String, String)}. */
    -    public boolean forgetPublicIpHostname(String publicIpId);
    +    public int acquirePublicPort(String publicIpId);
     
         /**
    -     * Returns the public host and port for use accessing the given mapping.
    +     * Records a location and private port against a public endpoint (ip and port),
    +     * to support {@link #lookup(Location, int)}.
          * <p>
    -     * Conceivably this may have to be access-location specific.
    +     * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
    +     * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
    +     * e.g. if the location is not known ahead of time.
          */
    -    public HostAndPort getPublicHostAndPort(PortMapping m);
    +    public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort);
     
    -    // -----------------
    -    
         /**
    -     * Reserves a unique public port for the purpose of forwarding to the given target,
    -     * associated with a given location for subsequent lookup purpose.
    -     * <p>
    -     * If already allocated, returns the previously allocated.
    +     * Records a mapping for pubilcIpId:privatePort to a public endpoint, such that it can
    --- End diff --
    
    Typo pubilcIpId


---
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: Fix various 20141120 (for PortFor...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/353


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027447
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    --- End diff --
    
    Duplicates sentence a few lines below.


---
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: Fix various 20141120 (for PortFor...

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/353#discussion_r21028696
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    + * handle a unique identifier for it.
    + * 
    + * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
    + * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
    + * 
      * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
      * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
      * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
      */
     @Beta
    -public interface PortForwardManager {
    +public interface PortForwardManager extends Location {
     
    -    /**
    -     * Reserves a unique public port on the given publicIpId.
    -     * <p>
    -     * Often followed by {@link #associate(String, int, Location, int)}
    -     * to enable {@link #lookup(Location, int)}.
    -     */
    -    public int acquirePublicPort(String publicIpId);
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    +            "scope",
    --- End diff --
    
    Yeah; not particularly happy with that.
    But it means we can use a location spec of `portForwardManager(scope=global)` rather than having to do `portForwardManager(brooklyn.portForwardManager.scope=global)`. 
    
    Also, the config being read by the `PortForwardManagerLocationResolver` doesn't respect `@SetFromFlag("scope")`. I guess that could look up the explicit string "scope" rather than referencing the config key for the lookup, but that doesn't feel good either.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027987
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
    @@ -0,0 +1,408 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.access;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.ArrayList;
    +import java.util.Collection;
    +import java.util.Iterator;
    +import java.util.LinkedHashMap;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.atomic.AtomicInteger;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.rebind.BasicLocationRebindSupport;
    +import brooklyn.entity.rebind.RebindContext;
    +import brooklyn.entity.rebind.RebindSupport;
    +import brooklyn.location.Location;
    +import brooklyn.location.basic.AbstractLocation;
    +import brooklyn.mementos.LocationMemento;
    +import brooklyn.util.collections.MutableMap;
    +
    +import com.google.common.annotations.Beta;
    +import com.google.common.base.Objects.ToStringHelper;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import com.google.common.net.HostAndPort;
    +
    +@SuppressWarnings("serial")
    +public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager {
    +
    +    // TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
    +    // Need to improve the efficiency and scale.
    +    // A quick win could be to use a different portReserved counter for each publicIpId,
    +    // when calling acquirePublicPort?
    +    
    +    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
    --- End diff --
    
    Is it worth indicating this in Javadoc on the class?


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027888
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java ---
    @@ -89,69 +95,275 @@ protected PortForwardManager getDelegate() {
             return _delegate;
         }
     
    +    @Override
         public int acquirePublicPort(String publicIpId) {
             return getDelegate().acquirePublicPort(publicIpId);
         }
     
    -    public PortMapping acquirePublicPortExplicit(String publicIpId, int port) {
    -        return getDelegate().acquirePublicPortExplicit(publicIpId, port);
    +    @Override
    +    public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
    +        getDelegate().associate(publicIpId, publicEndpoint, l, privatePort);
         }
     
    -    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
    -        return getDelegate().getPortMappingWithPublicSide(publicIpId, publicPort);
    +    @Override
    +    public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort) {
    +        getDelegate().associate(publicIpId, publicEndpoint, privatePort);
         }
     
    -    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
    -        return getDelegate().getPortMappingWithPublicIpId(publicIpId);
    +    @Override
    +    public HostAndPort lookup(Location l, int privatePort) {
    +        return getDelegate().lookup(l, privatePort);
    +    }
    +
    +    @Override
    +    public HostAndPort lookup(String publicIpId, int privatePort) {
    +        return getDelegate().lookup(publicIpId, privatePort);
         }
     
    -    public PortMapping forgetPortMapping(String publicIpId, int publicPort) {
    +    @Override
    +    public boolean forgetPortMapping(String publicIpId, int publicPort) {
             return getDelegate().forgetPortMapping(publicIpId, publicPort);
         }
     
    -    public boolean forgetPortMapping(PortMapping m) {
    -        return getDelegate().forgetPortMapping(m);
    +    @Override
    +    public boolean forgetPortMappings(Location location) {
    +        return getDelegate().forgetPortMappings(location);
    +    }
    +
    +    @Override
    +    public String getId() {
    +        return getDelegate().getId();
    +    }    
    +
    +    @Override
    +    public String getScope() {
    +        return getDelegate().getScope();
    +    }    
    +
    +    @Override
    +    public boolean isClient() {
    +        return true;
         }
     
    +    @Override
    +    public String toVerboseString() {
    +        return getClass().getName()+"[wrapping="+getDelegate().toVerboseString()+"]";
    +    }
    +    
    +    
    +    ///////////////////////////////////////////////////////////////////////////////////
    +    // Deprecated
    +    ///////////////////////////////////////////////////////////////////////////////////
    +
    +    /**
    +     * Reserves a unique public port for the purpose of forwarding to the given target,
    +     * associated with a given location for subsequent lookup purpose.
    +     * <p>
    +     * If already allocated, returns the previously allocated.
    +     * 
    +     * @deprecated since 0.7.0; use {@link #acquirePublicPort(String)}, and then use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
    +     */
    +    @Deprecated
    +    public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
    +        return getDelegate().acquirePublicPort(publicIpId, l, privatePort);
    +    }
    +
    +    /** 
    +     * Returns old mapping if it existed, null if it is new.
    +     * 
    +     * @deprecated since 0.7.0; use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
    +     */
    +    @Deprecated
    +    public PortMapping acquirePublicPortExplicit(String publicIpId, int publicPort) {
    +        return getDelegate().acquirePublicPortExplicit(publicIpId, publicPort);
    +    }
    +
    +    /**
    +     * Records a location and private port against a publicIp and public port,
    +     * to support {@link #lookup(Location, int)}.
    +     * <p>
    +     * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
    +     * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
    +     * e.g. if the location is not known ahead of time.
    +     * 
    +     * @deprecated Use {@link #associate(String, HostAndPort, Location, int)}
    +     */
    +    @Deprecated
    +    public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
    +        getDelegate().associate(publicIpId, publicPort, l, privatePort);
    +    }
    +
    +    /**
    +     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
    +     * <p>
    +     * Conceivably this may have to be access-location specific.
    +     * 
    +     * @deprecated Use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
    +     */
    +    @Deprecated
         public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
             getDelegate().recordPublicIpHostname(publicIpId, hostnameOrPublicIpAddress);
         }
     
    +    /**
    +     * Returns a recorded public hostname or address.
    +     * 
    +     * @deprecated Use {@link #lookup(String, int)} or {@link #lookup(Location, int)}
    +     */
    +    @Deprecated
         public String getPublicIpHostname(String publicIpId) {
             return getDelegate().getPublicIpHostname(publicIpId);
         }
    -
    +    
    +    /**
    +     * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
    +     * 
    +     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
    --- End diff --
    
    As before, `forgetPortMapping(Location, int)` doesn't exist.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21027494
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    + * handle a unique identifier for it.
    + * 
    + * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
    + * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
    + * 
      * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
      * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
      * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
      */
     @Beta
    -public interface PortForwardManager {
    +public interface PortForwardManager extends Location {
     
    -    /**
    -     * Reserves a unique public port on the given publicIpId.
    -     * <p>
    -     * Often followed by {@link #associate(String, int, Location, int)}
    -     * to enable {@link #lookup(Location, int)}.
    -     */
    -    public int acquirePublicPort(String publicIpId);
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    +            "scope",
    +            "The scope that this applies to, defaulting to global",
    --- End diff --
    
    How do I find the other scopes I can use?


---
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: Fix various 20141120 (for PortFor...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#issuecomment-64887105
  
    The failed build looks unconnected to this PR:
    
        brooklyn.event.feed.jmx.RebindJmxFeedTest.testJmxFeedIsPersisted (from TestSuite)
    
        Port already in use: 40131; nested exception is: 
        java.net.BindException: Address already in use
    
    I'll look at that for a separate 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.
---

[GitHub] incubator-brooklyn pull request: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21028105
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
    @@ -0,0 +1,408 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.access;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.ArrayList;
    +import java.util.Collection;
    +import java.util.Iterator;
    +import java.util.LinkedHashMap;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.atomic.AtomicInteger;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.rebind.BasicLocationRebindSupport;
    +import brooklyn.entity.rebind.RebindContext;
    +import brooklyn.entity.rebind.RebindSupport;
    +import brooklyn.location.Location;
    +import brooklyn.location.basic.AbstractLocation;
    +import brooklyn.mementos.LocationMemento;
    +import brooklyn.util.collections.MutableMap;
    +
    +import com.google.common.annotations.Beta;
    +import com.google.common.base.Objects.ToStringHelper;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import com.google.common.net.HostAndPort;
    +
    +@SuppressWarnings("serial")
    +public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager {
    +
    +    // TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
    +    // Need to improve the efficiency and scale.
    +    // A quick win could be to use a different portReserved counter for each publicIpId,
    +    // when calling acquirePublicPort?
    +    
    +    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
    +    // If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then they 
    +    // must not allocate the same public port (so can share the same PortForwardManagerAuthority
    +    // via PortForwardManager.Factory.sharedInstance).
    +    // However, this still doesn't check if the port is *actually* available. For example, if a
    +    // different Brooklyn instance is also deploying there then we can get port conflicts, or if 
    +    // some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
    +    // will not be respected. Callers should probably figure out the port number themselves, but
    +    // that also leads to concurrency issues.
    +
    +    // TODO The publicIpId means different things to different callers:
    +    //  - In acquirePublicPort() it is (often?) an identifier of the actual public ip.
    +    //  - In later calls to associate(), it is (often?) an identifier for the target machine
    +    //    such as the jcloudsMachine.getJcloudsId().
    +    
    +    // TODO Should PortForwardManager be changed to extend AbstractLocation, or is this wrapper ok?
    +    
    +    private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
    +    
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    --- End diff --
    
    Well, `PORT_FORWARD_MANAGER_STARTING_PORT` is `PORT_FORWARD_MANAGER_STARTING_POINT` on the interface but is otherwise the same.


---
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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#discussion_r21028075
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java ---
    @@ -0,0 +1,408 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.location.access;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.ArrayList;
    +import java.util.Collection;
    +import java.util.Iterator;
    +import java.util.LinkedHashMap;
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.atomic.AtomicInteger;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.rebind.BasicLocationRebindSupport;
    +import brooklyn.entity.rebind.RebindContext;
    +import brooklyn.entity.rebind.RebindSupport;
    +import brooklyn.location.Location;
    +import brooklyn.location.basic.AbstractLocation;
    +import brooklyn.mementos.LocationMemento;
    +import brooklyn.util.collections.MutableMap;
    +
    +import com.google.common.annotations.Beta;
    +import com.google.common.base.Objects.ToStringHelper;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import com.google.common.net.HostAndPort;
    +
    +@SuppressWarnings("serial")
    +public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager {
    +
    +    // TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
    +    // Need to improve the efficiency and scale.
    +    // A quick win could be to use a different portReserved counter for each publicIpId,
    +    // when calling acquirePublicPort?
    +    
    +    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
    +    // If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then they 
    +    // must not allocate the same public port (so can share the same PortForwardManagerAuthority
    +    // via PortForwardManager.Factory.sharedInstance).
    +    // However, this still doesn't check if the port is *actually* available. For example, if a
    +    // different Brooklyn instance is also deploying there then we can get port conflicts, or if 
    +    // some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
    +    // will not be respected. Callers should probably figure out the port number themselves, but
    +    // that also leads to concurrency issues.
    +
    +    // TODO The publicIpId means different things to different callers:
    +    //  - In acquirePublicPort() it is (often?) an identifier of the actual public ip.
    +    //  - In later calls to associate(), it is (often?) an identifier for the target machine
    +    //    such as the jcloudsMachine.getJcloudsId().
    +    
    +    // TODO Should PortForwardManager be changed to extend AbstractLocation, or is this wrapper ok?
    +    
    +    private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
    +    
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    --- End diff --
    
    Why redeclare `SCOPE` and `PORT_FORWARD_MANAGER_STARTING_PORT` when they're already on the interface?


---
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: Fix various 20141120 (for PortFor...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#issuecomment-64885639
  
    Note that the purpose of making PortForwardManager a singleton location is two fold:
    
    1. The instance needs to be shared by all applications (particularly multiple apps that are being deployed to the same cloud account, such as to vcloud's vCHS so are updating the same set of NAT rules).
    
    2. This simplifies peristence - we just persist that location automatically. Previously it was a config/attribute of an entity. We tried to have exactly one entity having the authoritative object, and all the others just being proxies that would talk to that shared instance. Another option was to add `managementContext.getPortForwardManager`, but having spoken it over with @ahgittin we were reluctant to more on `managementContext` (which would have made persisting it more complicated 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: Fix various 20141120 (for PortFor...

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#issuecomment-64916031
  
    @aledsage Thanks for incorporating those comments. Happy to merge this unless you want to get there first.


---
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: Fix various 20141120 (for PortFor...

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/353#issuecomment-64919255
  
    Thanks for great comments @sjcorbett - merging now that jenkins has confirmed ok.


---
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: Fix various 20141120 (for PortFor...

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/353#discussion_r21028327
  
    --- Diff: core/src/main/java/brooklyn/location/access/PortForwardManager.java ---
    @@ -20,87 +20,71 @@
     
     import java.util.Collection;
     
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.basic.ConfigKeys;
     import brooklyn.location.Location;
     
     import com.google.common.annotations.Beta;
     import com.google.common.net.HostAndPort;
     
     /**
    - * Records port mappings against public IP addresses with given identifiers.
    - * <p>
    - * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
    - * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
    - * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
    - * <p>
    - * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
    - * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
    - * provision the forwarding rule.
    - * <p>
    - * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
    - * location and private port it wishes to talk to.
    - * <p>
    + * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
    + * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
    + * via the host, or any other port mapping approach.
    + * 
    + * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
    + * (e.g. for port-mapping with DNAT, then which port to use for the public side).
    + * 
    + * Implementations typically will not know anything about what the firewall/IP actually is, they just 
    + * handle a unique identifier for it.
    + * 
    + * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
    + * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
    + * 
      * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
      * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
      * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
      */
     @Beta
    -public interface PortForwardManager {
    +public interface PortForwardManager extends Location {
     
    -    /**
    -     * Reserves a unique public port on the given publicIpId.
    -     * <p>
    -     * Often followed by {@link #associate(String, int, Location, int)}
    -     * to enable {@link #lookup(Location, int)}.
    -     */
    -    public int acquirePublicPort(String publicIpId);
    +    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
    +            "scope",
    +            "The scope that this applies to, defaulting to global",
    --- End diff --
    
    Adding javadoc:
    
        /**
         * The intention is that there is one PortForwardManager instance per "scope". If you 
         * use global, then it will be a shared instance (for that management context). If you 
         * pass in your own name (e.g. "docker-fjie3") then it will shared with just any other
         * places that use that same location spec (e.g. {@code portForwardManager(scope=docker-fjie3)}).
         */



---
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.
---