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 2015/02/13 15:18:46 UTC
[jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
improve DeploymentToNodeMetadata
enhance Deployment value object
add support for get/set NetworkConfiguration
added AzureComputeTemplateOptions to manage networkConfigurations
add support for create/checkAvailable/list storageAccounts
add support for inboundPorts
add more RoleSize
add support for SecurityGroupExtension by using NetworkSecurityGroup
fix destroy node
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/135
-- Commit Summary --
* AzureCompute: initial work to support ComputeServiceAdapter
-- File Changes --
M azurecompute/pom.xml (12)
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureComputeApi.java (36)
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureComputeProviderMetadata.java (2)
M azurecompute/src/main/java/org/jclouds/azurecompute/AzureManagementApiMetadata.java (13)
M azurecompute/src/main/java/org/jclouds/azurecompute/binders/DeploymentParamsToXML.java (27)
A azurecompute/src/main/java/org/jclouds/azurecompute/binders/NetworkConfigurationToXML.java (62)
A azurecompute/src/main/java/org/jclouds/azurecompute/binders/NetworkSecurityGroupToXML.java (45)
A azurecompute/src/main/java/org/jclouds/azurecompute/binders/RoleToXML.java (73)
A azurecompute/src/main/java/org/jclouds/azurecompute/binders/RuleToXML.java (46)
A azurecompute/src/main/java/org/jclouds/azurecompute/binders/StorageServiceParamsToXML.java (49)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapter.java (335)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/config/AzureComputeServiceContextModule.java (31)
A azurecompute/src/main/java/org/jclouds/azurecompute/compute/extensions/AzureComputeSecurityGroupExtension.java (460)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/DeploymentToNodeMetadata.java (87)
A azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/LocationToLocation.java (70)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/OSImageToImage.java (18)
M azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/RoleSizeToHardware.java (36)
A azurecompute/src/main/java/org/jclouds/azurecompute/compute/functions/internal/OperatingSystems.java (83)
A azurecompute/src/main/java/org/jclouds/azurecompute/compute/strategy/UseNodeCredentialsButOverrideFromTemplate.java (57)
A azurecompute/src/main/java/org/jclouds/azurecompute/config/AzureComputeConstants.java (23)
M azurecompute/src/main/java/org/jclouds/azurecompute/config/AzureComputeHttpApiModule.java (24)
A azurecompute/src/main/java/org/jclouds/azurecompute/config/AzureComputeParserModule.java (27)
M azurecompute/src/main/java/org/jclouds/azurecompute/config/AzureComputeProperties.java (3)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/DeploymentApi.java (1)
A azurecompute/src/main/java/org/jclouds/azurecompute/features/NetworkSecurityGroupApi.java (167)
A azurecompute/src/main/java/org/jclouds/azurecompute/features/StorageAccountApi.java (102)
A azurecompute/src/main/java/org/jclouds/azurecompute/features/SubscriptionApi.java (54)
M azurecompute/src/main/java/org/jclouds/azurecompute/features/VirtualMachineApi.java (36)
A azurecompute/src/main/java/org/jclouds/azurecompute/features/VirtualNetworkApi.java (74)
A azurecompute/src/main/java/org/jclouds/azurecompute/options/AzureComputeTemplateOptions.java (324)
A azurecompute/src/main/java/org/jclouds/azurecompute/util/NetworkSecurityGroups.java (73)
M azurecompute/src/main/java/org/jclouds/azurecompute/xml/StorageServiceHandler.java (2)
A azurecompute/src/test/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapterLiveTest.java (139)
A azurecompute/src/test/java/org/jclouds/azurecompute/compute/AzureComputeServiceContextLiveTest.java (99)
A azurecompute/src/test/java/org/jclouds/azurecompute/compute/extensions/AzureComputeSecurityGroupExtensionLiveTest.java (57)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/CloudServiceApiMockTest.java (55)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DeploymentApiMockTest.java (39)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/DiskApiMockTest.java (36)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/LocationApiMockTest.java (17)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/OSImageApiMockTest.java (41)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/OperationApiMockTest.java (18)
A azurecompute/src/test/java/org/jclouds/azurecompute/features/SubscriptionApiLiveTest.java (42)
M azurecompute/src/test/java/org/jclouds/azurecompute/features/VirtualMachineApiMockTest.java (8)
A azurecompute/src/test/java/org/jclouds/azurecompute/features/VirtualNetworkApiLiveTest.java (81)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/BaseAzureComputeApiLiveTest.java (2)
M azurecompute/src/test/java/org/jclouds/azurecompute/internal/BaseAzureComputeApiMockTest.java (8)
M docker/src/test/resources/logback.xml (2)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/135.patch
https://github.com/jclouds/jclouds-labs/pull/135.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsInLocation(final Location location) {
> + return FluentIterable.from(api.getNetworkSecurityGroupApi().list())
> + .transform(new NetworkSecurityGroupSecurityGroupFunction())
> + .toSet();
> + }
> +
> + /**
> + * @param name it represents both cloudservice and deployment name
> + * @return Set<SecurityGroup>
> + */
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsForNode(String name) {
> + checkNotNull(name, "name");
> +
> + final String virtualNetworkName;
Create the variable below. Don't create it here uninitialized.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977093
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private NetworkConfiguration originalNetworkConfiguration;
> + private Predicate<String> operationSucceeded;
> +
> + @BeforeClass
> + public void init() {
> + operationSucceeded = retry(new Predicate<String>() {
> + public boolean apply(String input) {
> + return api.getOperationApi().get(input).status() == Operation.Status.SUCCEEDED;
> + }
> + }, 600, 5, 5, SECONDS);
> +
> + originalNetworkConfiguration = api().get();
> + }
> +
> + @AfterClass
Always tear down
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978540
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + builder.ids(from.name());
> + builder.name(from.name());
> + builder.hostname(getHostname(from));
> + /* TODO
> + if (from.getDatacenter() != null) {
> + builder.location(from(locations.get()).firstMatch(
> + LocationPredicates.idEquals(from.getDatacenter().getId() + "")).orNull());
> + }
> + builder.group(nodeNamingConvention.groupInUniqueNameOrNull(from.getHostname()));
> + builder.hardware(roleSizeToHardware.apply(from.instanceSize()));
> + Image image = osImageToImage.apply(from);
> + if (image != null) {
> + builder.imageId(image.getId());
> + builder.operatingSystem(image.getOperatingSystem());
> + }
> + */
It is important to have the location and image set. Can you uncomment this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977411
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -57,16 +63,17 @@ public static Properties defaultProperties() {
> protected Builder() {
> id("azurecompute")
> .name("Microsoft Azure Service Management Service API")
> - .version("2014-06-01")
> + .version("2014-10-01")
> .identityName("Path to Management Certificate .p12 file, or PEM string")
> .credentialName("Password to Management Certificate")
> .defaultEndpoint("https://management.core.windows.net/${" + SUBSCRIPTION_ID + "}")
Endpoints are meant to be usable *without* user modification. If there is no endpoint that can be hardcoded, just remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24975259
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
@nacx squashed and rebased, let's wait for the builder and I'll merge it, ok?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-76377150
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Bhathiya <no...@github.com>.
> + @Override
> + public boolean apply(RoleInstance input) {
> + return input != null && input.instanceStatus() == READY_ROLE;
> + }
> + });
> + }
> + }, 30 * 60, 1, SECONDS).apply(name)) {
> + logger.warn("Instances %s of %s has not reached the status %s within %sms so it will be destroyed.",
> + Iterables.toString(api.getDeploymentApiForService(name).get(name).roleInstanceList()), name,
> + READY_ROLE, Long.toString(operationTimeout));
> + api.getDeploymentApiForService(group).delete(name);
> + api.getCloudServiceApi().delete(name);
> + throw new IllegalStateException(format("Deployment %s is being destroyed as its instanceStatus didn't reach " +
> + "status %s after %ss. Please, try by increasing `jclouds.azure.operation-timeout` and " +
> + " try again", name, READY_ROLE, 30 * 60));
> + }
@andreaturli
I think this is good place to continue the discussion we had so @nacx and Eduard can share their thoughts
As far as I understand RoleInstance represents a node in azure API. So InstacneStatus would represent the state of VM. Deployment is more a collections of nodes .However RoleInstance to NodeMetadata would be bit problematic as Azure RoleInstance represenation not consist of some important data
My suggestion is to
1 - Introduce VirtualMachine in domain
2 - Map DeploymentToVirtualMachines where single deployment produces list of virtual machines
3 - Map VirtualMachineToNodeMetadata
4 - implement ComputeServiceAdapter<VirtualMachine, RoleSize, OSImage, Location>
Proposed changes are partly implemented here
https://github.com/hsbhathiya/jclouds-labs/blob/MyServiceAdapter/azurecompute/src/main/java/org/jclouds/azurecompute/compute/AzureComputeServiceAdapter2.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r25013853
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * It considers only the custom rules added by the user and ignores the default rules created by Azure
> + */
> +public class AzureComputeSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final AzureComputeApi api;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @Inject
> + public AzureComputeSecurityGroupExtension(final AzureComputeApi api, @Named(OPERATION_TIMEOUT) long operationTimeout) {
> + this.api = checkNotNull(api, "api");
> + this.operationTimeout = checkNotNull(operationTimeout, "operationTimeout");
Remove the null checks once the constructor is package private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977064
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + return transformNetworkSecurityGroupToSecurityGroup(id);
> + }
> +
> + @Override
> + public SecurityGroup createSecurityGroup(final String name, Location location) {
> + checkNotNull(name, "name");
> + checkNotNull(location, "location");
> +
> + final NetworkSecurityGroup networkSecurityGroup = NetworkSecurityGroup.create(name, name, location.getId(), null);
> + String createNSGRequestId =
> + api.getNetworkSecurityGroupApi().create(networkSecurityGroup);
> + operationSucceededPredicate.apply(createNSGRequestId);
What if the predicate returns `false`? Should we throw an exception instead of silently continuing? Apply the same reasoning to the rest of operations in this file.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r25331472
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + private static final String DEFAULT_SUBNET_NAME = "jclouds-1";
> + private static final String DEFAULT_SUBNET_ADDRESS_PREFIX = "10.0.0.0/23";
> + private static final String DEFAULT_STORAGE_SERVICE_TYPE = "Standard_GRS";
> +
> + private static final String DEFAULT_LOGIN_USER = "jclouds";
> + private static final String DEFAULT_LOGIN_PASSWORD = "Azur3Compute!";
> + private static final String DEFAULT_STORAGE_ACCOUNT_PREFIX = "jclouds";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final AzureComputeApi api;
> + private final String subscriptionId;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
Make it final.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977020
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -48,6 +49,7 @@ public AzureComputeProviderMetadata(Builder builder) {
>
> public static Properties defaultProperties() {
> Properties properties = new Properties();
> + properties.setProperty(TEMPLATE, "osFamily=UBUNTU,loginUser=jclouds");
Don't duplicate the property here. It is enough to have it in the ApiMetadata.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24975173
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public boolean apply(RoleInstance input) {
> + return input != null && input.instanceStatus() == READY_ROLE;
> + }
> + });
> + }
> + }, 30 * 60, 1, SECONDS).apply(name)) {
> + logger.warn("Instances %s of %s has not reached the status %s within %sms so it will be destroyed.",
> + Iterables.toString(api.getDeploymentApiForService(name).get(name).roleInstanceList()), name,
> + READY_ROLE, Long.toString(operationTimeout));
> + api.getDeploymentApiForService(group).delete(name);
> + api.getCloudServiceApi().delete(name);
> + throw new IllegalStateException(format("Deployment %s is being destroyed as its instanceStatus didn't reach " +
> + "status %s after %ss. Please, try by increasing `jclouds.azure.operation-timeout` and " +
> + " try again", name, READY_ROLE, 30 * 60));
> + }
The jclouds ComputeService will already perform the active wait until the node is in state RUNNING. Is it really necessary to manually wait for the node being ready here? can't you just return the Deployment without waiting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976668
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * @see <a href="http://msdn.microsoft.com/en-us/library/gg715315.aspx">docs</a>
> + */
> +@Headers(keys = "x-ms-version", values = "{jclouds.api-version}")
> +@Path("/rolesizes")
> +@Consumes(MediaType.APPLICATION_XML)
> +public interface SubscriptionApi {
> +
> + /**
> + * The List Role Sizes request may be specified as follows. Replace <subscription-id> with the subscription ID.
> + */
> + @Named("ListRoleSizes")
> + @GET
> + @XMLResponseParser(ListRoleSizesHandler.class)
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<RoleSize> list(@PathParam("subscriptionId") String subscriptionId);
Change the name to `listRoleSizes`? Otherwise it gives the impression that this method will list subscription objects.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978012
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -88,17 +339,70 @@ public void resumeNode(String id) {
> @Override
> public void suspendNode(String id) {
> // TODO Auto-generated method stub
> -
Is this intentionally unimplemented? Should it be better to throw an exception if this is not supported?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976841
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + builder.status(serverStateToNodeStatus.get(from.status()));
> + }
> + Set<String> publicIpAddresses = Sets.newLinkedHashSet();
> + if (from.virtualIPs() != null) {
> + for (Deployment.VirtualIP virtualIP : from.virtualIPs()) {
> + publicIpAddresses.add(virtualIP.address());
> + }
> + builder.publicAddresses(publicIpAddresses);
> + }
> + Set<String> privateIpAddresses = Sets.newLinkedHashSet();
> + if (from.roleInstanceList() != null) {
> + for (Deployment.RoleInstance roleInstance : from.roleInstanceList()) {
> + privateIpAddresses.add(roleInstance.ipAddress());
> + }
> + builder.privateAddresses(privateIpAddresses);
> + }
Does the API return the credentials? If not, consider setting there from the credential store as a best-effort to get them. Take [DigitalOcean](https://github.com/jclouds/jclouds-labs/blob/master/digitalocean/src/main/java/org/jclouds/digitalocean/compute/functions/DropletToNodeMetadata.java#L115-L120) as an example.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977465
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> @Override
> public Iterable<Deployment> listNodes() {
> - // TODO Auto-generated method stub
> - return null;
> + Set<Deployment> deployments = FluentIterable.from(api.getCloudServiceApi().list())
> + .transform(new Function<CloudService, Deployment>() {
> + @Override
> + public Deployment apply(CloudService cloudService) {
> + return api.getDeploymentApiForService(cloudService.name()).get(cloudService.name());
> + }
> + })
> + .filter(notNull())
> + .toSet();
> + return deployments;
> }
>
> @Override public Iterable<Deployment> listNodesByIds(Iterable<String> ids) {
> // TODO Auto-generated method stub
> return null;
If there is no method in the provider API to get N nodes by their ID, just call the `listNodes()` and filter the results.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976873
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + String removeFromSubnetRequestId = api.getNetworkSecurityGroupApi().removeFromSubnet(virtualNetworkName, subnetName, networkSecurityGroupAppliedToSubnet.name());
> + String operationDescription = format("Remove existing networkSecurityGroup(%s) from subnet(%s)", networkSecurityGroupName, subnetName);
> + waitForOperationCompletion(removeFromSubnetRequestId, operationDescription);
> + }
> + }
> + // add nsg to subnet
> + logger.debug("Adding a networkSecurityGroup %s is already applied to subnet '%s' of virtual network %s ...", networkSecurityGroupName, subnetName, virtualNetworkName);
> + String addToSubnetId = api.getNetworkSecurityGroupApi().addToSubnet(virtualNetworkName, subnetName,
> + NetworkSecurityGroup.create(networkSecurityGroupName, null, null, null));
> + String operationDescription = format("Add networkSecurityGroup(%s) to subnet(%s)", networkSecurityGroupName, subnetName);
> + waitForOperationCompletion(addToSubnetId, operationDescription);
> + }
> +
> + logger.debug("Creating a cloud service with name '%s', label '%s' in location '%s'", name, name, location);
> + String createCloudServiceRequestId = api.getCloudServiceApi().createWithLabelInLocation(name, name, location);
> + waitForOperationCompletion(createCloudServiceRequestId, format("Create cloud service (%s)", name));
Same here. Move all operations global to the nodes to the upper layer. Keep in mind that you might be deploying N nodes and this method will be executed concurrently for all them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976595
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
@nacx thanks also for the summary!
Are we good in merging it now and then address the other issue later on?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-76374779
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -62,8 +62,8 @@ public Image apply(OSImage image) {
> .description(image.description())
> .status(Image.Status.AVAILABLE)
> .uri(image.mediaLink())
> - .providerId(image.name())
> - .location(createLocation(image.location()));
> + .providerId(image.name());
> + //.location(createLocation(image.location()));
Can we get this back?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977516
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + .toSet();
> + // TODO filter deployments
> + for (Deployment deployment : deployments) {
> + Deployment.VirtualIP virtualIP = Iterables.tryFind(deployment.virtualIPs(), Predicates.notNull()).orNull();
> + if (virtualIP == null) throw new IllegalStateException("");
> +
> + for (Role role : deployment.roles()) {
> + for (Role.ConfigurationSet configurationSet : role.configurationSets()) {
> + for (int i = ipPermission.getFromPort(); i <= ipPermission.getToPort(); i++) {
> + String name = String.format(AzureComputeConstants.TCP_FORMAT, i, i);
> + configurationSet.inputEndpoints().remove(createInputEndpoint(name, ipPermission.getIpProtocol()
> + .name().toLowerCase(), virtualIP.address(), i));
> + }
> + String updateRoleRequestId = api.getVirtualMachineApiForDeploymentInService(deployment.name(), deployment.name()).updateRole(role.roleName(), role);
> + if (!operationSucceeded.apply(updateRoleRequestId)) {
> + // TODO
Throw an exception here to fail? Also, implement the other similar TODOs in this class, to have all execution paths covered.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977217
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Splitter;
> +import com.google.common.collect.FluentIterable;
> +import com.google.common.collect.Iterables;
> +import com.google.common.collect.Multimap;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
> + *
> + * It considers only the custom rules added by the user and ignores the default rules created by Azure
> + */
> +public class AzureComputeSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final AzureComputeApi api;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
Make this final too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977011
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + private boolean checkAvailability(String name) {
> + return api.getStorageAccountApi().checkAvailable(subscriptionId, name).result();
> + }
> +
> + @VisibleForTesting
> + public static String generateStorageServiceName(String prefix) {
> + String characters = "abcdefghijklmnopqrstuvwxyz";
> + StringBuilder builder = new StringBuilder();
> + builder.append(prefix);
> + int charactersLength = characters.length();
> + for (int i = 0; i < 10; i++) {
> + double index = Math.random() * charactersLength;
> + builder.append(characters.charAt((int) index));
> + }
> + return builder.toString();
> + }
Move this to a Supplier that can be injected wherever it is needed. This way you have a clean way of generating storage service names. Consider using the `GroupNamingConvention` to generate it properly prefixed, if needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976948
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + final String subnetAddressPrefix = templateOptions.getSubnetAddressPrefix().or(DEFAULT_SUBNET_ADDRESS_PREFIX);
> + final String storageAccountType = templateOptions.getStorageAccountType().or(DEFAULT_STORAGE_SERVICE_TYPE);
> +
> + final String loginUser = templateOptions.getLoginUser() != null ? templateOptions.getLoginUser() : DEFAULT_LOGIN_USER;
> + final String loginPassword = templateOptions.getLoginPassword() != null ? templateOptions.getLoginPassword() : DEFAULT_LOGIN_PASSWORD;
> + final String location = template.getLocation().getId();
> + final int[] inboundPorts = template.getOptions().getInboundPorts();
> +
> + String storageAccountName = templateOptions.getStorageAccountName().or(generateStorageServiceName(DEFAULT_STORAGE_ACCOUNT_PREFIX));
> +
> + // get or create storage service
> + StorageService storageService = tryFindExistingStorageServiceAccountOrCreate(location, storageAccountName, storageAccountType);
> + String storageServiceAccountName = storageService.serviceName();
> +
> + // check existence or create virtual network
> + checkExistingVirtualNetworkNamedOrCreate(virtualNetworkName, location, subnetName, addressSpaceAddressPrefix, subnetAddressPrefix);
Also, there is the `tempalteOptions.getnetworks()` method that allows users to specify a set of network IDs where the node should be attached. Is this something that can be done in Azure? No need to address this in this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976547
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * @param virtualNetworkName
> + * @return
> + */
> + @Named("AddNetworkSecurityGroupToSubnet.")
> + @Path("/virtualnetwork/{virtualNetworkName}/subnets/{subnetName}/networksecuritygroups")
> + @POST
> + @Produces(MediaType.APPLICATION_XML)
> + @ResponseParser(ParseRequestIdHeader.class)
> + String addToSubnet(@PathParam("virtualNetworkName") String virtualNetworkName,
> + @PathParam("subnetName") String subnetName,
> + @BinderParam(NetworkSecurityGroupToXML.class) NetworkSecurityGroup networkSecurityGroup);
> +
> + /**
> + * Removes a Network Security Group from a subnet
> + */
> + @Named("RemoveNetworkSecurityGroupToSubnet.")
Remove the trailing `.`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977715
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + assertTrue(roleInstanceReady.apply(DEPLOYMENT), roleInstance.toString());
> + Logger.getAnonymousLogger().info("roleInstance started: " + roleInstance);
> + }
> +
> + @Test(dependsOnMethods = "testStart")
> + public void testRestart() {
> + String requestId = api().restart(roleName);
> + assertTrue(operationSucceeded.apply(requestId), requestId);
> + Logger.getAnonymousLogger().info("operation succeeded: " + requestId);
> +
> + RoleInstance roleInstance = getFirstRoleInstanceInDeployment(DEPLOYMENT);
> + assertTrue(roleInstanceReady.apply(DEPLOYMENT), roleInstance.toString());
> + Logger.getAnonymousLogger().info("roleInstance restarted: " + roleInstance);
> + }
> +
> + @AfterClass
Same comment about always running the tear down.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978518
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
> + *
> + * It considers only the custom rules added by the user and ignores the default rules created by Azure
> + */
> +public class AzureComputeSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final AzureComputeApi api;
> + private Predicate<String> operationSucceededPredicate;
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @Inject
> + public AzureComputeSecurityGroupExtension(final AzureComputeApi api, Predicate<String> operationSucceededPredicate) {
Make the injection constructor package private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r25331284
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + // remove existing nsg from subnet
> + String removeFromSubnetRequestId = api.getNetworkSecurityGroupApi().removeFromSubnet
> + (virtualNetworkName, subnetName, networkSecurityGroupAppliedToSubnet.name());
> + String operationDescription = format("Remove existing networkSecurityGroup(%s) from subnet(%s)", id, subnetName);
> + waitForOperationCompletion(removeFromSubnetRequestId, operationDescription);
> + }
> + }
> + }
> + }
> + }
> + }
> + String deleteRequestId = api.getNetworkSecurityGroupApi().delete(id);
> + if (!operationSucceeded.apply(deleteRequestId)) {
> + return false;
> + }
> + return true;
Just `return operationSucceeded.apply(deleteRequestId);`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977153
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + super(credentialsFromImageOrTemplateOptions);
> + }
> +
> + public LoginCredentials apply(Template template, LoginCredentials fromNode) {
> + RunScriptOptions options = checkNotNull(template.getOptions(), "template options are required");
> + LoginCredentials.Builder builder = LoginCredentials.builder(fromNode);
> + if (options.getLoginUser() != null)
> + builder.user(template.getOptions().getLoginUser());
> + if (options.getLoginPassword() != null)
> + builder.password(options.getLoginPassword());
> + if (options.getLoginPrivateKey() != null)
> + builder.privateKey(options.getLoginPrivateKey());
> + if (options.shouldAuthenticateSudo() != null && options.shouldAuthenticateSudo())
> + builder.authenticateSudo(true);
> + return builder.build();
> + }
This seems to be something that other providers can benefit from. Consider moving this class to the jclouds-compute project.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977590
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * 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.jclouds.azurecompute.features;
> +
> +import static org.testng.Assert.assertNotNull;
> +import org.jclouds.azurecompute.domain.RoleSize;
> +import org.jclouds.azurecompute.internal.BaseAzureComputeApiLiveTest;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "LocationApiLiveTest")
Test name should be "SubscriptionApiLiveTest".
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978495
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testGet() {
> + Deployment foundDeployment = api().get(deployment.name());
> + assertThat(foundDeployment).isEqualToComparingFieldByField(deployment);
> + }
> +
> + @Test(dependsOnMethods = "testGet")
> + public void testDelete() {
> + String requestId = api().delete(deployment.name());
> + assertTrue(operationSucceeded.apply(requestId), requestId);
> + Logger.getAnonymousLogger().info("operation succeeded: " + requestId);
> +
> + assertTrue(deploymentGone.apply(deployment), deployment.toString());
> + Logger.getAnonymousLogger().info("deployment deleted: " + deployment);
> + }
> +
> + @Override @AfterClass(groups = "live")
Same comment. Configure it with an `alwaysRun = true`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978432
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * is optional by providers.
> + *
> + * It considers only the custom rules added by the user and ignores the default rules created by Azure
> + */
> +public class AzureComputeSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final AzureComputeApi api;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @Inject
> + public AzureComputeSecurityGroupExtension(final AzureComputeApi api, @Named(OPERATION_TIMEOUT) long operationTimeout) {
Change the constructor to be package private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977041
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + */
> + @Named("CreateStorageAccount")
> + @POST
> + @Produces(APPLICATION_XML)
> + @ResponseParser(ParseRequestIdHeader.class)
> + String create(@BinderParam(StorageServiceParamsToXML.class) StorageServiceParams storageServiceParams);
> +
> + /**
> + * https://management.core.windows.net/<subscription-id>/services/storageservices
> + */
> + @Named("GetStorageAccountDetails")
> + @GET
> + @Path("/{storageAccountName}")
> + @XMLResponseParser(StorageServiceHandler.class)
> + @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
Change to `@Fallback(Fallbacks.NullOnNotFoundOr404.class)`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977865
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
I've created sub-tasks on jira [JCLOUDS-664](https://issues.apache.org/jira/browse/JCLOUDS-664) so we don't forget your valuable commets not yet addressed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-76375264
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public boolean apply(RoleInstance input) {
> + return input != null && input.instanceStatus() == READY_ROLE;
> + }
> + });
> + }
> + }, 30 * 60, 1, SECONDS).apply(name)) {
> + logger.warn("Instances %s of %s has not reached the status %s within %sms so it will be destroyed.",
> + Iterables.toString(api.getDeploymentApiForService(name).get(name).roleInstanceList()), name,
> + READY_ROLE, Long.toString(operationTimeout));
> + api.getDeploymentApiForService(group).delete(name);
> + api.getCloudServiceApi().delete(name);
> + throw new IllegalStateException(format("Deployment %s is being destroyed as its instanceStatus didn't reach " +
> + "status %s after %ss. Please, try by increasing `jclouds.azure.operation-timeout` and " +
> + " try again", name, READY_ROLE, 30 * 60));
> + }
@eduardkoller could you give us a bit of insight? There are a couple conceptual things I don't have clear:
* The current impl uses a Deployment as a Node. Would it be more correct to use a Role? (We can address this in an upcoming PR once this starting one is merged).
Taking into account the current implementation:
* How do we determine the state of a VM? There is the state of the RoleInstance, but there is also the state of the Deployment. Which one should we take into account?
* Finally, jclouds already polls for the status of the node, so there is no need for this class to wait for that. Can we remove this active wait from here and just return the Deployment, regardless of its RoleInstance status?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24990585
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
> + @Override
> + public boolean apply(RoleInstance input) {
> + return input != null && input.instanceStatus() == READY_ROLE;
> + }
> + });
> + }
> + }, 30 * 60, 1, SECONDS).apply(name)) {
> + logger.warn("Instances %s of %s has not reached the status %s within %sms so it will be destroyed.",
> + Iterables.toString(api.getDeploymentApiForService(name).get(name).roleInstanceList()), name,
> + READY_ROLE, Long.toString(operationTimeout));
> + api.getDeploymentApiForService(group).delete(name);
> + api.getCloudServiceApi().delete(name);
> + throw new IllegalStateException(format("Deployment %s is being destroyed as its instanceStatus didn't reach " +
> + "status %s after %ss. Please, try by increasing `jclouds.azure.operation-timeout` and " +
> + " try again", name, READY_ROLE, 30 * 60));
> + }
I need to be sure that the Role inside the Deployment is in READY_ROLE state, so I think it is needed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24986719
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> + @Named("GetDetailsNetworkSecurityGroup")
> + @Path("/networksecuritygroups/{networkSecurityGroupName}")
> + @GET
> + @QueryParams(keys = "detaillevel", values = "Full")
> + @XMLResponseParser(NetworkSecurityGroupHandler.class)
> + @Fallback(NullOnNotFoundOr404.class)
> + NetworkSecurityGroup getFullDetails(@PathParam("networkSecurityGroupName") String networkSecurityGroupName);
> +
> + /**
> + * Adds a Network Security Group to a subnet.
> + *
> + * @param virtualNetworkName
> + * @return
> + */
> + @Named("AddNetworkSecurityGroupToSubnet.")
Remove the trailing `.`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977713
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + null, // loadBalancerName
> + null, // loadBalancerProbe
> + null //idleTimeoutInMinutes
> + );
> + }
> +
> + public void waitForOperationCompletion(String operationId, String operationDescription) {
> + if (!operationSucceeded.apply(operationId)) {
> + final String warnMessage = format("%s) has not been completed within %sms.", operationDescription, Long.toString(operationTimeout));
> + logger.warn(warnMessage);
> + String illegalStateExceptionMessage = format("%s. Please, try by increasing `jclouds.azure.operation-timeout` and try again", warnMessage);
> + throw new IllegalStateException(illegalStateExceptionMessage);
> + } else {
> + logger.info("%s (id: %s) terminated successfully", operationDescription, operationId);
> + }
> + }
This method is already present in the compute service adapter. Extract it somewhere so it can be reused.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977279
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + final String subnetAddressPrefix = templateOptions.getSubnetAddressPrefix().or(DEFAULT_SUBNET_ADDRESS_PREFIX);
> + final String storageAccountType = templateOptions.getStorageAccountType().or(DEFAULT_STORAGE_SERVICE_TYPE);
> +
> + final String loginUser = templateOptions.getLoginUser() != null ? templateOptions.getLoginUser() : DEFAULT_LOGIN_USER;
> + final String loginPassword = templateOptions.getLoginPassword() != null ? templateOptions.getLoginPassword() : DEFAULT_LOGIN_PASSWORD;
> + final String location = template.getLocation().getId();
> + final int[] inboundPorts = template.getOptions().getInboundPorts();
> +
> + String storageAccountName = templateOptions.getStorageAccountName().or(generateStorageServiceName(DEFAULT_STORAGE_ACCOUNT_PREFIX));
> +
> + // get or create storage service
> + StorageService storageService = tryFindExistingStorageServiceAccountOrCreate(location, storageAccountName, storageAccountType);
> + String storageServiceAccountName = storageService.serviceName();
> +
> + // check existence or create virtual network
> + checkExistingVirtualNetworkNamedOrCreate(virtualNetworkName, location, subnetName, addressSpaceAddressPrefix, subnetAddressPrefix);
Same comment here. as the templateOptions are shared with all nodes, all "get or create" operations should be done *before* this method is run to avoid race conditions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976462
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
> + addRuleToNetworkSecurityGroup(id, ruleName, priority, ipPermission);
> +
> + // add endpoint to VM
> + Set<Deployment> deployments = FluentIterable.from(api.getCloudServiceApi().list())
> + .transform(new Function<CloudService, Deployment>() {
> + @Override
> + public Deployment apply(CloudService cloudService) {
> + return api.getDeploymentApiForService(cloudService.name()).get(cloudService.name());
> + }
> + })
> + .filter(Predicates.notNull())
> + .toSet();
> + // TODO filter deployments
> + for (Deployment deployment : deployments) {
> + Deployment.VirtualIP virtualIP = Iterables.tryFind(deployment.virtualIPs(), Predicates.notNull()).orNull();
> + if (virtualIP == null) throw new IllegalStateException("");
Added that but for some reason github is still showing that
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r25087787
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @Produces(MediaType.APPLICATION_XML)
> @ResponseParser(ParseRequestIdHeader.class)
> @Payload(value = "<StartRoleOperation xmlns=\"http://schemas.microsoft.com/windowsazure\"><OperationType>StartRoleOperation</OperationType></StartRoleOperation>")
> String start(@PathParam("name") String name);
> +
> + /**
> + * https://msdn.microsoft.com/en-us/library/azure/jj157193.aspx
> + */
> + @Named("GetRole")
> + @GET
> + @Path("/roles/{roleName}")
> + @Produces(MediaType.APPLICATION_XML)
> + @ResponseParser(ParseRequestIdHeader.class)
> + Role getRole(@PathParam("roleName") String roleName);
Add a fallback to return `null` if the role is not found.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978041
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
Merged at [e97ddaeeb59acc516e3d4d8398cf2da5e5492487](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=e97ddaeeb59acc516e3d4d8398cf2da5e5492487)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-76381931
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + private Predicate<String> operationSucceeded;
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + @Inject
> + public AzureComputeSecurityGroupExtension(final AzureComputeApi api, @Named(OPERATION_TIMEOUT) long operationTimeout) {
> + this.api = checkNotNull(api, "api");
> + this.operationTimeout = checkNotNull(operationTimeout, "operationTimeout");
> + this.operationSucceeded = retry(new Predicate<String>() {
> + public boolean apply(String input) {
> + final Operation operation = api.getOperationApi().get(input);
> + return operation.status() == Operation.Status.SUCCEEDED;
> + }
> + }, operationTimeout, 5, 5, SECONDS);
Same comment than above. Extract the predicate to a provider and get it injected here directly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977053
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * @return The response body is a netcfg.cfg file.
> +
> + */
> + @Named("GetVirtualNetworkConfiguration")
> + @Path("/media")
> + @GET
> + @XMLResponseParser(NetworkConfigurationHandler.class)
> + @Fallback(NullOnNotFoundOr404.class)
> + NetworkConfiguration get();
> +
> + @Named("ListVirtualNetworkSites")
> + @Path("/virtualnetwork")
> + @GET
> + @XMLResponseParser(ListVirtualNetworkSitesHandler.class)
> + @Fallback(NullOnNotFoundOr404.class)
Don't return null. Return an empty list here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978115
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private static final String DEFAULT_LOGIN_USER = "jclouds";
> + private static final String DEFAULT_LOGIN_PASSWORD = "Azur3Compute!";
> + private static final String DEFAULT_STORAGE_ACCOUNT_PREFIX = "jclouds";
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + private final AzureComputeApi api;
> + private final String subscriptionId;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
> +
> + @Inject
> + public AzureComputeServiceAdapter(final AzureComputeApi api, @Named(SUBSCRIPTION_ID) String subscriptionId, @Named(OPERATION_TIMEOUT) long operationTimeout) {
Make the constructor package private and remove the null checks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24975528
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + final String virtualNetworkName = templateOptions.getVirtualNetworkName().or(DEFAULT_VIRTUAL_NETWORK_NAME);
> + final String addressSpaceAddressPrefix = templateOptions.getAddressSpaceAddressPrefix().or(DEFAULT_ADDRESS_SPACE_ADDRESS_PREFIX);
> + final String subnetName = templateOptions.getSubnetName().or(DEFAULT_SUBNET_NAME);
> + final Set<String> networkSecurityGroupNames = templateOptions.getGroups().isEmpty() ? Sets.<String>newHashSet() : templateOptions.getGroups();
> + final String subnetAddressPrefix = templateOptions.getSubnetAddressPrefix().or(DEFAULT_SUBNET_ADDRESS_PREFIX);
> + final String storageAccountType = templateOptions.getStorageAccountType().or(DEFAULT_STORAGE_SERVICE_TYPE);
> +
> + final String loginUser = templateOptions.getLoginUser() != null ? templateOptions.getLoginUser() : DEFAULT_LOGIN_USER;
> + final String loginPassword = templateOptions.getLoginPassword() != null ? templateOptions.getLoginPassword() : DEFAULT_LOGIN_PASSWORD;
> + final String location = template.getLocation().getId();
> + final int[] inboundPorts = template.getOptions().getInboundPorts();
> +
> + String storageAccountName = templateOptions.getStorageAccountName().or(generateStorageServiceName(DEFAULT_STORAGE_ACCOUNT_PREFIX));
> +
> + // get or create storage service
> + StorageService storageService = tryFindExistingStorageServiceAccountOrCreate(location, storageAccountName, storageAccountType);
Take care with `get or create` operations here, as they will introduce race conditions. This method will be executed concurrently when creating more than one node.
This should be done in the upper layer, in a custom implementation of the `CreateNodesWithGroupEncodedIntoNameThenAddToSet` class. Take a look at how [digitalocean creates the key pairs globally](https://github.com/jclouds/jclouds-labs/blob/master/digitalocean/src/main/java/org/jclouds/digitalocean/compute/strategy/CreateKeyPairsThenCreateNodes.java) before creating the nodes to avoid this kind of race conditions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976158
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + deployment = adapter.createNodeWithGroupEncodedIntoName(group, name, template);
> + assertEquals(deployment.getNode().name(), name);
> + assertEquals(deployment.getNodeId(), deployment.getNode().name());
> + assert InetAddresses.isInetAddress(deployment.getNode().virtualIPs().get(0).address()) : deployment;
> + doConnectViaSsh(deployment.getNode(), prioritizeCredentialsFromTemplate.apply(template, deployment.getCredentials()));
> + }
> +
> + protected void doConnectViaSsh(Deployment deployment, LoginCredentials creds) {
> + SshClient ssh = sshFactory.create(HostAndPort.fromParts(deployment.virtualIPs().get(0).address(), 22), creds);
> + try {
> + ssh.connect();
> + ExecResponse hello = ssh.exec("echo hello");
> + assertEquals(hello.getOutput().trim(), "hello");
> + System.err.println(ssh.exec("df -k").getOutput());
> + System.err.println(ssh.exec("mount").getOutput());
> + System.err.println(ssh.exec("uname -a").getOutput());
Consider removing the console output.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978253
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + if (ssh != null)
> + ssh.disconnect();
> + }
> + }
> +
> + @Test
> + public void testListHardwareProfiles() {
> + Iterable<RoleSize> roleSizes = adapter.listHardwareProfiles();
> + assertFalse(Iterables.isEmpty(roleSizes));
> +
> + for (RoleSize roleSize : roleSizes) {
> + assertNotNull(roleSize);
> + }
> + }
> +
> + @AfterGroups(groups = "live")
Also configure it with an `alwayrRun = true` to make sure we don't leak resources when the tests fail.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978276
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + protected void tearDown() {
> + if (deployment != null) {
> + adapter.destroyNode(deployment.getNodeId());
> + }
> + super.tearDown();
> + }
> +
> + @Override
> + protected Iterable<Module> setupModules() {
> + return ImmutableSet.<Module> of(getLoggingModule(), new SshjSshClientModule());
> + }
> +
> + @Override
> + protected Properties setupProperties() {
> + Properties properties = super.setupProperties();
> + properties.setProperty("jclouds.ssh.max-retries", "10");
Users should be able to override this by setting the value from the command line. Set it only if not present.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978334
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +@Headers(keys = "x-ms-version", values = "{jclouds.api-version}")
> +@Consumes(MediaType.APPLICATION_XML)
> +public interface VirtualNetworkApi {
> +
> + /**
> + * The Get Network Configuration operation retrieves the network configuration file.
> + *
> + * @return The response body is a netcfg.cfg file.
> +
> + */
> + @Named("GetVirtualNetworkConfiguration")
> + @Path("/media")
> + @GET
> + @XMLResponseParser(NetworkConfigurationHandler.class)
> + @Fallback(NullOnNotFoundOr404.class)
> + NetworkConfiguration get();
Change to `getNetworkConfiguration`. This returns a `NetworkConfiguration` but the `list` method returns `VirtualNetworkSite`. The naming can be confusing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978102
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + addRuleToNetworkSecurityGroup(id, ruleName, priority, ipPermission);
> +
> + // add endpoint to VM
> + Set<Deployment> deployments = FluentIterable.from(api.getCloudServiceApi().list())
> + .transform(new Function<CloudService, Deployment>() {
> + @Override
> + public Deployment apply(CloudService cloudService) {
> + return api.getDeploymentApiForService(cloudService.name()).get(cloudService.name());
> + }
> + })
> + .filter(Predicates.notNull())
> + .toSet();
> + // TODO filter deployments
> + for (Deployment deployment : deployments) {
> + Deployment.VirtualIP virtualIP = Iterables.tryFind(deployment.virtualIPs(), Predicates.notNull()).orNull();
> + if (virtualIP == null) throw new IllegalStateException("");
Add some explanatory message to the exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977174
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + StringBuilder builder = new StringBuilder();
> + builder.append(prefix);
> + int charactersLength = characters.length();
> + for (int i = 0; i < 10; i++) {
> + double index = Math.random() * charactersLength;
> + builder.append(characters.charAt((int) index));
> + }
> + return builder.toString();
> + }
> +
> + @VisibleForTesting
> + public static URI createMediaLink(String storageServiceName, String diskName) {
> + return URI.create(String.format("https://%s.blob.core.windows.net/vhds/disk-%s.vhd", storageServiceName, diskName));
> + }
> +
> + public void waitForOperationCompletion(String operationId, String operationDescription) {
Does this need to be public?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976975
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Andrea Turli <no...@github.com>.
Closed #135.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#event-243211094
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + .put(Deployment.Status.RUNNING_TRANSITIONING, NodeMetadata.Status.PENDING)
> + .put(Deployment.Status.DEPLOYING, NodeMetadata.Status.PENDING)
> + .put(Deployment.Status.STARTING, NodeMetadata.Status.PENDING)
> + .put(Deployment.Status.SUSPENDED, NodeMetadata.Status.SUSPENDED)
> + .put(Deployment.Status.RUNNING, NodeMetadata.Status.RUNNING)
> + .put(Deployment.Status.UNRECOGNIZED, NodeMetadata.Status.UNRECOGNIZED).build();
> +
> + @Inject
> + DeploymentToNodeMetadata(@Memoized Supplier<Set<? extends Location>> locations,
> + GroupNamingConvention.Factory namingConvention, OSImageToImage osImageToImage,
> + RoleSizeToHardware roleSizeToHardware, Map<String, Credentials> credentialStore) {
> + this.nodeNamingConvention = checkNotNull(namingConvention, "namingConvention").createWithoutPrefix();
> + this.locations = checkNotNull(locations, "locations");
> + this.osImageToImage = checkNotNull(osImageToImage, "osImageToImage");
> + this.roleSizeToHardware = checkNotNull(roleSizeToHardware, "roleSizeToHardware");
> + this.credentialStore = checkNotNull(credentialStore, "credentialStore cannot be null");
Since the constructor is package private and only the Guice injector will be able to call it, you can remove all the null checks. The injector will already fail if any parameter is missing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977325
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
>
> + String deleteDeploymentRequestId = api.getDeploymentApiForService(cloudService.name()).delete(id);
> + String operationDescription = format("Delete deployment(%s) of cloudService(%s)", id, cloudService.name());
> + waitForOperationCompletion(deleteDeploymentRequestId, operationDescription);
> +
> + String cloudServiceDeleteRequestId = api.getCloudServiceApi().delete(cloudService.name());
> + operationDescription = format("Delete cloudService(%s)", cloudService.name(), Long.toString(operationTimeout));
> + waitForOperationCompletion(cloudServiceDeleteRequestId, operationDescription);
When destroying nodes, should we consider removing the storage account at some point?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24976789
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> @Override
> - public NodeMetadata apply(Deployment input) {
> - return null;
> + public NodeMetadata apply(Deployment from) {
> + NodeMetadataBuilder builder = new NodeMetadataBuilder();
> + builder.ids(from.name());
Is the deployment name unique? Is it the *real* id of the node int he provider? Otherwise call separately the `id()` and the `providerId()` methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977372
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @andreaturli! This is quite good. Apart from the inline comments here are some general considerations:
* Move all global operations in the ComputeService adapter to the upper layer to void race conditions.
* In all live tests, add the `alwaysRun = true` to the clenaup method so it is executed even if tests fail. Let's try to not leak resources.
* There are missing mock tests, but those can be added in upcoming PRs.
* There should also be unit tests for all transformation functions.
Good job!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-75031066
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
The ComputeService implementation looks much cleaner now. Goog job! There are just a few minor comments to be addressed.
There are also several comments that have not been considered or been answered that can be addressed in subsequenc PRs (but let's discuss them so we don't forget):
* As said [here](https://github.com/jclouds/jclouds-labs/pull/135#discussion-diff-24976948), should we use the `GroupNamingConvention` to generate the names?
* These comments regarding the `DeploymentToNodeMetadata`: [ids](https://github.com/jclouds/jclouds-labs/pull/135#discussion-diff-24977372), [commented code](https://github.com/jclouds/jclouds-labs/pull/135#discussion-diff-24977411) and [credentials](https://github.com/jclouds/jclouds-labs/pull/135#discussion-diff-24977465).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135#issuecomment-75942461
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private final AzureComputeApi api;
> + private final String subscriptionId;
> + private final long operationTimeout;
> + private Predicate<String> operationSucceeded;
> +
> + @Inject
> + public AzureComputeServiceAdapter(final AzureComputeApi api, @Named(SUBSCRIPTION_ID) String subscriptionId, @Named(OPERATION_TIMEOUT) long operationTimeout) {
> + this.api = checkNotNull(api, "api");
> + this.subscriptionId = checkNotNull(subscriptionId, "subscriptionId");
> + this.operationTimeout = checkNotNull(operationTimeout, "operationTimeout");
> + this.operationSucceeded = retry(new Predicate<String>() {
> + public boolean apply(String input) {
> + return api.getOperationApi().get(input).status() == Operation.Status.SUCCEEDED;
> + }
> + }, operationTimeout, 5, 5, SECONDS);
Consider moving this predicate to a provider method in the Guice configuration module so it can be injected and reused (reused in live tests too?).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24975581
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> + * The List Storage Accounts operation lists the storage accounts that are available in the specified subscription.
> + */
> + @Named("ListStorageAccounts")
> + @GET
> + @XMLResponseParser(ListStorageServicesHandler.class)
> + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
> + List<StorageService> list(@PathParam("subscriptionId") String subscriptionId);
> +
> + /**
> + * The Check Storage Account Name Availability operation checks to see if the specified storage account name is available, or if it has already been taken.
> + */
> + @Named("CheckStorageAccountNameAvailability")
> + @GET
> + @Path("/operations/isavailable/{storageAccountName}")
> + @XMLResponseParser(AvailabilityHandler.class)
> + @Fallback(NullOnNotFoundOr404.class)
What does it mean that a check returns `null`? Shouldn't it return just a boolean? if you want to return an object, shouldn't it be better to throw an exception if the check can not be performed? (I think it is not clear what a null means in this case: not available/the check operation failed).
Consider removing the fallback to propagate an exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977959
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Predicates;
> +import com.google.common.base.Splitter;
> +import com.google.common.collect.FluentIterable;
> +import com.google.common.collect.Iterables;
> +import com.google.common.collect.Multimap;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
> + *
> + * It considers only the custom rules added by the user and ignores the default rules created by Azure
> + */
> +public class AzureComputeSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final AzureComputeApi api;
> + private Predicate<String> operationSucceededPredicate;
Make this final
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r25331263
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Override
> + protected Properties setupProperties() {
> + Properties props = super.setupProperties();
> + setIfTestSystemPropertyPresent(props, SUBSCRIPTION_ID);
> + return props;
> + }
> +
> + @Override
> + protected Module getSshModule() {
> + return new SshjSshClientModule();
> + }
> +
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
This class is not instantiated by Guice, so the logger won't get injected.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24978356
Re: [jclouds-labs] AzureCompute: initial work to support
ComputeServiceAdapter (#135)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.location.suppliers.all.JustProvider;
> +
> +import com.google.common.base.Function;
> +import com.google.common.collect.ImmutableMap;
> +import com.google.common.collect.Iterables;
> +
> +/**
> + * Converts an Datacenter into a Location.
> + */
> +@Singleton
> +public class LocationToLocation implements Function<Location, org.jclouds.domain.Location> {
> + private final JustProvider provider;
> +
> + // allow us to lazy discover the provider of a resource
> + @Inject
> + public LocationToLocation(JustProvider provider) {
Same here. Make the constructor package private and remove the null checks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/135/files#r24977488