jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Pb image extension (#349)
Date Thu, 26 Jan 2017 22:01:39 GMT
nacx requested changes on this pull request.

I've commented on the image extension commit.

Regarding the SSH credentials, if not provided, is it possible that the compute service is
generating a new password for the node when creating it from the image, but that node (being
a snapshot), is not executing the init logic and is not getting the new password injected?
Does it work if you try to manually access the newly created node using the credentials from
the original one?

> +import org.jclouds.logging.Logger;
+
+public class ProfitBricksImageExtension implements ImageExtension {
+
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final ProfitBricksApi client;
+   private final ListeningExecutorService userExecutor;
+   private final Supplier<Set<? extends Location>> locations;
+   private final Predicate<String> snapshotAvailablePredicate;
+   private final Trackables trackables;
+
+   @Inject
+   public ProfitBricksImageExtension(ProfitBricksApi client,

Remove the public modifier, and all null checks, since the injector already enforces that.

> +           @Memoized Supplier<Set<? extends Location>> locations,
+           @Named(POLL_PREDICATE_SNAPSHOT) Predicate<String> snapshotAvailablePredicate,
+           Trackables trackables) {
+      this.client = checkNotNull(client, "client");
+      this.userExecutor = checkNotNull(userExecutor, "userExecutor");
+      this.locations = checkNotNull(locations, "locations");
+      this.snapshotAvailablePredicate = checkNotNull(snapshotAvailablePredicate, "snapshotAvailablePredicate");
+      this.trackables = checkNotNull(trackables);
+   }
+
+   @Override
+   public ImageTemplate buildImageTemplateFromNode(String name, String id) {
+      DataCenterAndId datacenterAndId = DataCenterAndId.fromSlashEncoded(id);
+      Server server = client.serverApi().getServer(datacenterAndId.getDataCenter(), datacenterAndId.getId());
+      if (server == null) {
+         throw new NoSuchElementException("Cannot find server with id: " + id);

Better throw an `IllegalArgumentException`.

> +      return template;
+   }
+
+   @Override
+   public ListenableFuture<Image> createImage(ImageTemplate template) {
+      checkState(template instanceof CloneImageTemplate,
+              " cloudstack only currently supports creating images through cloning.");
+      CloneImageTemplate cloneTemplate = (CloneImageTemplate) template;
+      DataCenterAndId datacenterAndId = DataCenterAndId.fromSlashEncoded(cloneTemplate.getSourceNodeId());
+
+      Server server = client.serverApi().getServer(datacenterAndId.getDataCenter(), datacenterAndId.getId());
+      List<Volume> volumes = client.volumeApi().getList(server.dataCenterId());
+
+      Volume volume = Iterables.getOnlyElement(volumes);
+
+      Snapshot snapshot = client.volumeApi().createSnapshot(Volume.Request.createSnapshotBuilder()

Let's wrap all the volume snapshot creation inside the future, so this method can return early.
If it returns a future, then, let's do all the job to create the snapshot asynchronously.
The operations to get the server and volume are ok outside the future, since they can be seen
as validation and it's better if we can fail synchronously if the preconditions are not met.

> + * 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.apache.jclouds.profitbricks.rest.compute.extensions;
+
+import com.google.inject.Module;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.extensions.internal.BaseImageExtensionLiveTest;
+import org.jclouds.sshj.config.SshjSshClientModule;
+import org.testng.annotations.Test;
+
+@Test(groups = "live", singleThreaded = true, testName = "ProfitBricksImageExtensionLiveTest")
+public class ProfitBricksImageExtensionLiveTest extends BaseImageExtensionLiveTest {
+
+   protected ComputeService client;

Remove this?

> @@ -0,0 +1,206 @@
+/*

Remove this class.

-- 
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-labs/pull/349#pullrequestreview-18733700
Mime
View raw message