You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrea Turli <no...@github.com> on 2014/03/21 13:36:02 UTC
[jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
You can merge this Pull Request by running:
git pull https://github.com/andreaturli/jclouds-labs master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/57
-- Commit Summary --
* [JCLOUDS-500] Initial commit for docker
-- File Changes --
A docker/README.md (51)
A docker/pom.xml (134)
A docker/src/main/java/org/jclouds/docker/DockerApi.java (38)
A docker/src/main/java/org/jclouds/docker/DockerApiMetadata.java (100)
A docker/src/main/java/org/jclouds/docker/binders/BindInputStreamToRequest.java (63)
A docker/src/main/java/org/jclouds/docker/compute/config/DockerComputeServiceContextModule.java (64)
A docker/src/main/java/org/jclouds/docker/compute/extensions/DockerImageExtension.java (117)
A docker/src/main/java/org/jclouds/docker/compute/features/RemoteApi.java (245)
A docker/src/main/java/org/jclouds/docker/compute/features/internal/Archives.java (97)
A docker/src/main/java/org/jclouds/docker/compute/functions/ContainerToNodeMetadata.java (116)
A docker/src/main/java/org/jclouds/docker/compute/functions/ImageToImage.java (99)
A docker/src/main/java/org/jclouds/docker/compute/strategy/DockerComputeServiceAdapter.java (213)
A docker/src/main/java/org/jclouds/docker/config/DockerHttpApiModule.java (42)
A docker/src/main/java/org/jclouds/docker/config/DockerParserModule.java (74)
A docker/src/main/java/org/jclouds/docker/config/DockerProperties.java (29)
A docker/src/main/java/org/jclouds/docker/domain/Config.java (336)
A docker/src/main/java/org/jclouds/docker/domain/Container.java (331)
A docker/src/main/java/org/jclouds/docker/domain/ExposedPorts.java (82)
A docker/src/main/java/org/jclouds/docker/domain/HostConfig.java (152)
A docker/src/main/java/org/jclouds/docker/domain/Image.java (161)
A docker/src/main/java/org/jclouds/docker/domain/NetworkSettings.java (94)
A docker/src/main/java/org/jclouds/docker/domain/Port.java (67)
A docker/src/main/java/org/jclouds/docker/domain/PortBindings.java (24)
A docker/src/main/java/org/jclouds/docker/domain/PortSpecs.java (23)
A docker/src/main/java/org/jclouds/docker/domain/State.java (75)
A docker/src/main/java/org/jclouds/docker/domain/Version.java (165)
A docker/src/main/java/org/jclouds/docker/domain/Volumes.java (20)
A docker/src/main/java/org/jclouds/docker/handlers/DockerErrorHandler.java (108)
A docker/src/main/java/org/jclouds/docker/options/PaginationOptions.java (116)
A docker/src/main/resources/META-INF/services/org.jclouds.apis.ApiMetadata (1)
A docker/src/main/resources/centos/Dockerfile (29)
A docker/src/main/resources/ubuntu/Dockerfile (30)
A docker/src/test/java/org/jclouds/docker/DockerApiMetadataTest.java (49)
A docker/src/test/java/org/jclouds/docker/compute/BaseDockerApiLiveTest.java (96)
A docker/src/test/java/org/jclouds/docker/compute/DockerComputeServiceAdapterLiveTest.java (135)
A docker/src/test/java/org/jclouds/docker/compute/DockerExperimentLiveTest.java (135)
A docker/src/test/java/org/jclouds/docker/compute/extensions/DockerImageExtensionLiveTest.java (56)
A docker/src/test/java/org/jclouds/docker/compute/features/RemoteApiLiveTest.java (133)
A docker/src/test/resources/logback.xml (34)
M pom.xml (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/57.patch
https://github.com/jclouds/jclouds-labs/pull/57.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
> + checkArgument(baseDir.isDirectory(), "%s is not a directory", baseDir);
> + File[] files = baseDir.listFiles();
> + File tarFile = new File(archivePath);
> +
> + String token = getLast(Splitter.on("/").split(archivePath.substring(0, archivePath.lastIndexOf("/"))));
> +
> + byte[] buf = new byte[1024];
> + int len;
> + TarArchiveOutputStream tos = new TarArchiveOutputStream(new FileOutputStream(tarFile));
> + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
> + for (File file : files) {
> + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
> + tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
> + tos.putArchiveEntry(tarEntry);
> + if (!file.isDirectory()) {
> + FileInputStream fin = new FileInputStream(file);
Call ```Files.asByteSource(file).copyTo(tos)``` instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936904
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1244](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1244/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47683314
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @Named("container:inspect")
> + @GET
> + @Path("/containers/{id}/json")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + Container inspectContainer(@PathParam("id") String containerId);
> +
> + /**
> + * Delete the container.
> + *
> + * @return the container.
> + */
> + @Named("container:delete")
> + @DELETE
> + @Path("/containers/{id}")
> + void removeContainer(@PathParam("id") String containerId, @QueryParam("v") boolean v);
What is this `v` parameter? Can we use a more descriptive name and document it in the javadoc?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838747
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + * 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 org.jclouds.docker;
> +
> +import org.jclouds.docker.features.RemoteApi;
> +import org.jclouds.rest.annotations.Delegate;
> +
> +import java.io.Closeable;
> +
> +/**
> + * Provides synchronous access to Docker Remote API.
> + *
> + * @see <a href="http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8"></a>
The link points to version 1.8 but the api metadata uses version 1.11. Change all links accordingly or just point to a root location that does not contain the version in the URL?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14173962
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +/**
> + * @author Andrea Turli
> + */
> +public class ImageToImage implements Function<org.jclouds.docker.domain.Image, org.jclouds.compute.domain.Image> {
> +
> + private static final String CENTOS = "centos";
> + private static final String UBUNTU = "ubuntu";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @Override
> + public Image apply(org.jclouds.docker.domain.Image from) {
> + checkNotNull(from, "image");
> + String description = checkNotNull(Iterables.getFirst(from.getRepoTags(), null));
Add a message to the null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937048
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Imesh Guaratne <no...@github.com>.
@andreaturli I just tried to build this pr but I get the following error: Any thoughts?
jclouds-labs/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndInstallVm.java:[109,29] cannot find symbol
symbol : constructor Stopwatch()
location: class com.google.common.base.Stopwatch
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-44984152
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + return result;
> + }
> +
> + protected Payload createPayload() throws IOException {
> + String folderPath = System.getProperty("user.dir") + "/docker/src/test/resources";
> + File parentDir = new File(folderPath + "/archive");
> + parentDir.mkdirs();
> + URL url = Resources.getResource("Dockerfile");
> + String content = Resources.toString(url, Charsets.UTF_8);
> + final File dockerfile = new File(parentDir.getAbsolutePath() + File.separator + "Dockerfile");
> + Files.write(content.getBytes(), dockerfile);
> + File archive = Archives.tar(parentDir, folderPath + "/archive/archive.tar");
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
Same as commented above. Is this the right media type? Note that it doesn't match the ones defined in the `RemoteApi.build` methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937709
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
> Not sure about the latest 2 checkstyle violations: any hint?
Ah, yes...that one. The warning is not very helpful, I'm afraid :-( I think it's "missing newline at end of file".
The place to verify that, by the way, is in the [Checkstyle result file in the workspace](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/ws/docker/target/checkstyle-result.xml).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40754523
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }, null);
> + }
> +
> + @Override
> + public Iterable<Container> listNodes() {
> + Set<Container> containers = Sets.newHashSet();
> + for (Container container : api.getRemoteApi().listContainers()) {
> + // less efficient than just listNodes but returns richer json
> + containers.add(api.getRemoteApi().inspectContainer(container.getId()));
> + }
> + return containers;
> + }
> +
> + @Override
> + public Iterable<Container> listNodesByIds(final Iterable<String> ids) {
> + return filter(listNodes(), new Predicate<Container>() {
This will generate a call to inspect *every* container, when you're only asking for a subset. Remove this and directly call `inspectContainer` on the provided ids.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937268
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + ", path='" + path + '\'' +
> + ", args=" + Arrays.toString(args) +
> + ", config=" + config +
> + ", state=" + state +
> + ", image='" + image + '\'' +
> + ", networkSettings=" + networkSettings +
> + ", sysInitPath='" + sysInitPath + '\'' +
> + ", resolvConfPath='" + resolvConfPath + '\'' +
> + ", volumes=" + volumes +
> + ", sizeRw=" + sizeRw +
> + ", sizeRootFs=" + sizeRootFs +
> + ", command='" + command + '\'' +
> + ", status='" + status + '\'' +
> + ", hostConfig=" + hostConfig +
> + ", ports=" + ports +
> + '}';
Also use the `Objects.toStringHelper` as in the Config class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10840012
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + NodeMetadataBuilder nodeMetadataBuilder = new NodeMetadataBuilder();
> + nodeMetadataBuilder.id(container.getId())
> + .name(name)
> + .group(group);
> + // TODO Set up location properly
> + LocationBuilder locationBuilder = new LocationBuilder();
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + nodeMetadataBuilder.location(locationBuilder.build());
> + // TODO setup hardware and hostname properly
> + if (container.getStatus() != null) {
> + nodeMetadataBuilder.status(container.getStatus().contains("Up") ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + } else {
> + nodeMetadataBuilder.status(container.getState().isRunning() ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + }
Refactor this to a reusable (injectable) function that transforms a Docker state to a jclouds node Status?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839026
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #138](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/138/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40698125
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
@imesh @andreaturli It looks like an explicit dependency on Guava might be required?
```XML
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>17.0</version>
</dependency>
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45006489
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + BufferedInputStream in = new BufferedInputStream(fin);
> + while ((len = in.read(buf)) != -1) {
> + tos.write(buf, 0, len);
> + }
> + in.close();
> + }
> + tos.closeArchiveEntry();
> + }
> + tos.close();
> + return tarFile;
> + }
> +
> + public static File tar(File baseDir, File tarFile) throws IOException {
> + // Check that the directory is a directory, and get its contents
> + if (!baseDir.isDirectory())
> + throw new IllegalArgumentException("Compress: not a directory: " + baseDir);
Same thing, use `checkArgument`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838930
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> +@Singleton
> +public class DockerComputeServiceAdapter implements
> + ComputeServiceAdapter<Container, Hardware, Image, Location> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final DockerApi api;
> + private final ApiContext<DockerApi> context;
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api, ApiContext<DockerApi> context) {
> + this.api = checkNotNull(api, "api");
> + this.context = context;
Also check not null here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839229
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
@imesh this provider is still at its early stage but it covers all of the "CRUD" operations offerred by remote api for image management and container management.
How do you plan to contribute?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40273258
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.logging.Logger;
> +import org.jclouds.rest.ApiContext;
> +
> +import javax.annotation.Resource;
> +import javax.inject.Inject;
> +import javax.inject.Named;
> +import java.net.URI;
> +import java.util.List;
> +import java.util.Map;
> +
> +import static com.google.common.collect.Iterables.getOnlyElement;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class ContainerToNodeMetadata implements Function<Container, NodeMetadata> {
Can this function be singleton?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838952
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
> +
> + if (templateOptions.getCommands().isPresent()) {
> + containerConfigBuilder.cmd(templateOptions.getCommands().get());
> + }
> +
> + if (templateOptions.getMemory().isPresent()) {
> + containerConfigBuilder.memory(templateOptions.getMemory().get());
> + }
> +
> + if (templateOptions.getCpuShares().isPresent()) {
> + containerConfigBuilder.cpuShares(templateOptions.getCpuShares().get());
> + }
> +
> + if (templateOptions.getVolumes().isPresent()) {
> + Map<String, Object> volumes = Maps.newLinkedHashMap();
> + for (String containerDir : templateOptions.getVolumes().get().values()) {
This isn't right. You're throwing away the host directory portion. We have two use cases for volumes, one is mounting a host directory in a specific location on a container, the other is creating a volume in a specific location on a container for export to another container. This will only cover the second case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14484014
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Imesh Guaratne <no...@github.com>.
@andreaturli
I found several problems while trying to run a docker container (apache2) and sent PR to your forked branch: https://github.com/andreaturli/jclouds-labs/pull/2 WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45140914
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
> + for (File file : files) {
> + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
> + tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
> + tos.putArchiveEntry(tarEntry);
> + if (!file.isDirectory()) {
> + FileInputStream fin = new FileInputStream(file);
> + BufferedInputStream in = new BufferedInputStream(fin);
> + while ((len = in.read(buf)) != -1) {
> + tos.write(buf, 0, len);
> + }
> + in.close();
> + }
> + tos.closeArchiveEntry();
> + }
> + tos.close();
Close this in a finally block?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936813
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + }
> +
> + private int getLoginPort(Container container) {
> + if (container.getNetworkSettings() != null) {
> + Map<String, List<Map<String,String>>> ports = container.getNetworkSettings().getPorts();
> + if(ports != null) {
> + return Integer.parseInt(getOnlyElement(ports.get("22/tcp")).get("HostPort"));
> + }
> + } else if (container.getPorts() != null) {
> + for (Port port : container.getPorts()) {
> + if (port.getPrivatePort() == 22) {
> + return port.getPublicPort();
> + }
> + }
> + }
> + throw new IllegalStateException("Cannot determine the login port for " + container.getId());
I think so as all the container will have a mapper port also for 22. In fact it is very much unlikely that a container can get port 22 on the host.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10845029
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @QueryParam("limit") String limit,
> + @QueryParam("since") String since,
> + @QueryParam("before") String before);
> +
> + /**
> + * List containers
> + *
> + * @return the running containers.
> + */
> + @Named("containers:list")
> + @GET
> + @Path("/containers/json")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + List<Container> listContainers(
> + @QueryParam("all") boolean all);
Following the conventions in other providers, can we have a method `listContainers()` and a `listContainers(ListContainerOptions options)` to avoid forcing users to always provide the query params?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838673
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testVersion() {
> + Assert.assertEquals(api().getVersion().getVersion(), "0.9.0");
> + }
> +
> + @Test(dependsOnMethods = "testVersion")
> + public void testCreateImage() throws IOException, InterruptedException {
> + CreateImageOptions options = CreateImageOptions.Builder.fromImage(BUSYBOX_IMAGE);
> + InputStream createImageStream = api().createImage(options);
> + consumeStream(createImageStream, false);
> + image = api().inspectImage(BUSYBOX_IMAGE);
> + assertNotNull(image);
> + }
> +
> + @Test(dependsOnMethods = "testCreateImage")
> + public void testListImages() {
> + Assert.assertNotNull(api().listImages());
Use the static import here too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937792
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.Binder;
> +
> +import com.google.common.base.Throwables;
> +import com.google.common.io.Files;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class BindInputStreamToRequest implements Binder {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @SuppressWarnings("unchecked")
This is unnecessary, you can remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936630
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #204](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/204/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47683209
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs #886 FAILURE
GitHub [timeout](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/886/console)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38275658
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for the contribution @andreaturli! I look forward to have this merged!
Next steps should be:
**General**
- [ ] Address all review comments.
- [ ] Make all constructors for the domain objects that have a builder `protected`, to enforce the use of the builder.
- [ ] Run `mvn checkstyle:checkstyle` and address all violations.
**Tests**
- [ ] Add unit tests for the `BindInputStreamToRequest` class.
- [ ] Add the `DockerImageExtensionLiveTest` that extends `BaseImageExtensionLiveTest`.
- [ ] Can the `Archives` class be unit tested in some way?
- [ ] Add tests for the `DockerTemplateOptions` class.
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43858160
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api) {
> + this.api = checkNotNull(api, "api");
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> + DockerTemplateOptions templateOptions = DockerTemplateOptions.class.cast(template.getOptions());
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String loginUser = template.getImage().getDefaultCredentials().getUser();
> + String loginUserPassword = template.getImage().getDefaultCredentials().getPassword();
Where are the credentials set? The `ImageToImage` does not set them. Is this going to throw a NPE?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937125
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.io.File;
> +import java.io.FileInputStream;
> +import java.io.FileOutputStream;
> +import java.io.IOException;
> +
> +import static com.google.common.base.Preconditions.checkArgument;
> +import static com.google.common.collect.Iterables.getLast;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class Archives {
> +
> + public static File tar(File baseDir, String archivePath) throws IOException {
> + // Check that the directory is a directory, and get its contents
> + checkArgument(baseDir.isDirectory(), baseDir + " is not a directory");
Use better the form `checkArgument(baseDir.isDirectory(), "%s is not a directory", baseDir)`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265644
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1258](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1258/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47839929
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Imesh Guaratne <no...@github.com>.
@andreaturli Thanks for the quick response! That's great!
Let me give you some background on this, I'm a committer in Apache Stratos (incubating) project and Stratos is using jclouds for IaaS communication. We are now looking into supporting Docker in Stratos via jclouds.
May be first I could checkout your pull request and try it out. I will update you after that.
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40281850
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + * 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 org.jclouds.docker;
> +
> +import org.jclouds.docker.compute.features.RemoteApi;
> +import org.jclouds.rest.annotations.Delegate;
> +
> +import java.io.Closeable;
> +
> +/**
> + * Provides synchronous access to Docker.
> + *
> + * @author Andrea Turli
> + * @see <a href="http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8"></a>
Add a text to the link?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838426
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +##Components
> +
> +- jclouds \- acts as a java client to access to docker features
> +- DOCKER_HOST \- hosts Docker API, NB: jclouds-docker assumes that the latest Docker is installed
> +- Image \- it is a docker image that can be started.
> +- Node \- is a docker container
> +
> +## Assumptions
> +
> +- jclouds-docker assumes that the images specified using the template are ssh'able.
> +- jclouds-docker will mount ${user.home} to /root of the container
> +
> +--------------
> +
> +#Notes:
> +- jclouds-docker is still at alpha stage please report any issues you find at [jclouds issues](https://github.com/jclouds/jclouds/issues?state=open)
Change the link to point to the jclouds JIRA
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936596
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @SerializedName("Binds")
> + private final List<String> binds;
> + @SerializedName("Privileged")
> + private final boolean privileged;
> + @SerializedName("PortBindings")
> + private final Map<String, List<Map<String, String>>> portBindings;
> + @SerializedName("Links")
> + private final List<String> links;
> + @SerializedName("PublishAllPorts")
> + private final boolean publishAllPorts;
> +
> + @ConstructorProperties({ "ContainerIDFile", "Binds", "Privileged", "PortBindings", "Links", "Size",
> + "PublishAllPorts" })
> + public HostConfig(@Nullable String containerIDFile, @Nullable List<String> binds, @Nullable boolean privileged,
> + @Nullable Map<String, List<Map<String, String>>> portBindings, @Nullable List<String> links,
> + @Nullable boolean publishAllPorts) {
Can a `HostConfig` object be actually empty, with all properties being null?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937535
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1150](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1150/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45648657
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + }
> +
> + @Override
> + public Iterable<Location> listLocations() {
> + return ImmutableSet.of();
> + }
> +
> + @Override
> + public Container getNode(String id) {
> + return api.getRemoteApi().inspectContainer(id);
> + }
> +
> + @Override
> + public void destroyNode(String id) {
> + api.getRemoteApi().stopContainer(id);
> + api.getRemoteApi().removeContainer(id);
removeContainer supports `force` that removes the container even if it was running. Default false.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13232276
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.user = user;
> + this.memory = checkNotNull(memory, "memory");
> + this.memorySwap = checkNotNull(memorySwap, "memorySwap");
> + this.cpuShares = checkNotNull(cpuShares, "cpuShares");
> + this.attachStdin = checkNotNull(attachStdin, "attachStdin");
> + this.attachStdout = checkNotNull(attachStdout, "attachStdout");
> + this.attachStderr = checkNotNull(attachStderr, "attachStderr");
> + this.exposedPorts = exposedPorts != null ? ImmutableMap.copyOf(exposedPorts) : ImmutableMap.<String, Object> of();
> + this.tty = checkNotNull(tty, "tty");
> + this.openStdin = checkNotNull(openStdin, "openStdin");
> + this.stdinOnce = checkNotNull(stdinOnce, "stdinOnce");
> + this.env = env != null ? ImmutableList.copyOf(cmd) : ImmutableList.<String> of();
> + this.cmd = cmd != null ? ImmutableList.copyOf(cmd) : ImmutableList.<String> of();
> + this.dns = dns != null ? ImmutableList.copyOf(dns) : ImmutableList.<String> of();
> + this.imageId = checkNotNull(imageId, "imageId");
> + this.volumes = volumes != null ? ImmutableMap.copyOf(volumes) : ImmutableMap.<String, Object> of();
`volumes` is not nullable. Add a null check or annotate it properly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937403
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + checkArgument(checkNotNull(input, "input") instanceof File, "this binder is only valid for File!");
> + checkNotNull(request, "request");
> +
> + File dockerFile = (File) input;
> + File tmpDir = Files.createTempDir();
> + final File targetFile = new File(tmpDir + File.separator + "Dockerfile");
> + try {
> + Files.copy(dockerFile, targetFile);
> + File archive = Archives.tar(tmpDir, File.createTempFile("archive", ".tar"));
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
> + request.setPayload(payload);
> + } catch (IOException e) {
> + e.printStackTrace();
Properly log and propagate the exception to prevent the request from being sent?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838367
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableMap;
> +
> +/**
> + * Contains options supported in the {@code ComputeService#runNode} operation on the
> + * "docker" provider. <h2>Usage</h2> The recommended way to instantiate a
> + * DockerTemplateOptions object is to statically import DockerTemplateOptions.* and invoke a static
> + * creation method followed by an instance mutator (if needed):
> + * <p/>
> + * <code>
> + * import static org.jclouds.aws.ec2.compute.options.DockerTemplateOptions.Builder.*;
> + * <p/>
> + * ComputeService api = // get connection
> + * templateBuilder.options(inboundPorts(22, 80, 8080, 443));
It would be great to have the example show the usage of the Docker specific options.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937085
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Override
> + public Iterable<Location> listLocations() {
> + return ImmutableSet.of();
> + }
> +
> + @Override
> + public Container getNode(String id) {
> + return api.getRemoteApi().inspectContainer(id);
> + }
> +
> + @Override
> + public void destroyNode(String id) {
> + api.getRemoteApi().stopContainer(id);
> + api.getRemoteApi().removeContainer(id);
Then we should try it and, if possible, just remove the stopContainer call or move the remove one to a finally block.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954881
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + */
> + @Named("images:list")
> + @GET
> + @Path("/images/json")
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<Image> listImages(ListImageOptions options);
> +
> + /**
> + * Inspect an image
> + *
> + * @param imageId The id of the image to inspect.
> + * @return low-level information on the image name
> + */
> + @Named("image:inspect")
> + @GET
> + @Path("/images/{name}/json")
it's a strange api, it accepts `name` or `id` ... so I will stick with name
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952760
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof File, "this binder is only valid for File!");
> + checkNotNull(request, "request");
> +
> + File dockerFile = (File) input;
> + File tmpDir = Files.createTempDir();
> + final File targetFile = new File(tmpDir + File.separator + "Dockerfile");
> + try {
> + Files.copy(dockerFile, targetFile);
> + File archive = Archives.tar(tmpDir, File.createTempFile("archive", ".tar"));
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
Worth a try :) If it is supported, I'd change it to `application/tar`, otherwise I'll leave it as-is, but change the headers in the [RemoteApi#build](https://github.com/andreaturli/jclouds-labs/blob/master/docker/src/main/java/org/jclouds/docker/compute/features/RemoteApi.java#L259-L272) methods accordingly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952833
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
> + }
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/build");
> + } finally {
> + dockerFile.deleteOnExit();
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + private File createDockerFile(String content) {
> + File newTempDir = Files.createTempDir();
> + File dockerFile = new File(newTempDir + "/dockerFile");
> + try {
> + Files.write(content, dockerFile, Charsets.UTF_8);
> + } catch(IOException e) {
> + Assert.fail();
Propagate ```IOException```?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938577
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(Constants.PROPERTY_MAX_RETRIES, "15");
> + properties.setProperty("jclouds.ssh.retry-auth", "true");
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
> + properties.setProperty(TEMPLATE, "osFamily=UBUNTU,os64Bit=true,osVersionMatches=1[012].[01][04]");
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<DockerApi, Builder> {
> +
> + protected Builder() {
> + super(DockerApi.class);
> + id("docker")
> + .name("Docker API")
> + .defaultIdentity("root")
> + .identityName("user")
capitalize and change by a text describing what the identity is?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838240
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #974](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/974/)?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email#issuecomment-40697780
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
@andreaturli there's a typo in `Config` but otherwise this looks good
@demobox any idea when this can be merged?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43833767
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #175](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/175/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45648690
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #174](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/174/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45491046
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.created = created;
> + this.path = path;
> + this.args = args;
> + this.config = config;
> + this.state = state;
> + this.image = image;
> + this.networkSettings = networkSettings;
> + this.sysInitPath = sysInitPath;
> + this.resolvConfPath = resolvConfPath;
> + this.volumes = volumes;
> + this.sizeRw = sizeRw;
> + this.sizeRootFs = sizeRootFs;
> + this.command = command;
> + this.status = status;
> + this.hostConfig = hostConfig;
> + this.ports = ports;
Same, null checks...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839975
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Override
> + public Iterable<Hardware> listHardwareProfiles() {
> + Set<Hardware> hardware = Sets.newLinkedHashSet();
> + // todo they are only placeholders at the moment
> + hardware.add(new HardwareBuilder().ids("micro").hypervisor("lxc").name("micro").ram(512).build());
> + hardware.add(new HardwareBuilder().ids("small").hypervisor("lxc").name("small").ram(1024).build());
> + hardware.add(new HardwareBuilder().ids("medium").hypervisor("lxc").name("medium").ram(2048).build());
> + hardware.add(new HardwareBuilder().ids("large").hypervisor("lxc").name("large").ram(3072).build());
> + return hardware;
> + }
> +
> + @Override
> + public Set<Image> listImages() {
> + //return api.getRemoteApi().listImages();
Remove this line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937230
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #202](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/202/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47677495
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + destroyNode(container.getId());
> + throw new IllegalStateException(String.format("Container %s has not started correctly", container.getId()));
> + }
> + return new NodeAndInitialCredentials<Container>(container, container.getId(),
> + LoginCredentials.builder().user(loginUser).password(loginUserPassword).build());
> + }
> +
> + @Override
> + public Iterable<Hardware> listHardwareProfiles() {
> + Set<Hardware> hardware = Sets.newLinkedHashSet();
> + // todo they are only placeholders at the moment
> + hardware.add(new HardwareBuilder().ids("micro").hypervisor("lxc").name("micro").ram(512).build());
> + hardware.add(new HardwareBuilder().ids("small").hypervisor("lxc").name("small").ram(1024).build());
> + hardware.add(new HardwareBuilder().ids("medium").hypervisor("lxc").name("medium").ram(2048).build());
> + hardware.add(new HardwareBuilder().ids("large").hypervisor("lxc").name("large").ram(3072).build());
> + return hardware;
The list of hardware profiles should also include the cpu. I see two things to improve here:
* Provide more cpu/ram combinations (otherwise is not enough flexible), such as providers like CloudSigma do (those also have hardcoded hardware profiles).
* Create a Supplier<Hardware>, qualified with something like `@Default` to avoid collisions with the jclouds hardware supplier, to let people bind their own hardware profiles if they need different ones.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937224
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.base.Preconditions.checkState;
> +import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_IMAGE_AVAILABLE;
> +
> +/**
> + * Docker implementation of {@link org.jclouds.compute.extensions.ImageExtension}
> + *
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class DockerImageExtension implements ImageExtension {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + private Logger logger = Logger.NULL;
> + private DockerApi api;
Declare this as `final`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838447
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-labs-pull-requests #140 FAILURE
Some internal DEV@cloud failure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40758454
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + NodeMetadataBuilder builder = new NodeMetadataBuilder();
> + builder.id(container.getId())
> + .name(name).group(group)
> + .hostname(container.getConfig().getHostname());
> + // TODO Set up location properly and hardware
> + LocationBuilder locationBuilder = new LocationBuilder();
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + builder.location(locationBuilder.build());
> + builder.status(toPortableStatus.apply(container.getState()));
> + builder.imageId(container.getImage());
> + builder.loginPort(getLoginPort(container));
> + builder.publicAddresses(getPublicIpAddresses());
> + builder.privateAddresses(getPrivateIpAddresses(container));
> + builder.operatingSystem(OperatingSystem.builder().description("unrecognized").family(OsFamily.UNRECOGNIZED).build());
Can't guess the OS?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11266304
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.openStdin = openStdin;
> + return this;
> + }
> +
> + public Builder stdinOnce(boolean stdinOnce) {
> + this.stdinOnce = stdinOnce;
> + return this;
> + }
> +
> + public Builder env(List<String> env) {
> + this.env = env;
> + return this;
> + }
> +
> + public Builder cmd(List<String> cmd) {
> + this.cmd = ImmutableList.builder();
Why overriding it here? If you don't really want to add, don't initialize the variable when declaring it and use `copyOf` here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839864
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setBody(payloadFromResource("/container-creation.json")));
> +
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + Config config = Config.builder().cmd(ImmutableList.of("date"))
> + .attachStdin(false)
> + .attachStderr(true)
> + .attachStdout(true)
> + .tty(false)
> + .imageId("base")
> + .build();
> + try {
> + Container container = remoteApi.createContainer("test", config);
> + assertNotNull(container);
> + assertEquals(container.getId(), "c6c74153ae4b1d1633d68890a68d89c40aa5e284a1ea016cbc6ef0e634ee37b2");
Also check the recorded request.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938136
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #896](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/896/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38605999
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1243](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1243/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47680874
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #203](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/203/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47680678
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @Named("image:delete")
> + @DELETE
> + @Path("/images/{name}")
> + @Consumes(MediaType.APPLICATION_JSON)
> + InputStream deleteImage(@PathParam("name") String name);
> +
> +
> + /**
> + * Build an image from Dockerfile via stdin
> + */
> + @Named("image:build")
> + @POST
> + @Path("/build")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Headers(keys = "Content-Type", values = "application/tar")
> + InputStream build(@QueryParam("t") String tag, Payload inputStream);
Should this method also support the `quiet` and `nocache` parameters?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838881
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -0,0 +1,275 @@
> +/*
The entire `features` package must be moved outside the `compute` one.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936907
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + .hardware(new HardwareBuilder()
> + .id("")
> + .ram(container.getContainerConfig().getMemory())
> + .processor(new Processor(container.getContainerConfig().getCpuShares(), container.getContainerConfig().getCpuShares()))
> + .build());
> + builder.status(toPortableStatus.apply(container.getState()));
> + builder.imageId(container.getImage());
> + builder.loginPort(getLoginPort(container));
> + builder.publicAddresses(getPublicIpAddresses());
> + builder.privateAddresses(getPrivateIpAddresses(container));
> +
> + Image image = images.get().get(container.getImage());
> + builder.imageId(image.getId());
> + builder.operatingSystem(image.getOperatingSystem());
> +
> + return builder.build();
Is there a way we can set the credentials here? Even if they are the default `root:password`? They may come in handy when getting a node (that has not been created by jclouds) using the ComputeService.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174259
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
Please run through Checkstyle.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43858507
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @SerializedName("ResolvConfPath")
> + private String resolvConfPath;
> + @SerializedName("Volumes")
> + private Volumes volumes;
> + @SerializedName("SizeRw")
> + private long sizeRw;
> + @SerializedName("SizeRootFs")
> + private long sizeRootFs;
> + @SerializedName("Command")
> + private String command;
> + @SerializedName("Status")
> + private String status;
> + @SerializedName("HostConfig")
> + private HostConfig hostConfig;
> + @SerializedName("Ports")
> + private List<Port> ports;
Same, make all fields final
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839967
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof File, "this binder is only valid for File!");
> + checkNotNull(request, "request");
> +
> + File dockerFile = (File) input;
> + File tmpDir = Files.createTempDir();
> + final File targetFile = new File(tmpDir + File.separator + "Dockerfile");
> + try {
> + Files.copy(dockerFile, targetFile);
> + File archive = Archives.tar(tmpDir, File.createTempFile("archive", ".tar"));
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
The doc says "**Content-type** – should be set to `"application/tar"` but I'm not sure it is supported
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952427
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @SuppressWarnings("unchecked")
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(checkNotNull(input, "input") instanceof File, "this binder is only valid for File!");
> + checkNotNull(request, "request");
> +
> + File dockerFile = (File) input;
> + File tmpDir = Files.createTempDir();
> + final File targetFile = new File(tmpDir + File.separator + "Dockerfile");
> + try {
> + Files.copy(dockerFile, targetFile);
> + File archive = Archives.tar(tmpDir, File.createTempFile("archive", ".tar"));
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
If the payload is a tar file, is "text/plain" the right content-type to send in the request headers?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936645
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.created = checkNotNull(created, "created");
> + this.path = checkNotNull(path, "path");
> + this.args = checkNotNull(args, "args");
> + this.config = checkNotNull(config, "config");
> + this.state = checkNotNull(state, "state");
> + this.image = checkNotNull(image, "image");
> + this.networkSettings = checkNotNull(networkSettings, "networkSettings");
> + this.resolvConfPath = checkNotNull(resolvConfPath, "resolvConfPath");
> + this.driver = checkNotNull(driver, "driver");
> + this.execDriver = checkNotNull(execDriver, "execDriver");
> + this.volumes = checkNotNull(volumes, "volumes");
> + this.volumesRw = checkNotNull(volumesRW, "volumesRW");
> + this.command = checkNotNull(command, "command");
> + this.status = checkNotNull(status, "status");
> + this.hostConfig = checkNotNull(hostConfig, "hostConfig");
> + this.ports = checkNotNull(ports, "ports");
Make this immutable too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937481
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + </test.docker.endpoint>
> + <test.docker.api-version>${test.docker.api-version}
> + </test.docker.api-version>
> + <test.docker.credential>${test.docker.credential}
> + </test.docker.credential>
> + </systemPropertyVariables>
> + </configuration>
> + </execution>
> + </executions>
> + </plugin>
> + </plugins>
> + </build>
> + </profile>
> + </profiles>
> +
> +</project>
Use a 2 space indent in XML files
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838130
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + nodeMetadataBuilder.location(locationBuilder.build());
> + // TODO setup hardware and hostname properly
> + if (container.getStatus() != null) {
> + nodeMetadataBuilder.status(container.getStatus().contains("Up") ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + } else {
> + nodeMetadataBuilder.status(container.getState().isRunning() ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + }
> + nodeMetadataBuilder.imageId(container.getImage());
> + nodeMetadataBuilder.loginPort(getLoginPort(container));
> + nodeMetadataBuilder.publicAddresses(getPublicIpAddresses());
> + nodeMetadataBuilder.privateAddresses(getPrivateIpAddresses(container));
> + nodeMetadataBuilder.operatingSystem(OperatingSystem.builder().description("my description")
> + .family(OsFamily.UNRECOGNIZED)
Unfortunately images don't have much metadata about the OS :(
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10844965
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #977](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/977/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40758682
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public DockerApiMetadata() {
> + this(new Builder());
> + }
> +
> + protected DockerApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(Constants.PROPERTY_MAX_RETRIES, "15");
> + properties.setProperty("jclouds.ssh.retry-auth", "true");
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
> + properties.setProperty("image.login-user", "root");
Where is this property (without the jclouds prefix) used?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265049
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
Thanks @demobox I will.
Not sure about the latest 2 checkstyle violations: any hint?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40726804
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + checkArgument(checkNotNull(input, "input") instanceof File, "this binder is only valid for File!");
> + checkNotNull(request, "request");
> +
> + File dockerFile = (File) input;
> + File tmpDir = Files.createTempDir();
> + final File targetFile = new File(tmpDir + File.separator + "Dockerfile");
> + try {
> + Files.copy(dockerFile, targetFile);
> + File archive = Archives.tar(tmpDir, File.createTempFile("archive", ".tar"));
> + FileInputStream data = new FileInputStream(archive);
> + Payload payload = Payloads.newInputStreamPayload(data);
> + payload.getContentMetadata().setContentLength(data.getChannel().size());
> + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);
> + request.setPayload(payload);
> + } catch (IOException e) {
> + logger.error("Couldn't create a tarball for {}", targetFile, e);
The throwable argument should be the first one, otherwise it will be considered as a message parameter. jclouds logger also uses String.format, so the `{}` placeholder should be replaced by `%s`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265181
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.common.io.CharStreams;
> +
> +@Test(groups = "unit", testName = "BindInputStreamToRequestTest")
> +public class BindInputStreamToRequestTest {
> +
> + @Test
> + public void testBindInputStreamToRequest() throws IOException {
> + BindInputStreamToRequest binder = new BindInputStreamToRequest();
> +
> + HttpRequest request = HttpRequest.builder().method("GET").endpoint("http://test").build();
> + request = binder.bindToRequest(request, File.createTempFile("dockerfile", ""));
> + String rawContent = CharStreams.toString(new InputStreamReader((FileInputStream) request.getPayload().getRawContent(), "UTF-8"));
> + assertTrue(rawContent.startsWith("Dockerfile"));
> + assertEquals(request.getPayload().getContentMetadata().getContentType(), "application/tar");
> + }
Add also a couple small tests to verify the preconditions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174685
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + assertRequestHasParameters(server.takeRequest(), "POST", "/images/create", ImmutableMultimap.of("fromImage",
> + "base"));
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testCreateImageFailure() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(404));
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + try {
> + remoteApi.createImage(CreateImageOptions.Builder.fromImage("base"));
> + assertRequestHasParameters(server.takeRequest(), "POST", "/images/create", ImmutableMultimap.of("fromImage",
Remove this and fail the test instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938208
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + throw Throwables.propagate(e);
> + }
> + command.setException(exception);
> + }
> + }
> +
> + public String parseMessage(HttpResponse response) {
> + if (response.getPayload() == null)
> + return null;
> + try {
> + return Strings2.toString(response.getPayload());
> + } catch (IOException e) {
> + throw Throwables.propagate(e);
> + } finally {
> + try {
> + response.getPayload().getInput().close();
The payload itself is already closeable. Close the payload to avoid the deprecation warning?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174535
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + .id("")
> + .ram(container.getConfig().getMemory())
> + .processor(new Processor(container.getConfig().getCpuShares(), container.getConfig().getCpuShares()))
> + .build());
> + // TODO Set up location properly
> + LocationBuilder locationBuilder = new LocationBuilder();
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + builder.location(locationBuilder.build());
> + builder.status(toPortableStatus.apply(container.getState()));
> + builder.imageId(container.getImage());
> + builder.loginPort(getLoginPort(container));
> + builder.publicAddresses(getPublicIpAddresses());
> + builder.privateAddresses(getPrivateIpAddresses(container));
> + builder.operatingSystem(OperatingSystem.builder().description("linux").family(OsFamily.LINUX).build());
Could you be more precise here and populate the OperatingSystem from the Image, to have the proper details of the distribution, etc?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936896
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + .privileged(true);
> +
> + // TODO improve volumes management
> + if (volumeBindings) {
> + for (Volume v : template.getHardware().getVolumes()) {
> + hostConfigBuilder.binds(ImmutableList.of(v.getDevice() + ":/root"));
> + }
> + } else {
> + hostConfigBuilder.binds(ImmutableList.of("/var/lib/docker:/root"));
> + }
> + HostConfig hostConfig = hostConfigBuilder.build();
> +
> + api.getRemoteApi().startContainer(container.getId(), hostConfig);
> + container = api.getRemoteApi().inspectContainer(container.getId());
> + if (!container.getState().isRunning()) {
> + throw new IllegalStateException(String.format("Container %s has not started correctly", container.getId()));
Rollback the container before throwing the exception?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839692
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + @QueryParam("limit") String limit,
> + @QueryParam("since") String since,
> + @QueryParam("before") String before);
> +
> + /**
> + * List containers
> + *
> + * @return the running containers.
> + */
> + @Named("containers:list")
> + @GET
> + @Path("/containers/json")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + List<Container> listContainers(
> + @QueryParam("all") boolean all);
do you mean something like https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/features/ImageAsyncApi.java#L68-L89
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10844014
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api) {
> + this.api = checkNotNull(api, "api");
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> + DockerTemplateOptions templateOptions = DockerTemplateOptions.class.cast(template.getOptions());
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String loginUser = template.getImage().getDefaultCredentials().getUser();
> + String loginUserPassword = template.getImage().getDefaultCredentials().getPassword();
good catch @nacx! Do you think it is ok to set as default credentials root/password for all of the images as this is one of the assumptions to use jclouds-docker. wdyt?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954268
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + builder.operatingSystem(OperatingSystem.builder().description("linux").family(OsFamily.LINUX).build());
> + return builder.build();
> + }
> +
> + private String cleanUpName(String name) {
> + return name.startsWith("/") ? name.substring(1) : name;
> + }
> +
> + private Iterable<String> getPrivateIpAddresses(Container container) {
> + if (container.getNetworkSettings() == null) return ImmutableList.of();
> + return ImmutableList.of(container.getNetworkSettings().getIpAddress());
> + }
> +
> + private List<String> getPublicIpAddresses() {
> + String dockerIpAddress = URI.create(providerMetadata.getEndpoint()).getHost();
> + return ImmutableList.of(dockerIpAddress);
this seems to be the default right now. Docker is running fast so probably this could change in next versions ...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953630
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + nodeMetadataBuilder.location(locationBuilder.build());
> + // TODO setup hardware and hostname properly
> + if (container.getStatus() != null) {
> + nodeMetadataBuilder.status(container.getStatus().contains("Up") ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + } else {
> + nodeMetadataBuilder.status(container.getState().isRunning() ? NodeMetadata.Status.RUNNING : NodeMetadata.Status.SUSPENDED);
> + }
> + nodeMetadataBuilder.imageId(container.getImage());
> + nodeMetadataBuilder.loginPort(getLoginPort(container));
> + nodeMetadataBuilder.publicAddresses(getPublicIpAddresses());
> + nodeMetadataBuilder.privateAddresses(getPrivateIpAddresses(container));
> + nodeMetadataBuilder.operatingSystem(OperatingSystem.builder().description("my description")
> + .family(OsFamily.UNRECOGNIZED)
Isn't there a way this could be detected? Even parsing some field?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839063
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + private final String gateway;
> + @SerializedName("Bridge")
> + private final String bridge;
> + @SerializedName("PortMapping")
> + private final String portMapping;
> + @SerializedName("Ports")
> + private final Map<String, List<Map<String, String>>> ports;
> +
> + @ConstructorProperties({ "IpAddress", "IpPrefixLen", "Gateway", "Bridge", "Ports" })
> + public NetworkSettings(String ipAddress, int ipPrefixLen, String gateway, String bridge, String portMapping,
> + Map<String, List<Map<String, String>>> ports) {
> + this.ipAddress = ipAddress;
> + this.ipPrefixLen = ipPrefixLen;
> + this.gateway = gateway;
> + this.bridge = bridge;
> + this.portMapping = portMapping;
Add null checks for all properties that are not nullable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937553
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
@imesh jclouds master branch uses Guava 17.0 which makes ```Stopwatch``` constructors private access. The jclouds dependency supersedes your application dependency on an older version of Guava. You should update your application dependency to 17.0 and address the Guava API changes and deprecations to make progress.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45007831
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +public class ContainerToNodeMetadata implements Function<Container, NodeMetadata> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final ApiContext<DockerApi> context;
> +
> + @Inject
> + public ContainerToNodeMetadata(ApiContext<DockerApi> context) {
> + this.context = context;
> + }
> +
> + @Override
> + public NodeMetadata apply(Container container) {
> + // TODO extract group and name from description?
You can inject the groupNamingConvention for this purpose.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838983
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + server.shutdown();
> + }
> + }
> +
> + public void testStartNonExistingContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + try {
> + try {
> + remoteApi.startContainer("1");
> + } catch (ResourceNotFoundException ex) {
> + // Expected exception
> + }
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/containers/1/start");
Move this to the catch block, and add a explicit `fail` after calling the api method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938183
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
> + return this;
> + }
> +
> + public TemplateOptions commands(Iterable<String> commands) {
> + for (String command : checkNotNull(commands, "commands"))
> + checkNotNull(command, "all commands must be non-empty");
> + this.commands = Optional.<List<String>> of(ImmutableList.copyOf(commands));
> + return this;
> + }
> +
> + public TemplateOptions commands(String... commands) {
> + return commands(ImmutableList.copyOf(checkNotNull(commands, "commands")));
> + }
> +
> + public TemplateOptions cpuShares(int cpuShares) {
> + checkNotNull(cpuShares, "cpuShares was null");
Why do you do the `checkNotNull` then create an `Optional`? In fact, since this is a primitive, it cannot ever *be* null. If we want the null semantics, change to the following (and see also `memory`, plus `dns` and `hostname` which are strings, so *could* be null...)
```Java
public TemplateOptions cpuShares(@Nullable Integer cpuShares) {
this.cpuShares = Optional.fromNullable(cpuShares);
return this;
}
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14482891
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
Here you can find the result of `mvn clean install -Plive`
https://gist.github.com/andreaturli/595ce6e44a987ed5be06
unfortunately there are still issue with the DockerComputeServiceLiveTest but I think it is good to be accepted on jclouds-labs at least and fix liveTests there, if you agree
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47675918
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
In general, make all domain model classes immutable by setting the fields to final. Also add the proper null and other checks for all parameters in the constructor.
There are also no unit tests, and there should be added for:
* All custom binders.
* The custom json parser.
* Mock tests for all RemoteApi methods.
* Tests for all transformation Functions.
This is enough for a first-round review. Nice work @andreaturli! Look forward to seeing progress on this!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38287947
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + return this;
> + }
> +
> + public TemplateOptions commands(Iterable<String> commands) {
> + for (String command : checkNotNull(commands, "commands"))
> + checkNotNull(command, "all commands must be non-empty");
> + this.commands = Optional.<List<String>> of(ImmutableList.copyOf(commands));
> + return this;
> + }
> +
> + public TemplateOptions commands(String... commands) {
> + return commands(ImmutableList.copyOf(checkNotNull(commands, "commands")));
> + }
> +
> + public TemplateOptions cpuShares(int cpuShares) {
> + checkNotNull(cpuShares, "cpuShares was null");
thanks @grkvlt, much cleaner
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14484314
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.attachStdin = attachStdin;
> + return this;
> + }
> +
> + public Builder attachStdout(boolean attachStdout) {
> + this.attachStdout = attachStdout;
> + return this;
> + }
> +
> + public Builder attachStderr(boolean attachStderr) {
> + this.attachStderr = attachStderr;
> + return this;
> + }
> +
> + public Builder exposedPorts(Map<String, ?> exposedPorts) {
> + this.exposedPorts = exposedPorts;
Better create a copy with `ImmutableMap.copyOf`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839891
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #975](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/975/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40729059
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
Hi @imesh thanks for your presentation. If you cat test it that would be great!
Please let us know your result/experience/suggestion eithere here or (better) on jclouds ml. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40286583
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +import javax.annotation.Resource;
> +import javax.inject.Named;
> +import java.io.IOException;
> +import java.util.Map;
> +import java.util.Set;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Test(groups = "live", singleThreaded = true, testName = "DockerExperimentLiveTest")
> +public class DockerExperimentLiveTest extends BaseDockerApiLiveTest {
Change the name to something more meaningful?
Also, if this test targets the ComputeServie, it should be better to extend `BaseViewLiveTest` instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937759
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + public NodeMetadata apply(Container container) {
> + String name = cleanUpName(container.getName());
> + String group = nodeNamingConvention.extractGroup(name);
> + NodeMetadataBuilder builder = new NodeMetadataBuilder();
> + builder.ids(container.getId())
> + .name(name)
> + .group(group)
> + .hostname(container.getConfig().getHostname())
> + // TODO Set up hardware
> + .hardware(new HardwareBuilder()
> + .id("")
> + .ram(container.getConfig().getMemory())
> + .processor(new Processor(container.getConfig().getCpuShares(), container.getConfig().getCpuShares()))
> + .build());
> + // TODO Set up location properly
> + LocationBuilder locationBuilder = new LocationBuilder();
If I'm not wrong, the default location, if no locations are configured is the "provider". You should inject here the location supplier and use that location, as the provider is already the host.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937658
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> +@Singleton
> +public class DockerComputeServiceAdapter implements
> + ComputeServiceAdapter<Container, Hardware, Image, Location> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final DockerApi api;
> + private final ApiContext<DockerApi> context;
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api, ApiContext<DockerApi> context) {
> + this.api = checkNotNull(api, "api");
> + this.context = context;
Do you really need the context? You could better inject directly the Identity and the Credential supplier.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839266
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + //return api.getRemoteApi().listImages();
> + Set<Image> images = Sets.newHashSet();
> + for (Image image : api.getRemoteApi().listImages()) {
> + // less efficient than just listNodes but returns richer json that needs repoTags coming from listImages
> + Image inspected = api.getRemoteApi().inspectImage(image.getId());
> + if(image.getRepoTags() != null) {
> + inspected = Image.builder().fromImage(inspected).repoTags(image.getRepoTags()).build();
> + }
> + images.add(inspected);
> + }
> + return images;
> + }
> +
> + @Override
> + public Image getImage(final String imageId) {
> + return find(listImages(), new Predicate<Image>() {
This will call the api to inspect every image, when you only want to retrieve one. Change this to directly call inspectImage.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937282
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + assertNull(api().inspectImage(image.getId()));
> + }
> +
> + @Test(dependsOnMethods = "testDeleteImage")
> + public void testBuildImage() throws IOException, InterruptedException, URISyntaxException {
> + BuildOptions options = BuildOptions.Builder.tag("testBuildImage").verbose(false).nocache(false);
> + InputStream buildImageStream = api().build(new File(Resources.getResource("centos/Dockerfile").toURI()), options);
> + String buildStream = consumeStream(buildImageStream, false);
> + Iterable<String> splitted = Splitter.on("\n").split(buildStream.replace("\r", "").trim());
> + String lastStreamedLine = Iterables.getLast(splitted).trim();
> + String rawImageId = Iterables.getLast(Splitter.on("Successfully built ").split(lastStreamedLine));
> + String imageId = rawImageId.substring(0, 11);
> + Image image = api().inspectImage(imageId);
> + api().removeContainer(image.getContainer());
> + api().deleteImage(imageId, DeleteImageOptions.Builder.force(true));
> + }
There are still methods int he RemoteApi that don't have the corresponding live test. They must be added.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937868
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
great @nacx! Thanks again for your valuable reviews
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-48165987
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.memory = memory;
> + this.memorySwap = memorySwap;
> + this.attachStdin = attachStdin;
> + this.attachStdout = attachStdout;
> + this.attachStderr = attachStderr;
> + this.exposedPorts = exposedPorts;
> + this.tty = tty;
> + this.openStdin = openStdin;
> + this.stdinOnce = stdinOnce;
> + this.env = env;
> + this.cmd = cmd == null ? Lists.<String>newArrayList() : cmd;
> + this.dns = dns;
> + this.image = image;
> + this.volumes = volumes;
> + this.volumesFrom = volumesFrom;
> + this.workingDir = workingDir;
Add null checks for all non-nullable properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839799
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + try {
> + remoteApi.stopContainer("1");
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/containers/1/stop");
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testStopNonExistingContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + try {
> + remoteApi.stopContainer("1");
Same here. Add a `fail` to make the test fail here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938195
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1139](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1139/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45490816
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + dockerFile.deleteOnExit();
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + private File createDockerFile(String content) {
> + File newTempDir = Files.createTempDir();
> + File dockerFile = new File(newTempDir + "/dockerFile");
> + try {
> + Files.write(content, dockerFile, Charsets.UTF_8);
> + } catch(IOException e) {
> + Assert.fail();
> + }
> + return dockerFile;
> + }
Add the missing tests for the methods:
* commit
* inspectImage
* listImages
* removeContainer with options
* deleteImage with options
* startContainer with options
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938064
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the detailed review, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-44155452
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import java.io.Closeable;
> +
> +/**
> + * Provides synchronous access to Docker.
> + *
> + * @author Andrea Turli
> + * @see <a href="http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8"></a>
> + */
> +public interface DockerApi extends Closeable {
> +
> + /**
> + * Provides synchronous access to Docker Remote API features.
> + */
> + @Delegate
> + RemoteApi getRemoteApi();
Fine by me then!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11264992
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #140](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/140/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40757727
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api, ApiContext<DockerApi> context) {
> + this.api = checkNotNull(api, "api");
> + this.context = context;
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String defaultIdentity = context.getProviderMetadata().getApiMetadata().getDefaultIdentity().get();
> + String defaultCredential = context.getProviderMetadata().getApiMetadata().getDefaultCredential().get();
This default identity and credentials defined in the metadata are supposed to be the "api" credentials, not the ones to connect to the containers. Default credentials for deploiyed nodes can be set with the [IMAGE_LOGIN_USER](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/ComputeServiceProperties.java#L87) property, in the metadata detault properties.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839513
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
Rebased and [merged](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=9b124ee9f12e0392b6d2f083308297bfcca8ea79). Huge thanks @andreaturli!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-48159799
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.common.base.Objects;
> +import com.google.common.collect.ImmutableSet;
> +
> +import java.beans.ConstructorProperties;
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class ExposedPorts {
> +
> + private final String portAndProtocol;
> + private final Set<String> hostPorts;
Add the missing `@SerlalizedName` annotations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937504
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +##Components
> +
> +- jclouds \- acts as a java client to access to docker features
> +- DOCKER_HOST \- hosts Docker API, NB: jclouds-docker assumes that the latest Docker is installed
> +- Image \- it is a docker image that can be started.
> +- Node \- is a docker container
> +
> +## Assumptions
> +
> +- jclouds-docker assumes that the images specified using the template are ssh'able.
> +- jclouds-docker will mount ${user.home} to /root of the container
> +
> +--------------
> +
> +#Notes:
> +- jclouds-docker is still at alpha stage please report any issues you find at [jclouds issues](https://github.com/jclouds/jclouds/issues?state=open)
Ping! This link still has to be changed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14173875
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + */
> +@Singleton
> +public class DockerComputeServiceAdapter implements
> + ComputeServiceAdapter<Container, Hardware, Image, Location> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final DockerApi api;
> + private final ApiContext<DockerApi> context;
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api, ApiContext<DockerApi> context) {
> + this.api = checkNotNull(api, "api");
> + this.context = context;
Do you have an example for that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10845146
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testVersion() {
> + Assert.assertEquals(api().getVersion().getVersion(), "0.9.0");
> + }
> +
> + @Test(dependsOnMethods = "testVersion")
> + public void testCreateImage() throws IOException, InterruptedException {
> + CreateImageOptions options = CreateImageOptions.Builder.fromImage(BUSYBOX_IMAGE);
> + InputStream createImageStream = api().createImage(options);
> + consumeStream(createImageStream, false);
> + image = api().inspectImage(BUSYBOX_IMAGE);
> + assertNotNull(image);
> + }
> +
> + @Test(dependsOnMethods = "testCreateImage")
> + public void testListImages() {
> + Assert.assertNotNull(api().listImages());
There are more after this one. Use them in general in this class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937821
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + String driver, String execDriver, Map<String, String> volumes, Map<String, Boolean> volumesRW,
> + String command, String status, HostConfig hostConfig, List<Port> ports) {
> + this.id = checkNotNull(id, "id");
> + this.name = checkNotNull(name, "name");
> + this.created = checkNotNull(created, "created");
> + this.path = checkNotNull(path, "path");
> + this.args = checkNotNull(args, "args");
> + this.config = checkNotNull(config, "config");
> + this.state = checkNotNull(state, "state");
> + this.image = checkNotNull(image, "image");
> + this.networkSettings = checkNotNull(networkSettings, "networkSettings");
> + this.resolvConfPath = checkNotNull(resolvConfPath, "resolvConfPath");
> + this.driver = checkNotNull(driver, "driver");
> + this.execDriver = checkNotNull(execDriver, "execDriver");
> + this.volumes = checkNotNull(volumes, "volumes");
> + this.volumesRw = checkNotNull(volumesRW, "volumesRW");
Same here, make it immutable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937472
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + <artifactId>maven-surefire-plugin</artifactId>
> + <executions>
> + <execution>
> + <id>integration</id>
> + <phase>integration-test</phase>
> + <goals>
> + <goal>test</goal>
> + </goals>
> + <configuration>
> + <systemPropertyVariables>
> + <test.docker.endpoint>${test.docker.endpoint}
> + </test.docker.endpoint>
> + <test.docker.api-version>${test.docker.api-version}
> + </test.docker.api-version>
> + <test.docker.credential>${test.docker.credential}
> + </test.docker.credential>
Close tags in the same line
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838140
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + .ids(from.getId())
> + .name(get(Splitter.on(":").split(description), 0))
> + .description(description)
> + .operatingSystem(os)
> + .status(Image.Status.AVAILABLE)
> + .build();
> + }
> +
> + /**
> + * Parses the item description to determine the OSFamily
> + *
> + * @return the @see OsFamily or OsFamily.UNRECOGNIZED
> + */
> + private Function<String, OsFamily> osFamily() {
> + return new Function<String, OsFamily>() {
> + OsFamily osFamily = OsFamily.UNRECOGNIZED;
There is no need for this local variable. Just return directly the detected os family.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839200
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @SerializedName("StdinOnce")
> + private boolean stdinOnce;
> + @SerializedName("Env")
> + private List<String> env;
> + @SerializedName("Cmd")
> + private List<String> cmd;
> + @SerializedName("Dns")
> + private List<String> dns;
> + @SerializedName("Image")
> + private String image;
> + @SerializedName("Volumes")
> + private Map<String, ?> volumes;
> + @SerializedName("VolumesFrom")
> + private String volumesFrom;
> + @SerializedName("WorkingDir")
> + private String workingDir;
Declare all fields final
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839772
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> +
> +import java.io.Closeable;
> +
> +/**
> + * Provides synchronous access to Docker.
> + *
> + * @author Andrea Turli
> + * @see <a href="http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8"></a>
> + */
> +public interface DockerApi extends Closeable {
> +
> + /**
> + * Provides synchronous access to Docker Remote API features.
> + */
> + @Delegate
> + RemoteApi getRemoteApi();
Potentially as they supports many APIs http://docs.docker.io/en/latest/reference/api/
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10843226
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
@nacx live tests are healthy too,
```
Results :
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38604658
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + return userExecutor.submit(new Callable<Image>() {
> + @Override
> + public Image call() throws Exception {
> + if (imageAvailablePredicate.apply(image))
> + return image.get();
> + throw new UncheckedTimeoutException("Image was not created within the time limit: " + image.get());
> + }
> + });
> + }
> +
> + @Override
> + public boolean deleteImage(String id) {
> + try {
> + api.getRemoteApi().deleteImage(id);
> + } catch (Exception e) {
> + logger.error(e, "Could not delete machine with id %s ", id);
"delete image"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838573
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
> +
> + if (templateOptions.getCommands().isPresent()) {
> + containerConfigBuilder.cmd(templateOptions.getCommands().get());
> + }
> +
> + if (templateOptions.getMemory().isPresent()) {
> + containerConfigBuilder.memory(templateOptions.getMemory().get());
> + }
> +
> + if (templateOptions.getCpuShares().isPresent()) {
> + containerConfigBuilder.cpuShares(templateOptions.getCpuShares().get());
> + }
> +
> + if (templateOptions.getVolumes().isPresent()) {
> + Map<String, Object> volumes = Maps.newLinkedHashMap();
> + for (String containerDir : templateOptions.getVolumes().get().values()) {
I missed the `HostConfig` setup a few lines later, which *does* do the host binding. We don't yet distinguish between RW and RO bindings, but that is a future enhancement probably.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14485991
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
PS: Thanks for the cleanup, of course, @andreaturli!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40754567
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +-->
> +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
> + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
> + <modelVersion>4.0.0</modelVersion>
> + <parent>
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>jclouds-labs</artifactId>
> + <version>1.8.0-SNAPSHOT</version>
> + </parent>
> +
> + <!-- TODO: when out of labs, switch to org.jclouds.provider -->
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>docker</artifactId>
> + <name>jclouds docker API</name>
> + <description>ComputeService binding to the Docker v1.8 API</description>
No need to put the concrete version here. Will be easier to upgrade.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11264924
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
Closed #57.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#event-138666393
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.util.Map;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static com.google.common.collect.Iterables.getOnlyElement;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class ContainerToNodeMetadata implements Function<Container, NodeMetadata> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final ApiContext<DockerApi> context;
Try to avoid injecting the context. If you only need the endpoint, you can inject the `ProviderMetadata` directly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11266257
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Override
> + public Iterable<Location> listLocations() {
> + return ImmutableSet.of();
> + }
> +
> + @Override
> + public Container getNode(String id) {
> + return api.getRemoteApi().inspectContainer(id);
> + }
> +
> + @Override
> + public void destroyNode(String id) {
> + api.getRemoteApi().stopContainer(id);
> + api.getRemoteApi().removeContainer(id);
Can the container be removed even if the stop operation fails?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937360
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.compute.ComputeServiceAdapter;
> +import org.jclouds.compute.config.ComputeServiceAdapterContextModule;
> +import org.jclouds.compute.domain.Hardware;
> +import org.jclouds.compute.domain.NodeMetadata;
> +import org.jclouds.compute.extensions.ImageExtension;
> +import org.jclouds.docker.compute.extensions.DockerImageExtension;
> +import org.jclouds.docker.compute.functions.ContainerToNodeMetadata;
> +import org.jclouds.docker.compute.functions.ImageToImage;
> +import org.jclouds.docker.compute.strategy.DockerComputeServiceAdapter;
> +import org.jclouds.docker.domain.Container;
> +import org.jclouds.docker.domain.Image;
> +import org.jclouds.domain.Location;
> +import org.jclouds.functions.IdentityFunction;
> +
> +/**
> + * @author Adrian Cole
Change the author here :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838431
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testListContainers() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setBody(payloadFromResource("/containers.json")));
> +
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> +
> + try {
> + Set<Container> containers = remoteApi.listContainers();
> + assertRequestHasCommonFields(server.takeRequest(), "/containers/json");
> + assertEquals(containers.size(), 1);
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
Add the corresponding test when the response is a 404 (to verify the fallback configured in the api).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937911
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.Binder;
> +
> +import com.google.common.base.Throwables;
> +import com.google.common.io.Files;
> +
> +/**
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class BindInputStreamToRequest implements Binder {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @SuppressWarnings("unchecked")
This still has to be removed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174051
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public ListenableFuture<Image> createImage(ImageTemplate template) {
> + checkArgument(template instanceof CloneImageTemplate,
> + " docker only currently supports creating images through cloning.");
> + CloneImageTemplate cloneTemplate = (CloneImageTemplate) template;
> +
> + Container container = api.getRemoteApi().inspectContainer(cloneTemplate.getSourceNodeId());
> + CommitOptions options = CommitOptions.Builder.containerId(container.getId()).tag(cloneTemplate.getName());
> + org.jclouds.docker.domain.Image dockerImage = api.getRemoteApi().commit(options);
> +
> + dockerImage = org.jclouds.docker.domain.Image.builder().fromImage(dockerImage)
> + .repoTags(ImmutableList.of(cloneTemplate.getName() + ":latest"))
> + .build();
> +
> + logger.info(">> Registered new image %s, waiting for it to become available.", dockerImage.getId());
> + final AtomicReference<Image> image = Atomics.newReference(new ImageToImage().apply(dockerImage));
Inject the `ImageToImage` function in the constructor.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936696
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api) {
> + this.api = checkNotNull(api, "api");
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> + DockerTemplateOptions templateOptions = DockerTemplateOptions.class.cast(template.getOptions());
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String loginUser = template.getImage().getDefaultCredentials().getUser();
> + String loginUserPassword = template.getImage().getDefaultCredentials().getPassword();
Having a deeper look, the Image will already have the default credentials populated with the value of the `ComputeServiceProperties.IMAGE_LOGIN_USER` value, so it should be enough.
However, you should take into account here that the user can have used the `templateOptions` to override the username, the password, or the private key. The login options provided in the `templateOptions` take preference over the image defaults.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954809
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setBody(payloadFromResource("/containers.json")));
> +
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> +
> + try {
> + Set<Container> containers = remoteApi.listContainers(ListContainerOptions.Builder.all(true));
> + assertRequestHasParameters(server.takeRequest(), "/containers/json", ImmutableMultimap.of("all",
> + "true"));
> + assertEquals(containers.size(), 1);
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
Add the corresponding test when the response is a 404 (to verify the fallback configured in the api).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937917
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @Named("version")
> + @GET
> + @Path("/version")
> + Version getVersion();
> +
> + /**
> + * List all running containers
> + *
> + * @param options the options to list the containers (@see ListContainerOptions)
> + * @return a set of containers
> + */
> + @Named("containers:list")
> + @GET
> + @Path("/containers/json")
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<Container> listContainers(ListContainerOptions... options);
The varargs pattern for options is no longer used. Add a `listContainers` method that lists all them (with no options) and change this one to accept just one options parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265417
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> + @Named("images:list")
> + @GET
> + @Path("/images/json")
> + @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
> + Set<Image> listImages(ListImageOptions options);
> +
> + /**
> + * Inspect an image
> + *
> + * @param imageId The id of the image to inspect.
> + * @return low-level information on the image name
> + */
> + @Named("image:inspect")
> + @GET
> + @Path("/images/{name}/json")
[minor] If the parameter is actually the "id" of the image, change the path param to "id"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265495
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import java.io.Closeable;
> +
> +/**
> + * Provides synchronous access to Docker.
> + *
> + * @author Andrea Turli
> + * @see <a href="http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8"></a>
> + */
> +public interface DockerApi extends Closeable {
> +
> + /**
> + * Provides synchronous access to Docker Remote API features.
> + */
> + @Delegate
> + RemoteApi getRemoteApi();
Is there a plan to add more apis to this class? If not, you could consider moving all methods from the `RemoteApi` to the `DockerApi`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838196
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api) {
> + this.api = checkNotNull(api, "api");
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> + DockerTemplateOptions templateOptions = DockerTemplateOptions.class.cast(template.getOptions());
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String loginUser = template.getImage().getDefaultCredentials().getUser();
> + String loginUserPassword = template.getImage().getDefaultCredentials().getPassword();
For further reference [this class](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/ReturnCredentialsBoundToImage.java) is the one that populates the default credentials to the images (you don't have to worry about that in the `ImageToImage` function, and [this is the one](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/BaseComputeServiceContextModule.java#L94-L95) that provides the default values, if no credentials are present, reading the value of the mentioned property.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12955077
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @andreaturli! All the points in the checklist have been addressed, and I think it is almost ready to be merged. Could you address the last comments? (Don't forget about [this one](https://github.com/jclouds/jclouds-labs/pull/57/files#r14173875) and [this one](https://github.com/jclouds/jclouds-labs/pull/57/files#r14174051)).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47076635
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + private int getLoginPort(Container container) {
> + if (container.getNetworkSettings() != null) {
> + Map<String, List<Map<String,String>>> ports = container.getNetworkSettings().getPorts();
> + if(ports != null) {
> + return Integer.parseInt(getOnlyElement(ports.get("22/tcp")).get("HostPort"));
> + }
> + } else if (container.getPorts() != null) {
> + for (Port port : container.getPorts()) {
> + if (port.getPrivatePort() == 22) {
> + return port.getPublicPort();
> + }
> + }
> + }
> + throw new IllegalStateException("Cannot determine the login port for " + container.getId());
Is it ok to fail here? By default, jclouds will assume port 22 as a login port. If we can't detect it, should we fail, or let jclouds try to use port 22 anyway?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839133
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.util.Set;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class ExposedPorts {
> +
> + private final String portAndProtocol;
> + private final Set<String> hostPorts;
> +
> + @ConstructorProperties({ "PortAndProtocol", "HostPorts" })
> + public ExposedPorts(String portAndProtocol, Set<String> hostPorts) {
> + this.portAndProtocol = checkNotNull(portAndProtocol, "portAndProtocol");
> + this.hostPorts = hostPorts != null ? ImmutableSet.copyOf(hostPorts) : ImmutableSet.<String> of();
If `hostPorts` can be null, annotate it as `@Nullable`, otherwise add a null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937518
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + private Iterable<String> getPrivateIpAddresses(Container container) {
> + if (container.getNetworkSettings() == null) return ImmutableList.of();
> + return ImmutableList.of(container.getNetworkSettings().getIpAddress());
> + }
> +
> + private List<String> getPublicIpAddresses() {
> + String dockerIpAddress = URI.create(providerMetadata.getEndpoint()).getHost();
> + return ImmutableList.of(dockerIpAddress);
> + }
> +
> + protected static int getLoginPort(Container container) {
> + if (container.getNetworkSettings() != null) {
> + Map<String, List<Map<String,String>>> ports = container.getNetworkSettings().getPorts();
> + if(ports != null) {
Add a space after the `ìf``.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936985
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + try {
> + remoteApi.deleteImage("1");
> + assertRequestHasCommonFields(server.takeRequest(), "DELETE", "/images/1");
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testDeleteNotExistingImage() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(404));
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + try {
> + remoteApi.deleteImage("1");
Same pattern. Fail the test here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938217
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1208](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1208/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47081016
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + private Logger logger = Logger.NULL;
> + private final DockerApi api;
> + private final ListeningExecutorService userExecutor;
> + private final Predicate<AtomicReference<Image>> imageAvailablePredicate;
> + private final ImageToImage imageToImage;
> +
> + @Inject
> + public DockerImageExtension(DockerApi api, @Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService
> + userExecutor, @Named(TIMEOUT_IMAGE_AVAILABLE) Predicate<AtomicReference<Image>> imageAvailablePredicate, ImageToImage imageToImage) {
> + this.api = checkNotNull(api, "api");
> + this.userExecutor = checkNotNull(userExecutor, "userExecutor");
> + this.imageAvailablePredicate = checkNotNull(imageAvailablePredicate, "imageAvailablePredicate");
> + this.imageToImage = imageToImage;
Add also a null check for this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174091
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #184](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/184/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47081238
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +/**
> + * Docker implementation of {@link org.jclouds.compute.extensions.ImageExtension}
> + *
> + * @author Andrea Turli
> + */
> +@Singleton
> +public class DockerImageExtension implements ImageExtension {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + private Logger logger = Logger.NULL;
> + private final DockerApi api;
> + private final ListeningExecutorService userExecutor;
> + private final Predicate<AtomicReference<Image>> imageAvailablePredicate;
> + private final DockerApi dockerApi;
The docker api is declared and injected twice.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11265246
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + destroyNode(container.getId());
> + throw new IllegalStateException(String.format("Container %s has not started correctly", container.getId()));
> + }
> + return new NodeAndInitialCredentials<Container>(container, container.getId(),
> + LoginCredentials.builder().user(loginUser).password(loginUserPassword).build());
> + }
> +
> + @Override
> + public Iterable<Hardware> listHardwareProfiles() {
> + Set<Hardware> hardware = Sets.newLinkedHashSet();
> + // todo they are only placeholders at the moment
> + hardware.add(new HardwareBuilder().ids("micro").hypervisor("lxc").name("micro").ram(512).build());
> + hardware.add(new HardwareBuilder().ids("small").hypervisor("lxc").name("small").ram(1024).build());
> + hardware.add(new HardwareBuilder().ids("medium").hypervisor("lxc").name("medium").ram(2048).build());
> + hardware.add(new HardwareBuilder().ids("large").hypervisor("lxc").name("large").ram(3072).build());
> + return hardware;
at the moment you can only setup cpuShares. By default, all containers run at the same priority and get the same proportion of CPU cycles, but you can tell the kernel to give more shares of CPU time to one or more containers when you start them via Docker. So it is not really an information about cpu limit, so not sure we can use it on hardwareProfiles
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13494140
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Imesh Guaratne <no...@github.com>.
Excellent work @andreaturli! Could you please explain up to which extent you have completed the features? If you prefer I would also like to contribute.
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40268139
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + .id("")
> + .ram(container.getConfig().getMemory())
> + .processor(new Processor(container.getConfig().getCpuShares(), container.getConfig().getCpuShares()))
> + .build());
> + // TODO Set up location properly
> + LocationBuilder locationBuilder = new LocationBuilder();
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + builder.location(locationBuilder.build());
> + builder.status(toPortableStatus.apply(container.getState()));
> + builder.imageId(container.getImage());
> + builder.loginPort(getLoginPort(container));
> + builder.publicAddresses(getPublicIpAddresses());
> + builder.privateAddresses(getPrivateIpAddresses(container));
> + builder.operatingSystem(OperatingSystem.builder().description("linux").family(OsFamily.LINUX).build());
But you have the `imageId` (you're setting it a few lines before). You could use that to get the corresponding `Image` from the `ImageSupplier` and populate the details of its operating system? That should be closer to the real one than the hardcoded Linux.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953837
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.compute.options.TemplateOptions;
> +import org.jclouds.domain.LoginCredentials;
> +import org.jclouds.scriptbuilder.domain.Statement;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableMap;
> +
> +/**
> + * Contains options supported in the {@code ComputeService#runNode} operation on the
> + * "docker" provider. <h2>Usage</h2> The recommended way to instantiate a
> + * DockerTemplateOptions object is to statically import DockerTemplateOptions.* and invoke a static
> + * creation method followed by an instance mutator (if needed):
> + * <p/>
> + * <code>
> + * import static org.jclouds.aws.ec2.compute.options.DockerTemplateOptions.Builder.*;
Wrong package? :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937069
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #139](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/139/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40729306
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
> + limitations under the License.
> +
> +-->
> +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
> + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
> + <modelVersion>4.0.0</modelVersion>
> + <parent>
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>jclouds-labs</artifactId>
> + <version>1.8.0-SNAPSHOT</version>
> + </parent>
> +
> + <!-- TODO: when out of labs, switch to org.jclouds.provider -->
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>docker</artifactId>
> + <version>1.8.0-SNAPSHOT</version>
Don't think we need this since we're inheriting from the labs parent POM?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10835165
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1242](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1242/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47677662
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
@andreaturli would be nice to be able to set `Memory` and `CpuShares` in` Config` from the template options.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-44749515
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + String image, NetworkSettings networkSettings, String resolvConfPath,
> + String driver, String execDriver, Map<String, String> volumes, Map<String, Boolean> volumesRW,
> + String command, String status, HostConfig hostConfig, List<Port> ports) {
> + this.id = checkNotNull(id, "id");
> + this.name = checkNotNull(name, "name");
> + this.created = checkNotNull(created, "created");
> + this.path = checkNotNull(path, "path");
> + this.args = checkNotNull(args, "args");
> + this.config = checkNotNull(config, "config");
> + this.state = checkNotNull(state, "state");
> + this.image = checkNotNull(image, "image");
> + this.networkSettings = checkNotNull(networkSettings, "networkSettings");
> + this.resolvConfPath = checkNotNull(resolvConfPath, "resolvConfPath");
> + this.driver = checkNotNull(driver, "driver");
> + this.execDriver = checkNotNull(execDriver, "execDriver");
> + this.volumes = checkNotNull(volumes, "volumes");
Make it immutable
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937465
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + this.imageAvailablePredicate = checkNotNull(imageAvailablePredicate, "imageAvailablePredicate");
> + }
> +
> + @Override
> + public ImageTemplate buildImageTemplateFromNode(String name, final String id) {
> + Container container = api.getRemoteApi().inspectContainer(id);
> + if (container == null)
> + throw new NoSuchElementException("Cannot find container with id: " + id);
> + CloneImageTemplate template = new ImageTemplateBuilder.CloneImageTemplateBuilder().nodeId(id).name(name).build();
> + return template;
> + }
> +
> + @Override
> + public ListenableFuture<Image> createImage(ImageTemplate template) {
> + checkState(template instanceof CloneImageTemplate,
> + " docker only currently supports creating images through cloning.");
Should better use `checkArgument` to propagate an `IllegalargumentException`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838516
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
> +
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> +
> + String content = new String(payloadFromResource("/Dockerfile"));
> + File dockerFile = createDockerFile(content);
> + try {
> + try {
> + remoteApi.build(dockerFile, BuildOptions.NONE);
> + fail("Build container should fail on 404");
> + } catch (ResourceNotFoundException ex) {
> + // Expected exception
> + }
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/build");
> + } finally {
> + dockerFile.deleteOnExit();
Why not ```dockerFile.delete()```?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938165
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + checkArgument(baseDir.isDirectory(), "%s is not a directory", baseDir);
> + File[] files = baseDir.listFiles();
> + File tarFile = new File(archivePath);
> +
> + String token = getLast(Splitter.on("/").split(archivePath.substring(0, archivePath.lastIndexOf("/"))));
> +
> + byte[] buf = new byte[1024];
> + int len;
> + TarArchiveOutputStream tos = new TarArchiveOutputStream(new FileOutputStream(tarFile));
> + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
> + for (File file : files) {
> + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
> + tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
> + tos.putArchiveEntry(tarEntry);
> + if (!file.isDirectory()) {
> + FileInputStream fin = new FileInputStream(file);
thank @andrewgaul, something like the following, makes sense?
```
for (File file : files) {
TarArchiveEntry tarEntry = new TarArchiveEntry(file);
tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
tos.putArchiveEntry(tarEntry);
if (!file.isDirectory()) {
Files.asByteSource(file).copyTo(tos);
tos.closeArchiveEntry();
}
tos.close();
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953161
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.sshj.config.SshjSshClientModule;
> +import org.testng.annotations.AfterMethod;
> +import org.testng.annotations.BeforeMethod;
> +import org.testng.annotations.Test;
> +
> +import javax.annotation.Resource;
> +import javax.inject.Named;
> +import java.io.IOException;
> +import java.util.Map;
> +import java.util.Set;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertTrue;
> +
> +@Test(groups = "live", singleThreaded = true, testName = "DockerExperimentLiveTest")
> +public class DockerExperimentLiveTest extends BaseDockerApiLiveTest {
Can this be renamed to something more meaningful?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174698
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + <plugins>
> + <plugin>
> + <groupId>org.apache.maven.plugins</groupId>
> + <artifactId>maven-surefire-plugin</artifactId>
> + <executions>
> + <execution>
> + <id>integration</id>
> + <phase>integration-test</phase>
> + <goals>
> + <goal>test</goal>
> + </goals>
> + <configuration>
> + <systemPropertyVariables>
> + <test.docker.endpoint>${test.docker.endpoint}</test.docker.endpoint>
> + <test.docker.api-version>${test.docker.api-version}</test.docker.api-version>
> + <test.docker.credential>${test.docker.credential}</test.docker.credential>
Add `test.docker.identity` too
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11264963
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.io.File;
> +import java.io.FileInputStream;
> +import java.io.FileOutputStream;
> +import java.io.IOException;
> +
> +import static com.google.common.collect.Iterables.getLast;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class Archives {
> +
> + public static File tar(File baseDir, String archivePath) throws IOException {
> + // Check that the directory is a directory, and get its contents
> + if (!baseDir.isDirectory())
> + throw new IllegalArgumentException("Compress: not a directory: " + baseDir);
Use `checkArgument` instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838908
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #176](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/176/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45945836
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #137](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/137/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38606328
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Imesh Guaratne <no...@github.com>.
Thanks all for the quick responses!
@andrewgaul
Yes Stopwatch constructor has been made private in com.google.guava 17.0. That's the reason why I think we see this problem.
@andreaturli
I think I found the problem. I was able to build jclouds-labs master branch without any problem therefore I checked following class there and found:
jclouds-labs/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndInstallVm.java
Stopwatch stopwatch = Stopwatch.createUnstarted();
stopwatch.start();
However after applying this pull request on top of the master branch I see the following:
jclouds-labs/virtualbox/src/main/java/org/jclouds/virtualbox/functions/CreateAndInstallVm.java
Stopwatch stopwatch = new Stopwatch();
stopwatch.start();
I fixed this problem locally and then got the following error:
jclouds-labs/fgcp/src/main/java/org/jclouds/fujitsu/fgcp/suppliers/FGCPCredentialsSupplier.java:[82,68] cannot find symbol
[ERROR] symbol : method privateKeySpec(org.jclouds.io.payloads.StringPayload)
[ERROR] location: class org.jclouds.crypto.Pems
I have a quick question: I was able to fix above two issues locally and test this PR. However when I start a Docker container via this jclouds Docker API, the container starts successfully but it exits soon after. This is the same behaviour as we start a container using docker client without specifying -d option to ask docker to detach it. Do you have any thoughts on this?
Many Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45089212
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Headers(keys = "Content-Type", values = "application/tar")
> + InputStream build(@QueryParam("t") String tag, Payload inputStream);
> +
> + /**
> + * Build an image from Dockerfile via stdin
> + */
> + @Named("image:build")
> + @POST
> + @Path("/build")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Headers(keys = "Content-Type", values = "application/tar")
> + InputStream build(@QueryParam("t") String tag, @QueryParam("q") boolean quiet, @QueryParam("nocache") boolean nocache,
> + @BinderParam(BindInputStreamToRequest.class) File dockerFile);
> +
> +}
Some general considerations on the api class:
* Add the javadoc for all method parameters.
* For those parameters that are optional, better have two method: one that doesn't have them, and another one that encapsulates all them in an Options object, so users don't have to call a method with lots of `null` parameters.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10839625
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.util.Set;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public interface RemoteApi extends Closeable {
> +
> + /**
> + * Get the information of the current docker version.
> + *
> + * @return The information of the current docker version.
> + */
> + @Named("version")
> + @GET
> + @Path("/version")
> + @Consumes(MediaType.APPLICATION_JSON)
Move all `@Consumes` annotation at class level, as it is the one used for all methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838603
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + }
> +
> + @Override
> + public Iterable<Location> listLocations() {
> + return ImmutableSet.of();
> + }
> +
> + @Override
> + public Container getNode(String id) {
> + return api.getRemoteApi().inspectContainer(id);
> + }
> +
> + @Override
> + public void destroyNode(String id) {
> + api.getRemoteApi().stopContainer(id);
> + api.getRemoteApi().removeContainer(id);
don't know :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954591
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #136](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/136/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38272933
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs #974 UNSTABLE
> jclouds-labs-pull-requests #138 UNSTABLE
Thanks for the update, @andreaturli! We now have [one test failure](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/org.apache.jclouds.labs$docker/138/testReport/junit/org.jclouds.docker.compute.functions/ContainerToNodeMetadataTest/testVirtualMachineToNodeMetadata/) and a bunch of [Checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/138/org.apache.jclouds.labs$docker/violations/). Could you address those?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40725751
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + String content = new String(payloadFromResource("/Dockerfile"));
> + File dockerFile = createDockerFile(content);
> + try {
> + try {
> + remoteApi.build(dockerFile, BuildOptions.NONE);
> + fail("Build container should fail on 404");
> + } catch (ResourceNotFoundException ex) {
> + // Expected exception
> + }
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/build");
> + } finally {
> + dockerFile.deleteOnExit();
> + api.close();
> + server.shutdown();
> + }
> + }
Add the corresponding tests for the `build` method that uses a `Payload` instead of a `File`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937961
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Kennedy <no...@github.com>.
> + Map<String, ?> volumes, @Nullable String volumesFrom, @Nullable String workingDir,
> + @Nullable String entrypoint, @Nullable boolean networkDisabled, @Nullable String onBuild) {
> + this.hostname = hostname;
> + this.domainName = domainName;
> + this.user = user;
> + this.memory = checkNotNull(memory, "memory");
> + this.memorySwap = checkNotNull(memorySwap, "memorySwap");
> + this.cpuShares = checkNotNull(cpuShares, "cpuShares");
> + this.attachStdin = checkNotNull(attachStdin, "attachStdin");
> + this.attachStdout = checkNotNull(attachStdout, "attachStdout");
> + this.attachStderr = checkNotNull(attachStderr, "attachStderr");
> + this.exposedPorts = exposedPorts != null ? ImmutableMap.copyOf(exposedPorts) : ImmutableMap.<String, Object> of();
> + this.tty = checkNotNull(tty, "tty");
> + this.openStdin = checkNotNull(openStdin, "openStdin");
> + this.stdinOnce = checkNotNull(stdinOnce, "stdinOnce");
> + this.env = env != null ? ImmutableList.copyOf(cmd) : ImmutableList.<String> of();
Should be `ImmutableList.copyOf(env)` here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12930244
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + * 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 org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +/**
> + * Options to customize container creation.
> + *
> + * @author Andrea Turli
> + */
> +public class CreateImageOptions extends BaseHttpRequestOptions {
> +
> + /*
Remove all this commented code
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937629
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
@imesh Thanks for your feedback.
That's odd because I can see
```
$ mvn dependency:tree|grep guava
[INFO] | \- com.google.guava:guava:jar:17.0:compile
```
and I can build jclouds docker master
Are you using jclouds-docker from another application as @andrewgaul was guessing?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45011359
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + builder.operatingSystem(OperatingSystem.builder().description("linux").family(OsFamily.LINUX).build());
> + return builder.build();
> + }
> +
> + private String cleanUpName(String name) {
> + return name.startsWith("/") ? name.substring(1) : name;
> + }
> +
> + private Iterable<String> getPrivateIpAddresses(Container container) {
> + if (container.getNetworkSettings() == null) return ImmutableList.of();
> + return ImmutableList.of(container.getNetworkSettings().getIpAddress());
> + }
> +
> + private List<String> getPublicIpAddresses() {
> + String dockerIpAddress = URI.create(providerMetadata.getEndpoint()).getHost();
> + return ImmutableList.of(dockerIpAddress);
Is this always true? Is the host sip the only valid public ip address? Or is there any network configuration in Docker that allows the containers to get their own public IP address?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936967
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
> + tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
> + tos.putArchiveEntry(tarEntry);
> + if (!file.isDirectory()) {
> + FileInputStream fin = new FileInputStream(file);
> + BufferedInputStream in = new BufferedInputStream(fin);
> + while ((len = in.read(buf)) != -1) {
> + tos.write(buf, 0, len);
> + }
> + in.close();
> + }
> + tos.closeArchiveEntry();
> + }
> + tos.close();
> + return tarFile;
> + }
This method seems pretty much identical to the previous one. Could you refactor to reuse the code?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936854
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> + .id("")
> + .ram(container.getConfig().getMemory())
> + .processor(new Processor(container.getConfig().getCpuShares(), container.getConfig().getCpuShares()))
> + .build());
> + // TODO Set up location properly
> + LocationBuilder locationBuilder = new LocationBuilder();
> + locationBuilder.description("");
> + locationBuilder.id("");
> + locationBuilder.scope(LocationScope.HOST);
> + builder.location(locationBuilder.build());
> + builder.status(toPortableStatus.apply(container.getState()));
> + builder.imageId(container.getImage());
> + builder.loginPort(getLoginPort(container));
> + builder.publicAddresses(getPublicIpAddresses());
> + builder.privateAddresses(getPrivateIpAddresses(container));
> + builder.operatingSystem(OperatingSystem.builder().description("linux").family(OsFamily.LINUX).build());
unfortunately not, unless we pay an ssh connection to the container. This mean that we should start a container from that image and extract the needed details. I've opened an issue https://github.com/dotcloud/docker/issues/5480 but it has been rejected :(
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953564
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
@nacx I've addressed your comments. Waiting for your feedback here
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40696948
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrew Gaul <no...@github.com>.
> + checkArgument(baseDir.isDirectory(), "%s is not a directory", baseDir);
> + File[] files = baseDir.listFiles();
> + File tarFile = new File(archivePath);
> +
> + String token = getLast(Splitter.on("/").split(archivePath.substring(0, archivePath.lastIndexOf("/"))));
> +
> + byte[] buf = new byte[1024];
> + int len;
> + TarArchiveOutputStream tos = new TarArchiveOutputStream(new FileOutputStream(tarFile));
> + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
> + for (File file : files) {
> + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
> + tarEntry.setName("/" + getLast(Splitter.on(token).split(file.toString())));
> + tos.putArchiveEntry(tarEntry);
> + if (!file.isDirectory()) {
> + FileInputStream fin = new FileInputStream(file);
Too many calls to ```TarArchiveStream.close``` on the successful code path, not guaranteed to call on the exceptional path, and ```TarArchiveStream.closeArchiveEntry``` not called when ```!file.isDirectory```. Otherwise the ```ByteSource``` change is a good improvement.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13241767
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + remoteApi.removeContainer(containerId);
> + assertRequestHasCommonFields(server.takeRequest(), "DELETE", "/containers/"+containerId);
> + } finally {
> + api.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testRemoveNonExistingContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(404));
> + DockerApi api = api(server.getUrl("/"));
> + RemoteApi remoteApi = api.getRemoteApi();
> + String containerId = "nonExisting";
> + try {
> + remoteApi.removeContainer(containerId);
Add a `fail` to fail the test if the `removeContainer` does not throw the exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938164
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1158](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1158/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45945956
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> + <parent>
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>jclouds-labs</artifactId>
> + <version>1.8.0-SNAPSHOT</version>
> + </parent>
> +
> + <!-- TODO: when out of labs, switch to org.jclouds.provider -->
> + <groupId>org.apache.jclouds.labs</groupId>
> + <artifactId>docker</artifactId>
> + <name>jclouds docker API</name>
> + <description>ComputeService binding to the Docker v1.8 API</description>
> + <packaging>bundle</packaging>
> +
> + <properties>
> + <test.docker.endpoint>https://localhost:4243</test.docker.endpoint>
> + <test.docker.api-version>1.8</test.docker.api-version>
Align this property with the one in the api metadata
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r11264906
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Ignasi Barrera <no...@github.com>.
> +import static com.google.common.collect.Iterables.getOnlyElement;
> +
> +/**
> + * @author Andrea Turli
> + */
> +public class ContainerToNodeMetadata implements Function<Container, NodeMetadata> {
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final ApiContext<DockerApi> context;
> +
> + @Inject
> + public ContainerToNodeMetadata(ApiContext<DockerApi> context) {
> + this.context = context;
Make sure parameter is not null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10838960
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #205](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/205/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47840689
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #886](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/886/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-38273289
Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)
Posted by Andrea Turli <no...@github.com>.
> +
> + @Inject
> + public DockerComputeServiceAdapter(DockerApi api, ApiContext<DockerApi> context) {
> + this.api = checkNotNull(api, "api");
> + this.context = context;
> + }
> +
> + @Override
> + public NodeAndInitialCredentials<Container> createNodeWithGroupEncodedIntoName(String group, String name,
> + Template template) {
> + checkNotNull(template, "template was null");
> + checkNotNull(template.getOptions(), "template options was null");
> +
> + String imageId = checkNotNull(template.getImage().getId(), "template image id must not be null");
> + String defaultIdentity = context.getProviderMetadata().getApiMetadata().getDefaultIdentity().get();
> + String defaultCredential = context.getProviderMetadata().getApiMetadata().getDefaultCredential().get();
Thanks for clarifying that!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r10845140