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.

Sign up for GitHub

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

Jump to bottom

GroupPlacementStrategy update and various other fixes #272

Merged
merged 23 commits into from
Apr 20, 2016
Merged

GroupPlacementStrategy update and various other fixes #272

merged 23 commits into from
Apr 20, 2016

Conversation

grkvlt
Copy link
Member

@grkvlt grkvlt commented Apr 1, 2016

@grkvlt
Copy link
Member Author

grkvlt commented Apr 1, 2016

@aledsage @nakomis can you take a look at this please?

@grkvlt grkvlt force-pushed the group-placement-locking branch from 29acc16 to d162648 Compare April 1, 2016 23:15
@grkvlt
Copy link
Member Author

grkvlt commented Apr 4, 2016

We will probably still need to make a private jclouds 1.9.3 build for Clocker to use the new template option properly?

nakomis
nakomis reviewed Apr 5, 2016
View reviewed changes
docker/src/main/java/brooklyn/location/docker/DockerContainerLocation.java

@Override
public int copyTo(final Map<String,?> props, final InputStream src, final String destination) {
File tmp = null;
Copy link

Choose a reason for hiding this comment

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

Why not use super.copyTo() if SSH available?

@nakomis
Copy link

nakomis commented Apr 6, 2016

Would be useful if the patches classes contained a description of what has been changed, and a link to any relevant PR

@nakomis
Copy link

nakomis commented Apr 6, 2016

A couple of minor comments, but other than that, LGTM

@grkvlt
Copy link
Member Author

grkvlt commented Apr 6, 2016

Good call on documenting patches project content...

@grkvlt
Copy link
Member Author

grkvlt commented Apr 6, 2016

Will fix typos and merge...

@grkvlt grkvlt force-pushed the group-placement-locking branch 4 times, most recently from 63f1e38 to 36878e3 Compare April 6, 2016 21:35
@grkvlt
Copy link
Member Author

grkvlt commented Apr 6, 2016

Waiting on review comments from @aledsage before merge. Also squashed and rebased.

@grkvlt grkvlt force-pushed the group-placement-locking branch from 36878e3 to 0db3c00 Compare April 7, 2016 01:46
@grkvlt grkvlt added this to the Version 1.1.0 milestone Apr 7, 2016
@grkvlt grkvlt self-assigned this Apr 7, 2016
@grkvlt grkvlt force-pushed the group-placement-locking branch 8 times, most recently from 6967dae to ccce467 Compare April 12, 2016 19:56
aledsage
aledsage reviewed Apr 16, 2016
View reviewed changes
...main/java/clocker/docker/entity/container/application/VanillaDockerApplicationSshDriver.java
Map<String, String> runtimeTemplates = entity.getConfig(VanillaSoftwareProcess.RUNTIME_TEMPLATES);
if ((runtimeFiles != null && runtimeFiles.size() > 0) ||
(runtimeTemplates != null && runtimeTemplates.size() > 0)) {
super.copyRuntimeResources();
Copy link
Member

Choose a reason for hiding this comment

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

Should we change Brooklyn's AbstractSoftwareProcessDriver, so that it only does the createDirectory(getRunDir(), "create run directory") if there are runtime resources to copy? (I presume nothing downstream in normal Brooklyn entities relies on runDir existing because of copyRuntimeResources, but we'd need to check that).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should, but I didn't want to have to wait to get the change into Brooklyn.

Copy link
Member

Choose a reason for hiding this comment

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

+1; can you raise a PR in Brooklyn for that at some point soon'ish please.

@aledsage
Copy link
Member

Thanks @grkvlt - looks good. I've gone through the code (some in detail, other bits just skimming). I've included some comments but nothing major. Over to you to see what you think of the comments and then to merge.

This was referenced Apr 17, 2016
Closed
Closed
grkvlt added 2 commits April 18, 2016 16:36
@grkvlt
Various fixes
0f3f98e 
- Remove Softlayer VLAN code from DockerHost
- Set hostname on SoftLayer properly
- Fix issue with /dev/urandom
- Setup calico endpoints with IP
- Update enrichers and port sensors for entities
@grkvlt
Updated docker daemon startup arguments
839bdb7
@grkvlt grkvlt force-pushed the group-placement-locking branch 3 times, most recently from afddceb to 8c8ea85 Compare April 19, 2016 19:20
grkvlt and others added 6 commits April 20, 2016 00:29
@grkvlt
Use Docker 1.10.3
049eca5
@grkvlt
Fix for SoftLayer hostname issue
f16ae3b
@johnmccabe @grkvlt
set container links with a map of alias string to target entity
d1d936e 
 - avoids having to set the `containerName` on the target which means you can deploy multiple instances of linked container groups
 - allows a container to be linked to multiple containers

This is currently a breaking change for anyone using the old list of entities links config.

So the following old yaml (showing only the links related config here):
```
services:
- type: docker:mariadb:5.5
  id: db
  containerName: mysql
- type: docker:wordpress:4.5
  links:
  - $brooklyn:component("db")
```

Is replaced with the following, note the containerName in the db container (which was being used as the alias), has been replaced with an explicitly defined alias in the links map.
```
services:
- type: docker:mariadb:5.5
  id: db
- type: docker:wordpress:4.5
  links:
    mysql: $brooklyn:component("db")
```
@johnmccabe @grkvlt
add environment variables from the source container to the linked target
a71ca79 
 - per https://docs.docker.com/engine/userguide/networking/default_network/dockerlinks/#environment-variables
@johnmccabe @grkvlt
use local subnet address for linked containers
8e3cc15 
- using docker outside of clocker to link containers results in the linked host being added using the local subnet address rather than the docker hosts address, this change aims to reproduce that behaviour

For example, linking a db with the alias `mysql` results in the following `/etc/hosts`: currently (where 169.45.xxx.xxx is the dockerhost address):
```
169.45.xxx.xxx   mysql
172.17.0.30      db_rjsof0ci
172.17.0.30      db_rjsof0ci.bridge
```
Linked containers usually expect the alias to be using the private addresses, ie so they'd have access to the unmapped service ports.
@grkvlt
Fix Calico issue with connectivity
f0a5cf7
@grkvlt grkvlt force-pushed the group-placement-locking branch 2 times, most recently from f4fe246 to f63672a Compare April 20, 2016 00:04
@grkvlt grkvlt force-pushed the group-placement-locking branch from f63672a to 24679f6 Compare April 20, 2016 00:32
@grkvlt
Copy link
Member Author

grkvlt commented Apr 20, 2016

Tested on various clouds successfully by @aledsage @johnmccabe and @grkvlt

@grkvlt grkvlt merged commit c13ed22 into master Apr 20, 2016
@grkvlt grkvlt deleted the group-placement-locking branch April 20, 2016 08:32
@grkvlt grkvlt mentioned this pull request Apr 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@grkvlt grkvlt

Labels
feature important
Projects
None yet
Milestone
Version 1.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@grkvlt @nakomis @johnmccabe @aledsage