Skip to content

Commit

Permalink
- refactored 'queryContainerPortMapping' into 'getPortBindings'
Browse files Browse the repository at this point in the history
- update dynamic ports in the run service
- cleaned up compiler warnings
- allow 'HostIp' to be bound as maven property and exported
- introduced 'PortMapping.Writer' to handle property file writing

Signed-off-by: Jae Gangemi <jgangemi@gmail.com>
  • Loading branch information
jgangemi committed Jul 27, 2015
1 parent bcbd5d9 commit 3c6054d
Show file tree
Hide file tree
Showing 21 changed files with 1,064 additions and 482 deletions.
4 changes: 3 additions & 1 deletion doc/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
- Allow 'build' and/or 'run' configuration to be skipped (#207)
- Refactored to use 'inspect' instead of 'list' for checking the existence of an image (#230)
- Refactored ApacheHttpClientDelegate to avoid leaking connections (#232)

- Introduced global `portPropertyFile` setting (#90)
- Allow the container's host ip to be bound to a maven property and exported

* **0.13.2**
- "run" directives can be added to the Dockerfile (#191)
- Support user information in wait URL (#211)
Expand Down
22 changes: 15 additions & 7 deletions doc/manual.md
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ parentheses.
container logs. This configuration can be overwritten by individual
run configurations and described below. The format is described in
the [section](#log-configuration) below.
* **portPropertyFile** if given, specifies a global file into which the
mapped properties should be written to. The format of this file and
its purpose are also described [below](#port-mapping). Please note, this field takes precidence
over any `portPropertyFile` value specified in an `image` configuration.
* **sourceDirectory** (`docker.source.dir`) specifies the default directory that contains

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

I don't think that this restriction is necessary. One could also write out the the portPropertyFile specified in the image configuration (so that two files are written). Only in the case when there is a name conflict, the 'global' one wins (since it then also contains the image specific properties).

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

that restriction is gone, i think i forgot to update the docs. if you specify a global file and an image file, both get written out.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

I see, just missed it before from PortMapping.Writer

the assembly descriptor(s) used by the plugin. The default value is `src/main/docker`. This
option is only relevant for the `docker:build` goal.
Expand Down Expand Up @@ -622,17 +626,17 @@ equivalent to the port mapping when using the Docker CLI with option
<ports>
<port>18080:8080</port>
<port>host.port:80</port>
<port>host.ip@host.port:80</port>
<ports>
```

A `port` stanza may take one of two forms:
A `port` stanza may take one of four forms:

* A tuple consisting of two numeric values separated by a `:`. This
form will result in an explicit mapping between the docker host and
the corresponding port inside the container. In the above example,
port 18080 would be exposed on the docker host and mapped to port
8080 in the running container.
* A tuple consisting of a string and a numeric value separated by a
* **18080:8080** : A tuple consisting of two numeric values separated by a `:`. This
form will result in an explicit mapping between the docker host and the corresponding
port inside the container. In the above example, port 18080 would be exposed on the
docker host and mapped to port 8080 in the running container.
* **host.port:80** A tuple consisting of a string and a numeric value separated by a
`:`. In this form, the string portion of the tuple will correspond
to a Maven property. If the property is undefined when the `start`
task executes, a port will be dynamically selected by Docker in the
Expand All @@ -647,6 +651,9 @@ A `port` stanza may take one of two forms:
expression similar to `<value>${host.port}</value>`. This can be
used to pin a port from the outside when doing some initial testing
similar to `mvn -Dhost.port=10080 docker:start`
* **host.ip+18080:80** Similar to above except the `host.ip` is mapped and it cannot be set
using a system property.
* **host.ip+host.port:80** Bind `host.ip` and `host.port` to maven properties.

Both forms of the `port` stanza also support binding to a specific ip
address on the docker host.
Expand All @@ -655,6 +662,7 @@ address on the docker host.
<ports>
<port>1.2.3.4:80:80</port>
<port>1.2.3.4:host.port:80</port>
<port>1.2.3.4:host.ip+host.port:80</port>
</ports>

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

Wouldn't in this last example ${host.ip} always set to 1.2.3.4 (so that in this case one could do the assignment also in the <properties> section of the pom) ?

Also, how can the ${host.ip} be different from the docker host whose address is implicitly available always as ${docker.host.address} ?

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

${host.ip} can be very different from ${docker.host.address} if you choose to bind to an ip that is something other then ${docker.host.address}. i do this all the time for a bunch of deployments i have.

and yes, it would always be 1.2.3.4 in the last example, so you could get it from the properties but that potentially means you're writing out yet another props file w/ whatever the host's ip is. so, rather then do that, it can just be included in the same file as the port so now my tests, etc just need to load a single file for this information instead of loading 1 or more files and/or looking in System properties.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

I just checked that:

  • The ${host.ip} is IMO always 0.0.0.0, namely the binding IP you provided during creation of the container.
  • If you provide a binding ip like 192.168.59.103:tomcat.host@tomcat.port:8080 you ${tomcat.host} will be 192.168.59.103 (but that you already know when you specify it that way).

So IMHO the ${host.ip} logic doesn't make much sense and overlaps with the 3-part variant.

What's your use case for the ${host.ip} ?

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

i don't want to have to write another property file and/or look in system properties for the ip address of the container when running tests, etc. instead i just want to load the property file and read the value out of there.

thinking about it now, perhaps as an enhancement if the ip comes back as 0.0.0.0 we replace that w/ whatever ${docker.host.address} is.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

ok, I understand.I have then a next question, though ;-):

We already have three possible parts:

<ip to bind>:<host port>:<container port>
  • ip to bind: Adress or hostname to bind to (literal value)
  • host port: Variable or literal value for the host port: If an unset variable is used, a dynamic port is taken and assigned to the variable. If the variable is already set, then this variable is used.
  • container port: Port within the container.

Instead of extending the host port part, what do you think about this idea: We treat ip to bind the same way has host port: It it is a variable name, this variable is used for the binding adress, if not set, it is filled with the address selected by docker (or ${docker.host.address} if 0.0.0.0). That what would be symettricallty and also support you use case.

What do you think ?

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

i thought about doing something similar but how would you tell the difference between a literal hostname vs a property name? it seems i'd loose the ability to specify a hostname in that case and i definitely don't want to give up that functionality.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

Good point.

But I still would go that route. I see several possibilitities:

  1. Being smart and try to detect whether it is a hostname (is it dns resolvable ? is it a syntactiv valid hostname ?)
  2. Being explicite by using a prefix to indicate a variable (e.g. '+' or '@' . sth which is not valid as a hostname)
  3. Use a different first separator than : (e.g. | or ::)

Personally I like suggestion 3. That would be backward compatible, easy to understand, symetrically.

Your turn :)

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

lol - ok, let's go this route: my use case is i want to be able to define a property name that can hold the bound ip address of the container (and if it's 0.0.0.0 we substitute in docker.host.address) such that it can be written to a property file in the same way the port can be without giving up the ability to specify a hostname and/or ip address that can be resolved/used as the bind address.

i'm open to whatever changes you want to make to achieve this but i'm still not sure how i will be able to define an actual hostname/ip and have that be bound to a property name w/ just a 3 part variant. i still think you need a '4th part'

1.2.3.4:foo.port:5677
hostname.pvt:foo.port:5677
bind.property:foo.port:5677

the ip is an ip, hostname.pvt is a hostname, bind.property is for dynamic substitution. i don't see how you'd be able to tell the difference between hostname,pvt and bind.property and still retain the ability to define what the container should bind to in the first place.

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

p.s, i need to run out for an errand but i'll be back in about 2 hours (not sure what the time difference is for you).

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 10, 2015

1.2.3.4:foo.port:5677
hostname.pvt:foo.port:5677
bind.property:foo.port:5677

My suggestion would be :

1.2.3.4:foo.port:5677
hostname.pvt:foo.port:5677
bind.property|foo.port:5677

or

1.2.3.4:foo.port:5677
hostname.pvt:foo.port:5677
bind.property::foo.port:5677

or any other separator (which can not be part of a property name nor part of an hostname).

An alternative would be the usage with a prefix:

1.2.3.4:foo.port:5677
hostname.pvt:foo.port:5677
+bind.property::foo.port:5677

We can continue tomorrow (I'll be probably off in 2 hours, TZ is CEST).

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 10, 2015

Author Owner

sorry, not trying to be difficult, but i still don't see how this works.

let's take this example:

1.2.3.4:foo.port:5677

even through i and the build know the bind ip should be 1.2.3.4, i have external code that does not and i want it to get the ip address and port from a single file using property names that i define. how do i accomplish that with a 3 part variant?

on some level, i think this discussion is similar to fabric8io#198, it's just a different ip address being exposed. as an alternative, i think that same mechanism could be used to support this instead of changing the 3 part variant, we'd just need to decide how the xml should look.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 11, 2015

Let's take this example:

1.2.3.4:foo.port:5677

even through i and the build know the bind ip should be 1.2.3.4, i have external code that does not and i want it to get the ip address and port from a single file using property names that i define. how do i accomplish that with a 3 part variant?

This case can be covered by using a property and setting its value before:

<properties>
   <host.ip>1.2.3.4</host.ip>
</properties>

and then

+host.ip:foo.port:5677

or

host.ip|foo.port:5677

Yes, a bit more complex but still ok I think. (Today I think the former variant is nicer and easier to implement from our current base. Let's see what I think tomorrow :-)

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 11, 2015

Author Owner

ah - now i understand what you are thinking. i'm going to defer to your judgement on this one - i also think it's a bit more complex to set up, but i'm ok w/ that.

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 11, 2015

Do you want to take that over (if so please pull and merge in '240-global-props' branch first since I did some slight adoptions), but I can work on it, too.

I want to make a release this week (which ends for me on thursday ;-), btw :)

This comment has been minimized.

Copy link
@jgangemi

jgangemi Aug 11, 2015

Author Owner

thursday!? where do i get a work week like that? :)

i'll make the changes - which of the two variants above do you want to use?

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 11, 2015

no, no, only this week ;-) (will be at a music festival over the weekend :). And that's my last holiday till November :)

I think that one with the prefix ('+') is better (and easier to implement since not so much needs to be changed)

This comment has been minimized.

Copy link
@rhuss

rhuss Aug 11, 2015

Thx! Let me know when you are done, I will merge that in asap.

```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ public abstract class AbstractDockerMojo extends AbstractMojo implements Context
/** @parameter property = "docker.registry" */
private String registry;

/**
* @parameter
*/
protected String portPropertyFile;

// Authentication information
/** @parameter */
Map authConfig;
Expand Down
66 changes: 25 additions & 41 deletions src/main/java/org/jolokia/docker/maven/StartMojo.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,29 @@
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/

import java.util.*;
import java.util.ArrayList;
import java.util.Properties;
import java.util.concurrent.TimeoutException;
import java.util.regex.Pattern;

import org.apache.commons.lang3.text.StrSubstitutor;
import org.apache.maven.plugin.MojoExecutionException;
import org.codehaus.plexus.util.StringUtils;
import org.jolokia.docker.maven.access.*;
import org.jolokia.docker.maven.access.DockerAccess;
import org.jolokia.docker.maven.access.DockerAccessException;
import org.jolokia.docker.maven.access.PortMapping;
import org.jolokia.docker.maven.access.log.LogCallback;
import org.jolokia.docker.maven.access.log.LogGetHandle;
import org.jolokia.docker.maven.config.*;
import org.jolokia.docker.maven.config.ImageConfiguration;
import org.jolokia.docker.maven.config.LogConfiguration;
import org.jolokia.docker.maven.config.RunImageConfiguration;
import org.jolokia.docker.maven.config.WaitConfiguration;
import org.jolokia.docker.maven.log.LogDispatcher;
import org.jolokia.docker.maven.service.QueryService;
import org.jolokia.docker.maven.service.RunService;
import org.jolokia.docker.maven.util.*;
import org.jolokia.docker.maven.util.StartOrderResolver;
import org.jolokia.docker.maven.util.Timestamp;
import org.jolokia.docker.maven.util.WaitUtil;


/**
Expand Down Expand Up @@ -51,11 +58,14 @@ public class StartMojo extends AbstractDockerMojo {
public synchronized void executeInternal(final DockerAccess dockerAccess) throws DockerAccessException, MojoExecutionException {
getPluginContext().put(CONTEXT_KEY_START_CALLED, true);

Properties projProperties = project.getProperties();

QueryService queryService = serviceHub.getQueryService();
RunService runService = serviceHub.getRunService();

LogDispatcher dispatcher = getLogDispatcher(dockerAccess);

LogDispatcher dispatcher = getLogDispatcher(dockerAccess);
PortMapping.Writer portMappingWriter = new PortMapping.Writer(portPropertyFile);

boolean success = false;
try {
for (StartOrderResolver.Resolvable resolvable : runService.getImagesConfigsInOrder(queryService, getImages())) {
Expand All @@ -69,47 +79,40 @@ public synchronized void executeInternal(final DockerAccess dockerAccess) throws
getConfiguredRegistry(imageConfig),imageConfig.getBuildConfiguration() == null);

RunImageConfiguration runConfig = imageConfig.getRunConfiguration();
PortMapping portMapping = runService.getPortMapping(runConfig, project.getProperties());
PortMapping portMapping = runService.getPortMapping(runConfig, projProperties);

String containerId = runService.createAndStartContainer(imageConfig, portMapping, project.getProperties());
String containerId = runService.createAndStartContainer(imageConfig, portMapping, projProperties);

if (showLogs(imageConfig)) {
dispatcher.trackContainerLog(containerId, getContainerLogSpec(containerId, imageConfig));
}

// Set maven properties for dynamically assigned ports.
updateDynamicPortProperties(dockerAccess, containerId, runConfig, portMapping, project.getProperties());
portMappingWriter.add(portMapping, runConfig.getPortPropertyFile());

// Wait if requested
waitIfRequested(dockerAccess,imageConfig, project.getProperties(), containerId);
waitIfRequested(dockerAccess,imageConfig, projProperties, containerId);
}
if (follow) {
runService.addShutdownHookForStoppingContainers(keepContainer,removeVolumes);
wait();
}

portMappingWriter.write();
success = true;
} catch (InterruptedException e) {
log.warn("Interrupted");
Thread.currentThread().interrupt();
throw new MojoExecutionException("interrupted", e);
} finally {
if (!success) {
log.error("Error occurred during container startup, shutting down...");
runService.stopStartedContainers(keepContainer, removeVolumes);
}
}
}

private void updateDynamicPortProperties(DockerAccess docker, String containerId, RunImageConfiguration runConfig, PortMapping mappedPorts, Properties properties) throws DockerAccessException, MojoExecutionException {
if (mappedPorts.containsDynamicPorts()) {
mappedPorts.updateVariablesWithDynamicPorts(docker.queryContainerPortMapping(containerId));
propagatePortVariables(mappedPorts, runConfig.getPortPropertyFile(),properties);
}
}


// ========================================================================================================


private void waitIfRequested(DockerAccess docker, ImageConfiguration imageConfig, Properties projectProperties, String containerId) throws MojoExecutionException {
RunImageConfiguration runConfig = imageConfig.getRunConfiguration();
WaitConfiguration wait = runConfig.getWaitConfiguration();
Expand Down Expand Up @@ -176,26 +179,6 @@ public void cleanUp() {
};
}

// Store dynamically mapped ports
private void propagatePortVariables(PortMapping mappedPorts, String portPropertyFile, Properties properties) throws MojoExecutionException {
Properties props = new Properties();
Map<String, Integer> dynamicPorts = mappedPorts.getPortVariables();
for (Map.Entry<String, Integer> entry : dynamicPorts.entrySet()) {
String var = entry.getKey();
String val = "" + entry.getValue();
properties.setProperty(var, val);
props.setProperty(var, val);
}

// However, this can be to late since properties in pom.xml are resolved during the "validate" phase
// (and we are running later probably in "pre-integration" phase. So, in order to bring the dynamically
// assigned ports to the integration tests a properties file is written. Not nice, but works. Blame it
// to maven to not allow late evaluation or any other easy way to inter-plugin communication
if (portPropertyFile != null) {
EnvUtil.writePortProperties(props, portPropertyFile);
}
}

protected boolean showLogs(ImageConfiguration imageConfig) {
if (showLogs != null) {
if (showLogs.equalsIgnoreCase("true")) {
Expand All @@ -219,4 +202,5 @@ protected boolean showLogs(ImageConfiguration imageConfig) {
}
return false;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public ContainerHostConfig links(List<String> links) {
}

public ContainerHostConfig portBindings(PortMapping portMapping) {
Map<String, Integer> portMap = portMapping.getPortsMap();
Map<String, Integer> portMap = portMapping.getContainerPortToHostPortMap();
if (!portMap.isEmpty()) {
JSONObject portBindings = new JSONObject();
Map<String, String> bindToMap = portMapping.getBindToHostMap();
Expand Down
11 changes: 0 additions & 11 deletions src/main/java/org/jolokia/docker/maven/access/DockerAccess.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,6 @@ public interface DockerAccess {
*/
void stopContainer(String containerId) throws DockerAccessException;

/**
* Query the port mappings for a certain container.
*
* @param containerId id of the container to query.
* @return mapped ports where the keys of this map are the container ports including the protocol (e.g. "8080/tcp")
* and the values are the mapped, potentially dynamically chosen, host ports (e.g. 49000).
* The returned map is never null but can be empty.
* @throws DockerAccessException if the query failed.
*/
Map<String, Integer> queryContainerPortMapping(String containerId) throws DockerAccessException;

/**
* Get logs for a container up to now synchronously.
*
Expand Down
Loading

0 comments on commit 3c6054d

Please sign in to comment.