Re: [jclouds/jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
Closed #929. --- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929#event-647082738
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
Whew. Now I'm relieved that this is not a *bugfoot*. After attaching the debugger one more time, I found out that the `AuthenticationException` was caused by the `ProfitBricksComputeServiceAdapter` [generating a password](https://github.com/devcsrj/jclouds/commit/1391384dd5d58e5ccbe91ca4f5fb8c0f63800fb8#diff-6af74fe6e33ae1d12faae140fee688d2L118), if none is provided. This doesn't make sense to snapshots. The latest commit sets loginUser and password as null for snapshots, so that [no `LoginCredentials` is created](https://github.com/devcsrj/jclouds/commit/1391384dd5d58e5ccbe91ca4f5fb8c0f63800fb8#diff-6af74fe6e33ae1d12faae140fee688d2R241). ``` testSpawnNodeFromImage(org.jclouds.profitbricks.compute.extensions.ProfitBricksImageExtensionLiveTest) Time elapsed: 329.445 sec <<< FAILURE! java.lang.NullPointerException: no credentials found for node 81a0ee10-7c64-4129-bf8a-68c14afc8cd7 at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:253) at org.jclouds.compute.functions.CreateSshClientOncePortIsListeningOnNode.apply(CreateSshClientOncePortIsListeningOnNode.java:62) at org.jclouds.compute.functions.CreateSshClientOncePortIsListeningOnNode.apply(CreateSshClientOncePortIsListeningOnNode.java:40) at org.jclouds.compute.extensions.internal.BaseImageExtensionLiveTest.checkReachable(BaseImageExtensionLiveTest.java:167) at org.jclouds.compute.extensions.internal.BaseImageExtensionLiveTest.testSpawnNodeFromImage(BaseImageExtensionLiveTest.java:136) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85) at org.testng.internal.Invoker.invokeMethod(Invoker.java:696) at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:882) at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1189) at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124) at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) at java.lang.Thread.run(Thread.java:745) ``` My question, what's the cleanest approach to set the credentials for the newly created image? Or should the [test method](https://github.com/jclouds/jclouds/blob/master/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseImageExtensionLiveTest.java#L134) be updated? --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929#issuecomment-191569462
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
I also get authentication failures. The override login credentials provided in the command line when running the tests seem to apply when creating the server, but not when creating the server from the created image. This should be fixed before merging. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929#issuecomment-191299261
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
Just two minors. lgtm! --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929#issuecomment-191285680
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> + public ListenableFuture createImage(ImageTemplate template) {
> + checkState(template instanceof CloneImageTemplate, "profitbricks only
> supports creating images through cloning.");
> + final CloneImageTemplate cloneTemplate = (CloneImageTemplate) template;
> + String serverId = cloneTemplate.getSourceNodeId();
> +
> + Server server = api.serverApi().getServer(serverId);
> + Storage bootDevice = Iterables.find(server.storages(),
> matchBootDevice);
> +
> + final Snapshot requested = api.snapshotApi().createSnapshot(
> + Snapshot.Request.creatingBuilder()
> + .storageId(bootDevice.id())
> + .name(template.getName())
> + .description(template.getName() + " (created with jclouds)")
> + .build());
> +
> + logger.info(">> creating a snapshot from storage: " + bootDevice.id());
Better use the form `logger.info(">> creating a snapshot from storage: %s",
bootDevice.id());`. You save unnecessary string builders if the log level is
not active.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54737196
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> +import org.jclouds.profitbricks.ProfitBricksApi;
> +import org.jclouds.profitbricks.domain.Provisionable;
> +import org.jclouds.profitbricks.domain.Server;
> +import org.jclouds.profitbricks.domain.Snapshot;
> +import org.jclouds.profitbricks.domain.Storage;
> +
> +import com.google.common.base.Function;
> +import com.google.common.base.Predicate;
> +import com.google.common.collect.Iterables;
> +import com.google.common.util.concurrent.ListenableFuture;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +import com.google.common.util.concurrent.UncheckedTimeoutException;
> +import com.google.inject.Inject;
> +
> +@Singleton
> +public class ProfitBricksImageExtension implements ImageExtension {
Let's add a `@Beta` annotation here too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54737057
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> ++ cloneTemplate.getName());
> + }
> + });
> + }
> +
> + @Override
> + public boolean deleteImage(String id) {
> + Snapshot snapshot = api.snapshotApi().getSnapshot(id);
> + if (snapshot != null)
> + try {
> +logger.debug(">> deleting snapshot %s..", id);
> +return api.snapshotApi().deleteSnapshot(id);
> + } catch (Exception ex) {
> +logger.error(ex, ">> error deleting snapshot %s..", id);
> + }
> + return true;
Oh. Nice catch!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54726582
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> + @Override
> + public ListenableFuture createImage(ImageTemplate template) {
> + checkState(template instanceof CloneImageTemplate, "profitbricks only
> supports creating images through cloning.");
> + final CloneImageTemplate cloneTemplate = (CloneImageTemplate) template;
> + String serverId = cloneTemplate.getSourceNodeId();
> +
> + Server server = api.serverApi().getServer(serverId);
> + Storage bootDevice = Iterables.find(server.storages(),
> matchBootDevice);
> +
> + final Snapshot requested = api.snapshotApi().createSnapshot(
> + Snapshot.Request.creatingBuilder()
> + .storageId(bootDevice.id())
> + .name(template.getName())
> + .description(template.getName() + " (created with jclouds)")
> + .build());
> +
Since this is something that takes time, add a log message here saying that the
image is being created.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54725844
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> public static final String TIMEOUT_DATACENTER_AVAILABLE = > "jclouds.profitbricks.timeout.datacenter-available"; > - public static final String POLL_INITIAL_PERIOD = > "jclouds.profitbricks.poll-status.initial-period"; > - public static final String POLL_MAX_PERIOD = > "jclouds.profitbricks.poll-status.poll.max-period"; > + public static final String TIMEOUT_SNAPSHOT_AVAILABLE= > "jclouds.profitbricks.timeout.snapshot-available"; Already commented, remove this property. --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929/files#r54725870
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> ++ cloneTemplate.getName());
> + }
> + });
> + }
> +
> + @Override
> + public boolean deleteImage(String id) {
> + Snapshot snapshot = api.snapshotApi().getSnapshot(id);
> + if (snapshot != null)
> + try {
> +logger.debug(">> deleting snapshot %s..", id);
> +return api.snapshotApi().deleteSnapshot(id);
> + } catch (Exception ex) {
> +logger.error(ex, ">> error deleting snapshot %s..", id);
> + }
> + return true;
Should this better be a `return false`, to cover the case when an official
Image is provided and also cover the return value with the `catch` block is
executed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54725804
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> + @Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService
> userExecutor) {
> + this.api = api;
> + this.snapshotAvailablePredicate = snapshotAvailablePredicate;
> + this.imageTransformer = imageTransformer;
> + this.userExecutor = userExecutor;
> + }
> +
> + @Override
> + public ImageTemplate buildImageTemplateFromNode(String name, String id) {
> + Server server = api.serverApi().getServer(id);
> +
> + if (server == null)
> + throw new NoSuchElementException("Cannot find server with id: " +
> id);
> +
> + List storages = server.storages();
> + if (storages.isEmpty() || !Iterables.tryFind(storages,
> matchBootDevice).isPresent())
`storages.isEmpty()` is redundant, you could remove it. Perhaps it reads better
as: `if (!Iterables.any(storages, matchBootDevice))
`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54725423
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> @@ -132,11 +135,11 @@ ProvisioningManager provideProvisioningManager(Closer
> closer) {
>
> @Provides
> @Singleton
> - @Named(POLL_PREDICATE_SNAPSHOT)
> - Predicate provideSnapshotAvailablePredicate(final ProfitBricksApi
> api, ComputeConstants constants) {
> + @Named(TIMEOUT_SNAPSHOT_AVAILABLE)
Better use the existing `ComputeServiceProperties.TIMEOUT_IMAGE_AVAILABLE`
property.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54725109
Re: [jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
> +
> + private final Predicate matchBootDevice = new
> Predicate() {
> + @Override
> + public boolean apply(Storage input) {
> + return input.bootDevice() == null ? false : input.bootDevice();
> + }
> + };
> +
> + private final ProfitBricksApi api;
> + private final Predicate snapshotAvailablePredicate;
> + private final Function imageTransformer;
> + private final ListeningExecutorService userExecutor;
> +
> + @Inject
> + ProfitBricksImageExtension(ProfitBricksApi api,
> + @Named(TIMEOUT_SNAPSHOT_AVAILABLE) Predicate
> snapshotAvailablePredicate,
Same about the image available property.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/929/files#r54725159
[jclouds] JCLOUDS-1078: Implement ImageExtension in ProfitBricks (#929)
This PR mainly implements ImageExtension (Snapshots) in ProfitBricks. Some of the compute service property names were renamed as well, to reflect more closely the jclouds convention. The live tests are failing though, with the exact stack traces from [JCLOUDS-1058](https://issues.apache.org/jira/browse/JCLOUDS-1058). Had tried ubuntu-15.10 on `de/fkb` and `de/fra` so far. I have yet to try for `us/las` and `us/lasdev` and see if I can get pass this error. ``` mvn clean install -Plive -Dtest.profitbricks.identity= -Dtest.profitbricks.credential= -Dtest.profitbricks.template="imageId=,loginUser=root:" -Dtest=org.jclouds.profitbricks.compute.extensions.ProfitBricksImageExtensionLiveTest ``` You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds/pull/929 -- Commit Summary -- * JCLOUDS-1078: Implement ImageExtension in ProfitBricks -- File Changes -- M providers/profitbricks/src/main/java/org/jclouds/profitbricks/ProfitBricksProviderMetadata.java (17) M providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/ProfitBricksComputeServiceAdapter.java (4) M providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/concurrent/ProvisioningJob.java (4) M providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/config/ProfitBricksComputeServiceContextModule.java (59) A providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/extensions/ProfitBricksImageExtension.java (139) M providers/profitbricks/src/main/java/org/jclouds/profitbricks/compute/strategy/AssignDataCenterToTemplate.java (4) M providers/profitbricks/src/main/java/org/jclouds/profitbricks/config/ProfitBricksComputeProperties.java (6) M providers/profitbricks/src/test/java/org/jclouds/profitbricks/BaseProfitBricksLiveTest.java (8) A providers/profitbricks/src/test/java/org/jclouds/profitbricks/compute/extensions/ProfitBricksImageExtensionLiveTest.java (37) -- Patch Links -- https://github.com/jclouds/jclouds/pull/929.patch https://github.com/jclouds/jclouds/pull/929.diff --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/929
