nacx requested changes on this pull request.
Thanks @alibazlamit!
I've added some comments. Let's address them first, and then we'll fix the live
tests once we are happy with the implementation.
> @@ -32,6 +32,7 @@
<packaging>bundle</packaging>
<properties>
+ <checkstyle.skip>true</checkstyle.skip>
Remove this
> @@ -49,10 +50,21 @@ public OneAndOneProviderMetadata(Builder builder) {
public static Properties defaultProperties() {
Properties properties = OneAndOneApiMetadata.defaultProperties();
+
+ properties.setProperty(PROPERTY_REGIONS, "de,us,es,gb");
+// properties.setProperty(PROPERTY_ISO3166_CODES, "DE,US,ES,GB");
This one is not needed if you set the codes in the provider metadata, so it
could be removed.
> @@ -49,10 +50,21 @@ public OneAndOneProviderMetadata(Builder builder) {
public static Properties defaultProperties() {
Properties properties = OneAndOneApiMetadata.defaultProperties();
+
+ properties.setProperty(PROPERTY_REGIONS, "de,us,es,gb");
+// properties.setProperty(PROPERTY_ISO3166_CODES, "DE,US,ES,GB");
+// properties.setProperty(PROPERTY_REGION + ".de." + ISO3166_CODES,
"DE-DEU");
+// properties.setProperty(PROPERTY_REGION + ".us." + ISO3166_CODES,
"US-USA");
+// properties.setProperty(PROPERTY_REGION + ".es." + ISO3166_CODES,
"ES-ESP");
+// properties.setProperty(PROPERTY_REGION + ".gb." + ISO3166_CODES,
"GB-GBR");
These ones qualify the iso code for each region, so they should be added back.
> +import org.jclouds.domain.LoginCredentials;
+import org.jclouds.logging.Logger;
+import org.jclouds.rest.ResourceNotFoundException;
+
+@Singleton
+public class OneandoneComputeServiceAdapter implements
ComputeServiceAdapter<Server, HardwareFlavour, Image, Location> {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final OneAndOneApi api;
+ private final Predicate<Server> waitServerUntilAvailable;
+ private final List<DataCenter> datacetners;
+
+// private static final Integer DEFAULT_LAN_ID = 1;
Remove this
> +import org.jclouds.domain.Location;
+import org.jclouds.domain.LocationScope;
+import org.jclouds.domain.LoginCredentials;
+import org.jclouds.logging.Logger;
+import org.jclouds.rest.ResourceNotFoundException;
+
+@Singleton
+public class OneandoneComputeServiceAdapter implements
ComputeServiceAdapter<Server, HardwareFlavour, Image, Location> {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final OneAndOneApi api;
+ private final Predicate<Server> waitServerUntilAvailable;
+ private final List<DataCenter> datacetners;
Delete this unused variable
> + private final OneAndOneApi api;
+ private final Predicate<Server> waitServerUntilAvailable;
+ private final List<DataCenter> datacetners;
+
+// private static final Integer DEFAULT_LAN_ID = 1;
+ @Inject
+ OneandoneComputeServiceAdapter(OneAndOneApi api,
+ @Named(POLL_PREDICATE_SERVER) Predicate<Server>
waitServerUntilAvailable) {
+ this.api = api;
+ this.datacetners = ImmutableList.of();
+ this.waitServerUntilAvailable = waitServerUntilAvailable;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Server>
createNodeWithGroupEncodedIntoName(String group, String name, Template
template) {
+ checkArgument(template.getLocation().getScope() == LocationScope.REGION,
"Template must use a REGION-scoped location");
Only available locations will get here, so this check is redundant. Let's
better remove it.
> + .hardware(hardwareRequest)
+ .applianceId(image.getId())
+ .dataCenterId(dataCenterId)
+ .password(password)
+ .powerOn(Boolean.TRUE).build();
+
+ logger.trace("<< provisioning server '%s'", serverRequest);
+
+ server = api.serverApi().create(serverRequest);
+ logger.trace(">> provisioning complete for server. returned id='%s'",
server.id());
+
+ } catch (Exception ex) {
+ logger.error(ex, ">> failed to provision server. rollbacking..");
+ throw Throwables.propagate(ex);
+ }
+ waitServerUntilAvailable.apply(server);
There is no need to wait here. Jclouds will already wait for the server to be
running, so let's better remove this.
> + }
+
+ // provision server
+ final Server server;
+ Double cores = ComputeServiceUtils.getCores(hardware);
+
+ try {
+ List<? extends Processor> processors = hardware.getProcessors();
+ org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware
hardwareRequest
+ =
org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware.create(processors.size(),
cores, hardware.getRam(), hdds);
+ final Server.CreateServer serverRequest =
Server.CreateServer.builder()
+ .name(name)
+ .hardware(hardwareRequest)
+ .applianceId(image.getId())
+ .dataCenterId(dataCenterId)
+ .password(password)
Is it mandatory to force a password login? What about ssh key access?
> + }
+
+ @Override
+ public Iterable<Location> listLocations() {
+ // Will never be called
+ throw new UnsupportedOperationException("Locations are configured in
jclouds properties");
+ }
+
+ @Override
+ public Server getNode(String id) {
+ return api.serverApi().get(id);
+ }
+
+ @Override
+ public void destroyNode(String id) {
+ waitServerUntilAvailable.apply(getNode(id));
Is this really needed here and in the next state-change methods?
> +import org.jclouds.compute.domain.internal.TemplateBuilderImpl;
+import org.jclouds.domain.Location;
+import static org.jclouds.util.Predicates2.retry;
+
+public class OneAndOneComputeServiceContextModule extends
+ ComputeServiceAdapterContextModule<Server, HardwareFlavour,
org.apache.jclouds.oneandone.rest.domain.Image, Location> {
+
+ @SuppressWarnings("unchecked")
+ @Override
+ protected void configure() {
+ super.configure();
+
+ bind(new TypeLiteral<ComputeServiceAdapter<Server, HardwareFlavour,
org.apache.jclouds.oneandone.rest.domain.Image, Location>>() {
+ }).to(OneandoneComputeServiceAdapter.class);
+
+
bind(TemplateBuilderImpl.class).to(ArbitraryCpuRamTemplateBuilderImpl.class);
Can users set custom CPU/RAM values even if there are no such predefined
hardware flavours in OneAndOne? If not, remove this binding.
> + public void destroyNode(String id) {
+ waitServerUntilAvailable.apply(getNode(id));
+ logger.debug("Destroying %s ...", id);
+ api.serverApi().delete(id);
+ logger.debug("Destroyed %s!", id);
+ }
+
+ @Override
+ public void rebootNode(String id) {
+ waitServerUntilAvailable.apply(getNode(id));
+ api.serverApi().updateStatus(id,
Server.UpdateStatus.create(Types.ServerAction.REBOOT,
Types.ServerActionMethod.HARDWARE));
+ }
+
+ @Override
+ public void resumeNode(String id) {
+ waitServerUntilAvailable.apply(getNode(id));
This won't work? You wait until the node is powered on when you want to resume
it. This will take forever.
> + return OperatingSystem.builder()
+ .description(OsFamily.LINUX.value())
+ .family(OsFamily.LINUX)
+ .build();
+ default:
+ break;
+ }
+ }
+ return OperatingSystem.builder()
+ .description(OsFamily.UNRECOGNIZED.value())
+ .family(OsFamily.UNRECOGNIZED)
+ .build();
+ }
+
+ private boolean is64Bit(int architecture) {
+ switch (architecture) {
Just `return architecture != 32;`
> +import com.google.common.base.Function;
+import com.google.common.collect.ImmutableList;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.jclouds.oneandone.rest.domain.HardwareFlavour;
+import org.jclouds.compute.domain.Hardware;
+import org.jclouds.compute.domain.HardwareBuilder;
+import org.jclouds.compute.domain.Processor;
+import org.jclouds.compute.domain.Volume;
+import org.jclouds.compute.domain.VolumeBuilder;
+
+public class HardwareFlavourToHardware implements Function<HardwareFlavour,
Hardware> {
+
+ @Override
+ public Hardware apply(HardwareFlavour from) {
+ int MinRamSize = (int) from.hardware().ram();
Start variable name in lowercase
> + @Override
+ public NodeMetadata apply(final Server server) {
+ checkNotNull(server, "Null server");
+
+ DataCenter dataCenter =
api.dataCenterApi().get(server.datacenter().id());
+ Location location = find(locations.get(),
idEquals(dataCenter.location()));
+
+ double size = 0d;
+ List<Volume> volumes = Lists.newArrayList();
+ List<Hdd> storages = server.hardware().hdds();
+ if (storages != null) {
+ for (Hdd storage : storages) {
+ size += storage.size();
+ volumes.add(fnVolume.apply(storage));
+ }
+ }
Always building a custom hardware object? This seems not right. You should take
into account prefedined flavours.
> + }
+
+ // Build hardware
+ String id = String.format("cpu=%f,ram=%d,disk=%f",
server.hardware().coresPerProcessor(), (int) server.hardware().ram(), size);
+ Hardware hardware = new HardwareBuilder()
+ .ids(id)
+ .name(id)
+ .ram((int) server.hardware().ram())
+ .processor(new Processor(server.hardware().coresPerProcessor(),
1d))
+ .hypervisor("kvm")
+ .volumes(volumes)
+ .location(location)
+ .build();
+
+ // Collect ips
+ List<String> addresses = fnCollectIps.apply(server.ips());
No need for that dummy function in the constructor. Remove it and change this
to something like:
```java
List<String> addresses = Lists.transform(new Function<ServerIp, String>() {
@Override public String apply(ServerIp in) {
return in.ip();
}
});
```
> + case DEPLOYING:
+ case POWERING_OFF:
+ case POWERING_ON:
+ case REBOOTING:
+ case REMOVING:
+ return NodeMetadata.Status.PENDING;
+ case POWERED_OFF:
+ return NodeMetadata.Status.SUSPENDED;
+ case POWERED_ON:
+ return NodeMetadata.Status.RUNNING;
+ default:
+ return NodeMetadata.Status.UNRECOGNIZED;
+ }
+ }
+
+ static OperatingSystem mapOsType(OSFamliyType osType) {
This is a duplicate of the one in the Image transformation function. Reuse it.
> return retry(new PrivateNetworkReadyPredicate(
api),
constants.pollTimeout(), constants.pollPeriod(),
constants.pollMaxPeriod(), TimeUnit.SECONDS);
}
@Provides
- @Named(POLL_PREDICATE_SERVER)
- Predicate<Server> provideServerReadyPredicate(final OneAndOneApi api,
OneAndOneConstants constants) {
+ Predicate<Server> provideServerReadyPredicate(final OneAndOneApi api,
ComputeConstants constants) {
Do we have now two different predicates to check if a server is ready?
> + * Unless required by applicable law or agreed to in writing, software
+ * 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.apache.jclouds.oneandone.rest.domain;
+
+public enum Location {
+
+ DE("de", "Germany"),
+ US("us", "USA"),
+ ES("es", "Spain"),
+ GB("gb", "UNITED KINGDOM"),
+ UNRECOGNIZED("unrecognized", "Unrecognized location"),
+ MOCK("mock", "Mock");
What are the unrecognized and mock locations for? Can we remove them?
> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * 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.apache.jclouds.oneandone.rest.domain;
+
+import com.google.common.base.Enums;
+
+/**
+ * Marker interface for {@link org.jclouds.profitbricks.domain.Image} and
+ * {@link org.jclouds.profitbricks.domain.Snapshot}
+ */
+public interface Provisionable {
Remove this class.
> + @Override
+ protected Module getSshModule() {
+ return new SshjSshClientModule();
+ }
+
+ @Override
+ protected LoggingModule getLoggingModule() {
+ return new SLF4JLoggingModule();
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+ modules.addAll(super.setupModules());
+ return modules.build();
+ }
Remove this method.
> + @Override
+ protected LoggingModule getLoggingModule() {
+ return new SLF4JLoggingModule();
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+ modules.addAll(super.setupModules());
+ return modules.build();
+ }
+
+ @Override
+ public void testOptionToNotBlock() throws Exception {
+ // OneAndOne implementation intentionally blocks until the node is
'AVAILABLE'
+ }
There should be no need for that, so this method could be removed.
> + }
+
+ @Override
+ protected void checkUserMetadataContains(NodeMetadata node,
ImmutableMap<String, String> userMetadata) {
+ // OneAndOne doesn't support user metadata
+ }
+
+ @Override
+ protected void checkResponseEqualsHostname(ExecResponse execResponse,
NodeMetadata node1) {
+ // OneAndOne doesn't support hostname
+ }
+
+ @Override
+ protected void checkOsMatchesTemplate(NodeMetadata node) {
+ // Not enough description from API to match template
+ }
Can you provide an example response for the getImage call? Isn't anything there
we can use to determine the OS?
> +
+ public OneAndOneTemplateBuilderLiveTest() {
+ this.provider = "oneandone";
+ }
+
+ @Override
+ protected Set<String> getIso3166Codes() {
+ return ImmutableSet.of("DE-DEU", "US-USA", "ES-ESP", "GB-GBR");
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ ImmutableSet.Builder<Module> modules = ImmutableSet.builder();
+ modules.addAll(super.setupModules());
+ return modules.build();
+ }
Remove this method.
> + // Destroy all nodes in the group but also make sure to destroy
> other created nodes that might not be in it.
+ // The "testCreateTwoNodesWithOneSpecifiedName" creates nodes with
an explicit name that puts them outside the group,
+ // so the list of nodes should also be taken into account when
destroying the nodes.
+
client.destroyNodesMatching(Predicates.<NodeMetadata>or(inGroup(group),
(Predicate<NodeMetadata>) in(nodes)));
+ }
+ } catch (Exception e) {
+
+ }
+ super.tearDownContext();
+ }
+
+ @Override
+ protected Module getSshModule() {
+ throw new IllegalStateException("ssh is required for this test!");
+ }
+}
Remove this file.
> +import static com.google.common.collect.Iterables.getOnlyElement;
+import com.google.inject.Module;
+import static org.assertj.core.api.Assertions.assertThat;
+import org.jclouds.compute.domain.ExecResponse;
+import org.jclouds.compute.domain.NodeMetadata;
+import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.internal.BaseComputeServiceLiveTest;
+import static org.jclouds.compute.predicates.NodePredicates.inGroup;
+import org.jclouds.logging.config.LoggingModule;
+import org.jclouds.logging.slf4j.config.SLF4JLoggingModule;
+import org.jclouds.sshj.config.SshjSshClientModule;
+import org.testng.annotations.Test;
+
+@Test(groups = "live", singleThreaded = true, testName =
"OneAndOneComputeServiceLiveTest")
+public class OneAndOneComputeServiceLiveTest extends
BaseComputeServiceLiveTest {
+// public class OneAndOneComputeServiceLiveTest extends baseclass {
Remove this comment
> +
+ private DataCenterApi dataCenterApi() {
+
+ return api.dataCenterApi();
+ }
+
+ @Test
+ public void testList() {
+ dataCenters = dataCenterApi().list();
+ currentDataCenter = dataCenters.get(0);
+ assertNotNull(dataCenters);
+ assertFalse(dataCenters.isEmpty());
+ Assert.assertTrue(dataCenters.size() > 0);
+ }
+
+ @Test(dependsOnMethods = "testList")
No need for this dependency
--
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/338#pullrequestreview-15879083
