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 2018/08/01 14:53:50 UTC
[jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
- add instance API
- add compute abstraction
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/443
-- Commit Summary --
* [JCLOUDS-1430] Aliyun ECS
-- File Changes --
A aliyun-ecs/README.md (47)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/ECSComputeServiceApi.java (4)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/ECSServiceApiMetadata.java (2)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/ECSComputeService.java (105)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/ECSComputeServiceAdapter.java (279)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/config/ECSServiceContextModule.java (156)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/ImageToImage.java (84)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/InstanceStatusToStatus.java (43)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/InstanceToNodeMetadata.java (136)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/InstanceTypeToHardware.java (46)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/RegionToLocation.java (54)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/functions/internal/OperatingSystems.java (51)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/options/ECSServiceTemplateOptions.java (156)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/strategy/CleanupResources.java (129)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/compute/strategy/CreateResourcesThenCreateNodes.java (277)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/AllocatePublicIpAddressRequest.java (59)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/AvailableResource.java (43)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/AvailableZone.java (47)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/DedicatedHostAttribute.java (37)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/EipAddress.java (39)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/Instance.java (178)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/InstanceRequest.java (59)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/InstanceStatus.java (55)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/InstanceType.java (39)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/NetworkInterface.java (41)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/ResourceType.java (40)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/SupportedResource.java (36)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/Tag.java (10)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/VpcAttributes.java (48)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/options/CreateInstanceOptions.java (161)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/options/ListInstancesOptions.java (258)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/options/TagOptions.java (12)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/domain/regionscoped/RegionAndId.java (58)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/features/InstanceApi.java (209)
M aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/features/TagApi.java (7)
A aliyun-ecs/src/main/java/org/jclouds/aliyun/ecs/predicates/InstanceStatusPredicate.java (33)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/ECSComputeServiceLiveTest.java (113)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/ECSTemplateBuilderLiveTest.java (53)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/InstanceApiLiveTest.java (124)
A aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/InstanceApiMockTest.java (69)
M aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/SecurityGroupApiLiveTest.java (2)
M aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/SshKeyPairApiLiveTest.java (4)
M aliyun-ecs/src/test/java/org/jclouds/aliyun/ecs/compute/features/TagApiLiveTest.java (9)
A aliyun-ecs/src/test/resources/instanceTypes.json (4281)
A aliyun-ecs/src/test/resources/instances-first.json (960)
A aliyun-ecs/src/test/resources/instances-last.json (960)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/443.patch
https://github.com/jclouds/jclouds-labs/pull/443.diff
--
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/443
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate<InstanceType>() {
+ @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List<String> getInstanceTypeId(String regionId) {
ok
--
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/443#discussion_r207133364
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + }
+ }
+ }
+ return instanceTypeIds;
+ }
+
+ @Override
+ public Iterable<Image> listImages() {
+ final ImmutableList.Builder<Image> images = ImmutableList.builder();
+ final List<String> availableLocationNames = newArrayList(
+ transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
awesome
--
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/443#discussion_r207126456
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ return properties;
+ }
+
+ @Override
+ protected TemplateBuilder templateBuilder() {
+ return super.templateBuilder()
+ .options(vpcId(vpcId)
+ .vSwitchId(vSwitchId));
+ }
+
+ @Override
+ protected Template addRunScriptToTemplate(Template template) {
+ template.getOptions().runScript(Statements.newStatementList(
+ new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK() }));
+ return template;
this was an ugly test, we need to fix `InstallJDK` as it is broken for yum-based OS, IMHO
--
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/443#discussion_r207259289
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
Thanks, @andreaturli! I think we should merge this now and keep iterating on it. Thanks for all the effort!
--
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/443#issuecomment-419863524
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String group, String name, Template template) {
+ String instanceType = template.getHardware().getId();
+ String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
I think I've added the necessary code to support this design: vpcApi vSwitchApi and I've modified the `CreateResourcesThenCreateNodes` accordingly
--
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/443#discussion_r211981725
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
I'd say we want to delete the VPCs *we created* if they are no longer in use. When the last node is removed, then delete the VPC and vSwitch, if we created them. To do that, though, we need to be able to identify them, and the description and name fields seem flaky.
Some questions:
* Are users charged by VPCs, vSwitches?
* Is there a limit on the number of VPCs?
* Other concerns that may have a direct impact on users?
--
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/443#discussion_r208852605
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String group, String name, Template template) {
+ String instanceType = template.getHardware().getId();
+ String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
@nacx that's a fair point, but I think the best we can do is to `checkNotNull` vSwitchId.
In fact, the automatic creation of a vSwitch requires the creation of a VPC as well, which could be handled as VPC api and vSwitch api are quite straightforward *but* which require a number of details: ZoneId, CidrBlock, VpcId and RegionId of course.
Those details are *not* related to the `Template` IMHO but are related to the environment where you want to deploy the VM, so passing the vswitch details in the templateOptions doesn't seem right to me.
Conversely, making a lot of assumptions on behalf of the user to create a reasonably sensible `vSwitch` sounds unreliable and potentially not satisfying for the end-user anyway.
Notice also that a fresh aliyun account comes with no VPC or vSwitch at all so there's no default value to be used here, unfortunately.
wdyt?
--
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/443#discussion_r207111616
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
sorry again as this PR is becoming huge. @nacx I think I've addressed all of your comments now and I've also simplified the RetryHandler for 4xx errors
I'm running the live tests to see the result, some of them are expected to fail though - particularly from `ECSComputeServiceLiveTest`. I'll keep you posted
Thanks again for your help
--
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/443#issuecomment-415059572
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
b2ce3b5 address some comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/b79a887eb460662edb19201977ae961f208dff88..b2ce3b5056c0330d747f04e85dfa815685422e4d
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + checkTagsInNodeEquals(node, tags);
+
+ getAnonymousLogger().info(
+ format("<< available node(%s) os(%s) in %ss", node.getId(), node.getOperatingSystem(), createSeconds));
+
+ watch.reset().start();
+
+ client.runScriptOnNode(nodeId, AdminAccess.builder().adminUsername("web").build(), nameTask("admin-web"));
+
+ long configureSeconds = watch.elapsed(TimeUnit.SECONDS);
+
+ getAnonymousLogger().info(
+ format(
+ "<< configured node(%s) in %ss",
+ nodeId,
+ configureSeconds));
Ill remove the override as I think we can accept to have this test broken until we fix the BaseComputeServiceLiveTest
--
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/443#discussion_r209217688
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> @@ -54,6 +46,7 @@ protected Properties setupProperties() {
Properties properties = super.setupProperties();
vpcId = setIfTestSystemPropertyPresent(properties, provider + ".vpcId");
vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ group = "jclouds"; // otherwise jclouds will use provider name as group but `aliyun` is a forbidden prefix for tags
good point, Ill look into it
--
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/443#discussion_r207828745
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
+
+ for (String regionId : availableLocationNames) {
+ instances.addAll(api.instanceApi().list(regionId).concat());
+ }
+ return instances.build();
+ }
+
+ @Override
+ public Iterable<Instance> listNodesByIds(final Iterable<String> ids) {
+ return filter(listNodes(), new Predicate<Instance>() {
done
--
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/443#discussion_r207133421
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ return properties;
+ }
+
+ @Override
+ protected TemplateBuilder templateBuilder() {
+ return super.templateBuilder()
+ .options(vpcId(vpcId)
+ .vSwitchId(vSwitchId));
+ }
+
+ @Override
+ protected Template addRunScriptToTemplate(Template template) {
+ template.getOptions().runScript(Statements.newStatementList(
+ new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK() }));
+ return template;
Oh, really? I have to give it a try then...
--
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/443#discussion_r207274950
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ public void testList() {
+ final AtomicInteger found = new AtomicInteger(0);
+ assertTrue(Iterables.all(api().list(Regions.EU_CENTRAL_1.getName()).concat(), new Predicate<VPC>() {
+ @Override
+ public boolean apply(VPC input) {
+ found.incrementAndGet();
+ return !isNullOrEmpty(input.id());
+ }
+ }), "All vpcs must have at least the 'id' field populated");
+ assertTrue(found.get() > 0, "Expected some vpc to be returned");
+ }
+
+ private VPCApi api() {
+ return api.vpcApi();
+ }
added missing
--
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/443#discussion_r211981335
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ public abstract Date creationTime();
+
+ public abstract String description();
+
+ public abstract String zoneId();
+
+ public abstract String status();
+
+ public abstract int availableIpAddressCount();
+
+ public abstract String vpcId();
+
+ public abstract String id();
+
+ public abstract String name();
ok
--
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/443#discussion_r209217408
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -50,7 +51,11 @@ public void handleError(HttpCommand command, HttpResponse response) {
break;
case 401:
case 403:
- exception = new AuthorizationException(message, exception);
+ if (message.contains("\"Code\":\"DependencyViolation\"")) {
This looks very flaky. What if there is a space between the field name and the value? (as i the examples you copied). We need to do this better.
> @@ -50,7 +51,11 @@ public void handleError(HttpCommand command, HttpResponse response) {
break;
case 401:
case 403:
- exception = new AuthorizationException(message, exception);
+ if (message.contains("\"Code\":\"DependencyViolation\"")) {
This is also going to fail. Have you really tried this?
The message variable is built by reading the payload (the payload's InputStream), but in order to determine if the error is a dependency violation, you are already consuming that InputStream (you read the body) in the retry handler, so at this point, the message will probably be `null`.
As I already commented to you, if you read the body in a retry handler you need to take care of buffering the payload in memory so it can be read again later when needed, or make sure all retry & error handlers that may come next never try to read the body.
--
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/443#pullrequestreview-145564798
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 2 commits.
2d991e1 wip
88d83f3 more comments addressed
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/801266bac92ae8d6c54cdec540b1860b95f8cb94..88d83f336aefd7a8013dac58f79e5b9ea4ec27c9
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 4 commits.
701f36e add pagination to instanceStatus api
9dc04fb add network apis
ebd1642 improve CreateResourcesThenCreateNodes
b79a887 wip
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/88d085280a99fcc7bb799721b873661fbb3feaeb..b79a887eb460662edb19201977ae961f208dff88
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> @@ -54,6 +46,7 @@ protected Properties setupProperties() {
Properties properties = super.setupProperties();
vpcId = setIfTestSystemPropertyPresent(properties, provider + ".vpcId");
vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ group = "jclouds"; // otherwise jclouds will use provider name as group but `aliyun` is a forbidden prefix for tags
@nacx as it can't start with `aliyun` shall I use `alibaba-ecs` as id for the provider in the `ECSComputeServiceProviderMetadata` ?
--
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/443#discussion_r207836467
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ public abstract String description();
+
+ public abstract String regionId();
+
+ public abstract String status();
+
+ public abstract Map<String, List<UserCidr>> userCidrs();
+
+ public abstract String vRouterId();
+
+ public abstract Map<String, List<String>> vSwitchIds();
+
+ public abstract String id();
+
+ public abstract String name();
done
--
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/443#discussion_r209219842
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ final Image image = imageInRegionToImage.apply(ImageInRegion.create(Regions.EU_CENTRAL_1.getName(), ecsImage));
+ assertEquals(ecsImage.id(), image.getProviderId());
+ assertEquals(ecsImage.name(), image.getName());
+ assertEquals(Image.Status.AVAILABLE, image.getStatus());
+ final org.jclouds.compute.domain.OperatingSystem operatingSystem = image.getOperatingSystem();
+
+ assertEquals(ecsImage.osName(), operatingSystem.getName());
+ assertEquals(ecsImage.description(), operatingSystem.getDescription());
+ assertTrue(operatingSystem.is64Bit());
+ assertEquals(region, image.getLocation());
+ }
+
+ Date parseDate(final String dateString) {
+ return DatatypeConverter.parseDateTime(dateString).getTime();
+ }
done
--
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/443#discussion_r211980875
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.ResponseParser;
+import org.jclouds.rest.annotations.Transform;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import java.beans.ConstructorProperties;
+import java.util.List;
+import java.util.Map;
+
thanks @danielestevez added the URL for that
--
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/443#discussion_r209215893
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
I don't see the value in having jclouds deleting VPCs that were not created by jclouds. We should always keep the resources created by the user by the provider API/Portal or 3rd party apps
What case of use would make sense here?
--
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/443#discussion_r208733508
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +@AutoValue
+public abstract class EipAddress {
+
+ EipAddress() {
+ }
+
+ @SerializedNames({ "IpAddress", "AllocationId", "InternetChargeType" })
+ public static EipAddress create(String ipAddress, String allocationId, String internetChargeType) {
+ return new AutoValue_EipAddress(ipAddress, allocationId, internetChargeType);
+ }
+
+ public abstract String ipAddress();
+
+ public abstract String allocationId();
+
+ public abstract String internetChargeType();
don't know, they are not well documented
--
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/443#discussion_r207833707
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks, @andreaturli!
There are many comments but in general, it looks pretty good.
Apart from the comments, we need proper tests for most of the things. Basically, every class in the `compute/functions` package needs to have a proper unit test.
Also, add unit tests for every utility class and small function you have created.
> +Aliyun ECS provider works exactly as any other jclouds provider.
+Notice that as Aliyun supports dozens of locations and to limit the scope of some operations, one may want to use:
+
+and
+```bash
+jclouds.regions
+```
+which is by default `null`. If you want to target only the `north europe` region, you can use
+
+```bash
+jclouds.regions="eu-central-1"
+```
+
+# Setting Up Test Environment
+
+Get or create the `User Access Key` and `Access Key Secret `for your account at `https://usercenter.console.aliyun.com/#/manage/ak`
[nit] Remove the extra spaces after "secret"
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String group, String name, Template template) {
+ String instanceType = template.getHardware().getId();
+ String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
This seems to be mandatory, but its presence is not enforced anywhere. Should we validate it is there in the create nodes strategy and fail with an appropriate message if missing?
It is not good to have mandatory options that are not part of the abstraction. It would be better if we could automatically create one vSwitch or have a default value instead of requiring users to always provide this particular template option.
> + String instanceType = template.getHardware().getId();
+ String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
+ Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+ TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
The two charge field types should be custom template options.
> + String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
+ Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+ TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
Same here.
> + String vSwitchId = templateOptions.getVSwitchId();
+ Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+ TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
You want to check the result of the predicate and fail if it returned `false`.
> +
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
+ api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+ api.instanceApi().powerOn(instanceRequest.getInstanceId());
+ instanceRunningPredicate.apply(regionAndInstanceId);
You want to check the result of the predicate and fail if it returned `false`.
> + String vSwitchId = templateOptions.getVSwitchId();
+ Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+ TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
Also, when failing after having called the instance create API, we should rollback all the created resources.
> +
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
+ api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+ api.instanceApi().powerOn(instanceRequest.getInstanceId());
+ instanceRunningPredicate.apply(regionAndInstanceId);
Failing here should rollback the public ip too.
> + transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
+
+ for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate<InstanceType>() {
+ @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
Don't call `build` in every iteration. Build the list outside.
> + for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate<InstanceType>() {
+ @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List<String> getInstanceTypeId(String regionId) {
Rename to plural.
> + @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List<String> getInstanceTypeId(String regionId) {
+ List<String> instanceTypeIds = Lists.newArrayList();
+ for (AvailableZone availableZone : api.instanceApi().listInstanceTypesByAvailableZone(regionId)) {
+ for (AvailableResource availableResource : availableZone.availableResources().get("AvailableResource")) {
+ for (SupportedResource supportedResource : availableResource.supportedResources()
+ .get("SupportedResource")) {
+ if ("Available".equals(supportedResource.status())) {
It would be preferred if the status was an enum
> + }
+ }
+ }
+ return instanceTypeIds;
+ }
+
+ @Override
+ public Iterable<Image> listImages() {
+ final ImmutableList.Builder<Image> images = ImmutableList.builder();
+ final List<String> availableLocationNames = newArrayList(
+ transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
This `availableLocation` should be refactored to its own method, as the same lines are used in several places.
> +
+ for (String regionId : availableLocationNames) {
+ images.addAll(api.imageApi().list(regionId).concat());
+ }
+ return images.build();
+ }
+
+ @Override
+ public Image getImage(final String id) {
+ Optional<Image> firstInterestingImage = Iterables.tryFind(listImages(), new Predicate<Image>() {
+ public boolean apply(Image input) {
+ return input.id().equals(id);
+ }
+ });
+ if (!firstInterestingImage.isPresent()) {
+ throw new IllegalStateException("Cannot find image with the required slug " + id);
We'd better not fail here and just return `null`.
> + transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
+
+ for (String regionId : availableLocationNames) {
+ instances.addAll(api.instanceApi().list(regionId).concat());
+ }
+ return instances.build();
+ }
+
+ @Override
+ public Iterable<Instance> listNodesByIds(final Iterable<String> ids) {
+ return filter(listNodes(), new Predicate<Instance>() {
Use the API method and pass all given ids as `ListInstancesOptions`.
> +
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
+ api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+ api.instanceApi().powerOn(instanceRequest.getInstanceId());
+ instanceRunningPredicate.apply(regionAndInstanceId);
Also, do we really need for the instance to be powered on to retrieve it? Can we just not wait, get the instance, and let the default jclouds waiting code to the active waiting?
> + @Override
+ public boolean apply(String input) {
+ return label.contains(input);
+ }
+ }).transform(new Function<String, OsFamily>() {
+ @Override
+ public OsFamily apply(String input) {
+ return OTHER_OS_MAP.get(input);
+ }
+ });
+ }
+
+ @Override
+ public org.jclouds.compute.domain.Image apply(Image input) {
+ ImageBuilder builder = new ImageBuilder();
+ builder.ids(input.id());
Are images global? Or do we need to prefix their IDs with the region too?
> + private final Supplier<Map<String, ? extends Hardware>> hardwares;
+ private final Supplier<Set<? extends Location>> locations;
+ private final Function<Instance.Status, NodeMetadata.Status> toPortableStatus;
+ private final GroupNamingConvention groupNamingConvention;
+ @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger logger = Logger.NULL;
+
+ @Inject
+ public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,
+ Supplier<Map<String, ? extends Image>> images,
+ Supplier<Map<String, ? extends Hardware>> hardwares,
+ @Memoized Supplier<Set<? extends Location>> locations,
+ Function<Instance.Status, NodeMetadata.Status> toPortableStatus,
+ GroupNamingConvention.Factory groupNamingConvention) {
+ this.instanceTypeToHardware = instanceTypeToHardware;
+ this.images = checkNotNull(images, "images cannot be null");
+ this.hardwares = checkNotNull(hardwares, "hardwares cannot be null");
Null checks are redundant in injected constructors. Remove them.
> +/**
+ * Transforms an {@link Instance} to the jclouds portable model.
+ */
+@Singleton
+public class InstanceToNodeMetadata implements Function<Instance, NodeMetadata> {
+
+ private final InstanceTypeToHardware instanceTypeToHardware;
+ private final Supplier<Map<String, ? extends Image>> images;
+ private final Supplier<Map<String, ? extends Hardware>> hardwares;
+ private final Supplier<Set<? extends Location>> locations;
+ private final Function<Instance.Status, NodeMetadata.Status> toPortableStatus;
+ private final GroupNamingConvention groupNamingConvention;
+ @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger logger = Logger.NULL;
+
+ @Inject
+ public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,
Remove the public modifier to leave this in default scope.
> + if (hardware.isPresent()) {
+ builder.hardware(hardware.get());
+ } else {
+ logger.info(">> hardware with id %s for instance %s was not found. "
+ + "This might be because the image that was used to create the instance has a new id.",
+ from.instanceType(), from.instanceId());
+ }
+
+ builder.id(RegionAndId.slashEncodeRegionAndId(from.regionId(), from.instanceId()));
+ builder.providerId(from.instanceId());
+ builder.name(from.instanceName());
+ builder.hostname(String.format("%s", from.hostname()));
+ builder.group(groupNamingConvention.extractGroup(from.instanceName()));
+ builder.status(toPortableStatus.apply(from.status()));
+ builder.privateAddresses(from.innerIpAddress().entrySet().iterator().next().getValue());
+ builder.publicAddresses(from.publicIpAddress().entrySet().iterator().next().getValue());
Why do you just get the first IP and not all the IPs in the set?
> +
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+import org.jclouds.compute.options.TemplateOptions;
+
+import static com.google.common.base.Objects.equal;
+
+/**
+ * Custom options for the Alibaba Elastic Compute Service API.
+ */
+public class ECSServiceTemplateOptions extends TemplateOptions implements Cloneable {
+
+ private String keyPairName = "";
+ private String vpcId = "";
+ private String vSwitchId = "";
+ private String userData = "";
Unused?
> +import org.jclouds.logging.Logger;
+import org.jclouds.rest.AuthorizationException;
+
+import javax.annotation.Resource;
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.util.List;
+
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED;
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_TERMINATED;
+
+@Singleton
+public class CleanupResources {
+
+ public static final String RESOURCE_TYPE = "securitygroup";
Change the constant name to `RESOURCE_TYPE_SECURITY_GROUP`.
> + } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroupId);
+ }
+ }
+
+ return instanceDeleted;
+ }
+
+ private List<String> getSecurityGroupIdsUsedByNode(RegionAndId regionAndId) {
+ List<String> securityGroupIds = Lists.newArrayList();
+ PaginatedCollection<Instance> instances = api.instanceApi().list(regionAndId.regionId(), ListInstancesOptions.Builder.instanceIds(regionAndId.id()));
+ if (instances.isEmpty()) return securityGroupIds;
+
+ Instance instance = Iterables.get(instances, 0);
+ if (instance != null && !instance.securityGroupIds().isEmpty()) {
+ securityGroupIds = instance.securityGroupIds().values().iterator().next();
Just one security group? Why not getting the entire set?
> +
+ String securityGroupId = null;
+ if (!options.getGroups().isEmpty()) {
+ Iterable<String> securityGroupNames = api.securityGroupApi().list(location.getId()).concat().transform(new Function<SecurityGroup, String>() {
+ @Override
+ public String apply(SecurityGroup input) {
+ return input.name();
+ }
+ });
+ for (String securityGroupName : options.getGroups()) {
+ checkState(Iterables.contains(securityGroupNames, securityGroupName), "Cannot find security group with name " + securityGroupName + ". \nSecurity groups available are: \n" + Iterables.toString(securityGroupNames)); // {
+ }
+ } else if (options.getInboundPorts().length > 0) {
+ String name = namingConvention.create().sharedNameForGroup(group);
+ SecurityGroupRequest securityGroupRequest = api.securityGroupApi().create(location.getId(),
+ CreateSecurityGroupOptions.Builder.securityGroupName(name).vpcId(options.getVpcId()));
The `vpcId` template option seems mandatory. LIke with the `vSwitch` one, options should not be mandatory. Can we provide a default value, or create one if no value has been provided?
At least, if this is not possible, we should validate the template options as a precondition to the `execute` method and fail soon with an understandable message.
> +@AutoValue
+public abstract class EipAddress {
+
+ EipAddress() {
+ }
+
+ @SerializedNames({ "IpAddress", "AllocationId", "InternetChargeType" })
+ public static EipAddress create(String ipAddress, String allocationId, String internetChargeType) {
+ return new AutoValue_EipAddress(ipAddress, allocationId, internetChargeType);
+ }
+
+ public abstract String ipAddress();
+
+ public abstract String allocationId();
+
+ public abstract String internetChargeType();
Candidate for an enum?
> +import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class DedicatedHostAttribute {
+
+ DedicatedHostAttribute() {
+ }
+
+ @SerializedNames({ "DedicatedHostId", "DedicatedHostName" })
+ public static DedicatedHostAttribute create(String dedicatedHostId, String dedicatedHostName) {
+ return new AutoValue_DedicatedHostAttribute(dedicatedHostId, dedicatedHostName);
+ }
+
+ public abstract String dedicatedHostId();
+
+ public abstract String dedicatedHostName();
Remove the `dedicatedHost` prefix in these fields. You are already in the `DedicatedHost` class.
> + hostname, instanceType, creationTime, status,
+ tags == null ? ImmutableMap.<String, List<Tag>>of() : ImmutableMap.copyOf(tags), clusterId, recyclable,
+ regionId, gpuSpec, dedicatedHostAttribute,
+ operationLocks == null ? ImmutableMap.<String, List<String>>of() : ImmutableMap.copyOf(operationLocks),
+ InstanceChargeType, gpuAmount, expiredTime);
+ }
+
+ public abstract Map<String, List<String>> innerIpAddress();
+
+ public abstract String imageId();
+
+ public abstract String instanceTypeFamily();
+
+ public abstract String vlanId();
+
+ @Nullable
We are enforcing presence in the constructor. This will never return `null`.
> +
+ public abstract Boolean recyclable();
+
+ public abstract String regionId();
+
+ public abstract String gpuSpec();
+
+ public abstract DedicatedHostAttribute dedicatedHostAttribute();
+
+ public abstract Map<String, List<String>> operationLocks();
+
+ public abstract String InstanceChargeType();
+
+ public abstract Integer gpuAmount();
+
+ public abstract Date expiredTime();
Are all these properties always (and in every case) present? Or do we need to add some `@Nullable` annotations?
> +
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class InstanceType {
+
+ InstanceType() {
+ }
+
+ @SerializedNames({ "InstanceTypeId", "CpuCoreCount", "MemorySize" })
+ public static InstanceType create(String instanceTypeId, Integer cpuCoreCount, Double memorySize) {
+ return new AutoValue_InstanceType(instanceTypeId, cpuCoreCount, memorySize);
+ }
+
+ public abstract String instanceTypeId();
Just `id`.
> +public abstract class NetworkInterface {
+
+ NetworkInterface() {
+ }
+
+ @SerializedNames({ "MacAddress", "PrimaryIpAddress", "NetworkInterfaceId" })
+ public static NetworkInterface create(String macAddress, String primaryIpAddress, String networkInterfaceId) {
+ return new AutoValue_NetworkInterface(macAddress, primaryIpAddress, networkInterfaceId);
+ }
+
+ @Nullable
+ public abstract String macAddress();
+
+ public abstract String primaryIpAddress();
+
+ public abstract String networkInterfaceId();
Just `id`.
> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.Enums;
+import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * The type of the ECS resource. All values must be lowercase.
Should we override the `toString()` method or similar to return them in lowercase when serializing?
> + public static final String KEYPAIR_PARAM = "KeyPairName";
+ public static final String INTERNET_CHARGE_TYPE_PARAM = "InternetChargeType";
+ public static final String INTERNET_MAX_BANDWITH_IN_PARAM = "InternetMaxBandwidthIn";
+ public static final String INTERNET_MAX_BANDWITH_OUT_PARAM = "InternetMaxBandwidthOut";
+ public static final String INSTANCE_CHARGE_TYPE_PARAM = "InstanceChargeType";
+
+ /**
+ * Configures the name of the instance to be returned.
+ */
+ public CreateInstanceOptions instanceName(String instanceName) {
+ queryParameters.put(INSTANCE_NAME_PARAM, instanceName);
+ return this;
+ }
+
+ /**
+ * Configures the number of the instances to be returned.
Wrong javadoc?
> + .join(Iterables.transform(Arrays.asList(innerIpAddresses), new Function<String, String>() {
+ @Override
+ public String apply(String s) {
+ return new StringBuilder(s.length() + 1).append('"').append(s).append('"').toString();
+ }
+ }));
+ queryParameters.put(INNER_IP_ADDRESSES_PARAM, String.format("[%s]", instanceIdsAsString));
+ return this;
+ }
+
+ public ListInstancesOptions publicIpAddresses(String... publicIpAddresses) {
+ String instanceIdsAsString = Joiner.on(",")
+ .join(Iterables.transform(Arrays.asList(publicIpAddresses), new Function<String, String>() {
+ @Override
+ public String apply(String s) {
+ return new StringBuilder(s.length() + 1).append('"').append(s).append('"').toString();
Extract this to a `quote` function or similar you can reuse and properly unit test.
> @@ -92,4 +94,14 @@ public boolean apply(@Nullable String input) {
}), "Cannot starts with " + Iterables.toString(FORBIDDEN_PREFIX));
}
+ /**
+ * This is strictly not needed but apparently tags with `-` can create a problem when using API, so I've decided to use
+ * base64 encoding
+ * @param value
+ * @return
+ */
+ public String encodeTag(String value) {
Make this method `static`.
> @@ -92,4 +94,14 @@ public boolean apply(@Nullable String input) {
}), "Cannot starts with " + Iterables.toString(FORBIDDEN_PREFIX));
}
+ /**
+ * This is strictly not needed but apparently tags with `-` can create a problem when using API, so I've decided to use
+ * base64 encoding
+ * @param value
+ * @return
Properly document the parameter and return value.
> + vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ return properties;
+ }
+
+ @Override
+ protected TemplateBuilder templateBuilder() {
+ return super.templateBuilder()
+ .options(vpcId(vpcId)
+ .vSwitchId(vSwitchId));
+ }
+
+ @Override
+ protected Template addRunScriptToTemplate(Template template) {
+ template.getOptions().runScript(Statements.newStatementList(
+ new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK() }));
+ return template;
Do we really need to override this to wait?
> +
+ @Override
+ protected Template addRunScriptToTemplate(Template template) {
+ template.getOptions().runScript(Statements.newStatementList(
+ new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK() }));
+ return template;
+ }
+
+ @Override
+ protected Module getSshModule() {
+ return new SshjSshClientModule();
+ }
+
+ @Override
+ @Test(expectedExceptions = AuthorizationException.class)
+ public void testCorrectAuthException() throws Exception {
Why do we need to override this test?
> + context.getComputeService().listImages();
+ } catch (AuthorizationException e) {
+ throw e;
+ } catch (RuntimeException e) {
+ e.printStackTrace();
+ throw e;
+ } finally {
+ if (context != null)
+ context.close();
+ }
+ }
+
+ @Override
+ public void testOptionToNotBlock() throws Exception {
+ // Aliyun ECS ComputeService implementation has to block until the node
+ // is provisioned, to be able to return it.
If we don't block after power on, we can probably remove this override. It would be a nice to have, as jclouds also provides non-blocking options when deploying nodes.
> + private String vSwitchId = "vsw-gw8c79bsp4ezbe34ij3w8";
+ private String securityGroupId;
+ private String instanceId;
+
+ @BeforeClass
+ public void setUp() {
+ SecurityGroupRequest request = api.securityGroupApi().create(Regions.EU_CENTRAL_1.getName(),
+ CreateSecurityGroupOptions.Builder.securityGroupName(InstanceApiLiveTest.class.getSimpleName())
+ );
+ securityGroupId = request.getSecurityGroupId();
+
+ Properties properties = super.setupProperties();
+ vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ }
+
+ @AfterClass
Use `alwaysRun + true` to make sure you always cleanup even if tests fail.
> +
+ @Test(groups = "live", dependsOnMethods = "testListInstance")
+ public void testCreate() {
+ InstanceRequest instanceRequest = api().create(Regions.EU_CENTRAL_1.getName(), imageId, securityGroupId, hostname, instanceType,
+ CreateInstanceOptions.Builder.vSwitchId(vSwitchId));
+ instanceId = instanceRequest.getInstanceId();
+ assertNotNull(instanceId, "Instance must not be null");
+ }
+
+ @Test(groups = "live", dependsOnMethods = "testCreate")
+ public void testGet() {
+ Instance instance = Iterables.getOnlyElement(api().list(Regions.EU_CENTRAL_1.getName(), ListInstancesOptions.Builder.instanceIds(instanceId)));
+ assertNotNull(instance.instanceId(), "Instance must not be null");
+ }
+
+ @Test(groups = "live", dependsOnMethods = "testGet")
Can depend just on the `testCreate`?
> + public void testListInstances() throws InterruptedException {
+ server.enqueue(jsonResponse("/instances-first.json"));
+ server.enqueue(jsonResponse("/instances-last.json"));
+
+ Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+ assertEquals(size(instances), 20); // Force the PagedIterable to advance
+ assertEquals(server.getRequestCount(), 2);
+ assertSent(server, "GET", "DescribeInstances");
+ assertSent(server, "GET", "DescribeInstances", 2);
+ }
+
+ public void testListInstancesReturns404() {
+ server.enqueue(response404());
+ Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+ assertTrue(isEmpty(instances));
+ assertEquals(server.getRequestCount(), 1);
Verify the request that has been sent too.
> +
+ public void testListInstanceTypes() throws InterruptedException {
+ server.enqueue(jsonResponse("/instanceTypes.json"));
+
+ List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+ assertEquals(size(instanceTypes), 308);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeInstanceTypes");
+ }
+
+ public void testListInstanceTypesReturns404() {
+ server.enqueue(response404());
+ List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+ assertTrue(isEmpty(instanceTypes));
+ assertEquals(server.getRequestCount(), 1);
+ }
There are a lot of missing methods in this test 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/443#pullrequestreview-142434557
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
thanks @nacx, merging it now
--
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/443#issuecomment-420501158
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
merger at [master](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=a5dbf0065d8fa8cabcaf020b7c10fe2f7ccf8d6a)
--
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/443#issuecomment-420508064
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +
+ public DependencyViolationException() {
+ super();
+ }
+
+ public DependencyViolationException(String arg0, Throwable arg1) {
+ super(arg0, arg1);
+ }
+
+ public DependencyViolationException(String arg0) {
+ super(arg0);
+ }
+
+ public DependencyViolationException(Throwable arg0) {
+ super(arg0);
+ }
Put readable names to all method arguments.
> VPCRequest create(@QueryParam("RegionId") String region, CreateVPCOptions vpcOptions);
@Named("vpc:delete")
@POST
@QueryParams(keys = "Action", values = "DeleteVpc")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Actually, we use fallbacks on DELETE operations. If we want to delete a resource that is no longer there, just don't fail as we are already in the desired status.
> @@ -138,7 +139,6 @@ VSwitchRequest create(@QueryParam("ZoneId") String zone,
@Named("vswitch:delete")
@POST
@QueryParams(keys = "Action", values = "DeleteVSwitch")
- @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Same here.
> +import org.jclouds.http.HttpCommand;
+import org.jclouds.http.HttpResponse;
+import org.jclouds.http.handlers.RateLimitRetryHandler;
+
+import javax.inject.Inject;
+import javax.inject.Singleton;
+
+import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream;
+
+/**
+ * Handles 403 DependencyViolation.
+ * <p>
+ */
+@Beta
+@Singleton
+public class ECSDependencyViolationRetryHandler extends RateLimitRetryHandler {
I think this is not correct. It is not a rate limit error. You should just implement a backoff based retry handler.
Also, the `millisToNextAvailableRequest` method reads the `Retry-After` header. Is that header present at all? How could the backend know when the dependency would be satisfied?
It makes no sense to me to extend the rate limit handler class. This has to be changed to a retry handler that implements a backoff based retry.
> + private static final String RETRYABLE_ERROR_CODE = "DependencyViolation";
+
+ @Resource
+ protected Logger logger = Logger.NULL;
+ private final ParseJson<ErrorMessage> parseError;
+
+ @Inject
+ ECSRetryableErrorHandler(ParseJson<ErrorMessage> parseError) {
+ this.parseError = parseError;
+ }
+
+ @Override
+ public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ // Only consider retryable errors and discard rate limit ones
+ if (response.getStatusCode() != 403) {
+ return false;
You should call `super` here. This class just handles 403 errors; otherwise fallback to the default class that handles retries.
> + public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
+ // Only consider retryable errors and discard rate limit ones
+ if (response.getStatusCode() != 403) {
+ return false;
+ }
+
+ try {
+ // Note that this will consume the response body. At this point,
+ // subsequent retry handlers or error handlers will not be able to read
+ // again the payload, but that should only be attempted when the
+ // command is not retryable and an exception should be thrown.
+ ErrorMessage error = parseError.apply(response);
+ logger.debug("processing error: %s", error);
+
+ boolean isRetryable = RETRYABLE_ERROR_CODE.equals(error.code());
+ return isRetryable ? super.shouldRetryRequest(command, response) : false;
This seems to be wrong. If it IS a dependency violation, then impose the backoff and just retry. If it is not, then we don't handle it, delegate to the parent class.
> public class BaseECSComputeServiceApiLiveTest extends BaseApiLiveTest<ECSComputeServiceApi> {
+ protected static final String TEST_REGION = Regions.EU_CENTRAL_1.getName();
Make this readable form a property that can be passed int he command-line.
--
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/443#pullrequestreview-145559508
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> @@ -92,4 +94,14 @@ public boolean apply(@Nullable String input) {
}), "Cannot starts with " + Iterables.toString(FORBIDDEN_PREFIX));
}
+ /**
+ * This is strictly not needed but apparently tags with `-` can create a problem when using API, so I've decided to use
+ * base64 encoding
+ * @param value
+ * @return
+ */
+ public String encodeTag(String value) {
not needed anymore, thx
--
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/443#discussion_r207255836
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + private final Supplier<Map<String, ? extends Hardware>> hardwares;
+ private final Supplier<Set<? extends Location>> locations;
+ private final Function<Instance.Status, NodeMetadata.Status> toPortableStatus;
+ private final GroupNamingConvention groupNamingConvention;
+ @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger logger = Logger.NULL;
+
+ @Inject
+ public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,
+ Supplier<Map<String, ? extends Image>> images,
+ Supplier<Map<String, ? extends Hardware>> hardwares,
+ @Memoized Supplier<Set<? extends Location>> locations,
+ Function<Instance.Status, NodeMetadata.Status> toPortableStatus,
+ GroupNamingConvention.Factory groupNamingConvention) {
+ this.instanceTypeToHardware = instanceTypeToHardware;
+ this.images = checkNotNull(images, "images cannot be null");
+ this.hardwares = checkNotNull(hardwares, "hardwares cannot be null");
ok
--
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/443#discussion_r207135462
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
> + <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.jetbrains</groupId>
+ <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.jetbrains</groupId>
+ <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
Remove all this
> .instanceName(name)
.keyPairName(keyPairName)
.tagOptions(tagOptions)
);
- String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
- instanceSuspendedPredicate.apply(regionAndInstanceId);
+ String regionAndInstanceId = slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ if (!instanceSuspendedPredicate.apply(regionAndInstanceId)) {
Add some log here explaining what went wrong.
> }
@Override
- public Image getImage(final String id) {
- Optional<Image> firstInterestingImage = Iterables.tryFind(listImages(), new Predicate<Image>() {
- public boolean apply(Image input) {
- return input.id().equals(id);
- }
- });
- if (!firstInterestingImage.isPresent()) {
- throw new IllegalStateException("Cannot find image with the required slug " + id);
- }
- return firstInterestingImage.get();
+ public ImageInRegion getImage(final String id) {
+ RegionAndId regionAndId = fromSlashEncoded(id);
+ return ImageInRegion.create(regionAndId.regionId(), Iterables.getFirst(api.imageApi().list(regionAndId.regionId(),
This returns an object even if the image does not exist. That-s not correct. Just return `null`.
> @@ -54,6 +46,7 @@ protected Properties setupProperties() {
Properties properties = super.setupProperties();
vpcId = setIfTestSystemPropertyPresent(properties, provider + ".vpcId");
vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ group = "jclouds"; // otherwise jclouds will use provider name as group but `aliyun` is a forbidden prefix for tags
Then we should probably preconfigure the `prefix` property in the provider metadata too, so users don't have to do it manually. We must provide defaults that work.
> +// expect(serverApi.getServer(serverId)).andReturn(server);
+// }
+//
+// private void applyWithExpectedErrorMessage(String expectedErrorMessage) {
+// try {
+// cleanupServer.apply(serverId);
+// } catch (IllegalStateException e) {
+// assertEquals(expectedErrorMessage, e.getMessage());
+// }
+// }
+//
+// private void networkApiExpectations() {
+// expect(api.getNetworkApi()).andReturn(networkApi);
+// }
+//
+//}
Why is all this commented?
> +
+ final Image image = imageInRegionToImage.apply(ImageInRegion.create(Regions.EU_CENTRAL_1.getName(), ecsImage));
+ assertEquals(ecsImage.id(), image.getProviderId());
+ assertEquals(ecsImage.name(), image.getName());
+ assertEquals(Image.Status.AVAILABLE, image.getStatus());
+ final org.jclouds.compute.domain.OperatingSystem operatingSystem = image.getOperatingSystem();
+
+ assertEquals(ecsImage.osName(), operatingSystem.getName());
+ assertEquals(ecsImage.description(), operatingSystem.getDescription());
+ assertTrue(operatingSystem.is64Bit());
+ assertEquals(region, image.getLocation());
+ }
+
+ Date parseDate(final String dateString) {
+ return DatatypeConverter.parseDateTime(dateString).getTime();
+ }
Add tests that check the special cases such as the `OTHER_OS_MAP`.
> + .resourceGroupId("resourceGroupId")
+ .osType("osType")
+ .osName("osName")
+ .instanceNetworkType("instanceNetworkType")
+ .hostname("hostname")
+ .creationTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T05:16:45.784120972Z"))
+ .status(Instance.Status.RUNNING)
+ .clusterId("clusterId")
+ .recyclable(false)
+ .gpuSpec("")
+ .dedicatedHostAttribute(DedicatedHostAttribute.create("id", "name"))
+ .instanceChargeType("instanceChargeType")
+ .gpuAmount(1)
+ .expiredTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T09:16:45.784120972Z"))
+ .innerIpAddress(ImmutableMap.<String, List<String>>of("IpAddress", ImmutableList.of("192.168.0.1")))
+ .publicIpAddress(ImmutableMap.<String, List<String>>of("IpAddress", ImmutableList.of("47.254.152.220")))
Add more values to the addresses to make sure to test we them all.
> + }
+
+ private void assertNodeMetadata(NodeMetadata result, org.jclouds.compute.domain.OperatingSystem os, String imageId,
+ NodeMetadata.Status status, List<String> privateIpAddresses, List<String> publicIpAddresses) {
+ assertNotNull(result);
+ assertEquals(result.getProviderId(), instance.id());
+ assertEquals(result.getName(), instance.name());
+ assertEquals(result.getHostname(), instance.hostname());
+ assertEquals(result.getGroup(), "[" + serverName + "]");
+ assertEquals(result.getHardware(), hardware);
+ assertEquals(result.getOperatingSystem(), os);
+ assertEquals(result.getLocation(), location);
+ assertEquals(result.getImageId(), RegionAndId.slashEncodeRegionAndId(regionId, imageId));
+ assertEquals(result.getStatus(), status);
+ assertEquals(result.getPrivateAddresses(), privateIpAddresses);
+ assertEquals(result.getPublicAddresses(), publicIpAddresses);
There is custom logic to extract the tags. Make sure you test tags too.
Also add a couple tests to verify what happens when the image and hardware are not found.
> +import org.jclouds.logging.Logger;
+import org.jclouds.rest.AuthorizationException;
+
+import javax.annotation.Resource;
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.util.List;
+
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED;
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_TERMINATED;
+
+@Singleton
+public class CleanupResources {
+
+ public static final String RESOURCE_TYPE = "securitygroup";
Rename this.
> +@AutoValue
+public abstract class EipAddress {
+
+ EipAddress() {
+ }
+
+ @SerializedNames({ "IpAddress", "AllocationId", "InternetChargeType" })
+ public static EipAddress create(String ipAddress, String allocationId, String internetChargeType) {
+ return new AutoValue_EipAddress(ipAddress, allocationId, internetChargeType);
+ }
+
+ public abstract String ipAddress();
+
+ public abstract String allocationId();
+
+ public abstract String internetChargeType();
Are these values fixed so we can have them in an enum?
> + public void testListInstances() throws InterruptedException {
+ server.enqueue(jsonResponse("/instances-first.json"));
+ server.enqueue(jsonResponse("/instances-last.json"));
+
+ Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+ assertEquals(size(instances), 20); // Force the PagedIterable to advance
+ assertEquals(server.getRequestCount(), 2);
+ assertSent(server, "GET", "DescribeInstances");
+ assertSent(server, "GET", "DescribeInstances", 2);
+ }
+
+ public void testListInstancesReturns404() {
+ server.enqueue(response404());
+ Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+ assertTrue(isEmpty(instances));
+ assertEquals(server.getRequestCount(), 1);
Still not addressed
> +
+ public void testListInstanceTypes() throws InterruptedException {
+ server.enqueue(jsonResponse("/instanceTypes.json"));
+
+ List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+ assertEquals(size(instanceTypes), 308);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeInstanceTypes");
+ }
+
+ public void testListInstanceTypesReturns404() {
+ server.enqueue(response404());
+ List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+ assertTrue(isEmpty(instanceTypes));
+ assertEquals(server.getRequestCount(), 1);
+ }
Still not addressed.
--
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/443#pullrequestreview-143506207
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@nacx it is a wip, so please don't bother to review it yet. I'll ping when I think it's ready for your review, ok?
--
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/443#issuecomment-414683491
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + if (hardware.isPresent()) {
+ builder.hardware(hardware.get());
+ } else {
+ logger.info(">> hardware with id %s for instance %s was not found. "
+ + "This might be because the image that was used to create the instance has a new id.",
+ from.instanceType(), from.instanceId());
+ }
+
+ builder.id(RegionAndId.slashEncodeRegionAndId(from.regionId(), from.instanceId()));
+ builder.providerId(from.instanceId());
+ builder.name(from.instanceName());
+ builder.hostname(String.format("%s", from.hostname()));
+ builder.group(groupNamingConvention.extractGroup(from.instanceName()));
+ builder.status(toPortableStatus.apply(from.status()));
+ builder.privateAddresses(from.innerIpAddress().entrySet().iterator().next().getValue());
+ builder.publicAddresses(from.publicIpAddress().entrySet().iterator().next().getValue());
it's a weird data structure, getValue returns a List! :D
--
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/443#discussion_r207135700
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+import org.jclouds.compute.options.TemplateOptions;
+
+import static com.google.common.base.Objects.equal;
+
+/**
+ * Custom options for the Alibaba Elastic Compute Service API.
+ */
+public class ECSServiceTemplateOptions extends TemplateOptions implements Cloneable {
+
+ private String keyPairName = "";
+ private String vpcId = "";
+ private String vSwitchId = "";
+ private String userData = "";
possibly, removing
--
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/443#discussion_r207135922
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + checkTagsInNodeEquals(node, tags);
+
+ getAnonymousLogger().info(
+ format("<< available node(%s) os(%s) in %ss", node.getId(), node.getOperatingSystem(), createSeconds));
+
+ watch.reset().start();
+
+ client.runScriptOnNode(nodeId, AdminAccess.builder().adminUsername("web").build(), nameTask("admin-web"));
+
+ long configureSeconds = watch.elapsed(TimeUnit.SECONDS);
+
+ getAnonymousLogger().info(
+ format(
+ "<< configured node(%s) in %ss",
+ nodeId,
+ configureSeconds));
I actually like the idea of using `python -m SimpleHttpServer` instead of installing `java` and `jetty` as it takes much longer
If we agree on this I think i can open a PR for jclouds/jclouds and change the ComputeServiceLiveTest for all of the providers
--
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/443#discussion_r208873439
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> +import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.ResponseParser;
+import org.jclouds.rest.annotations.Transform;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import java.beans.ConstructorProperties;
+import java.util.List;
+import java.util.Map;
+
Is there a documentation reference for this API?
--
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/443#pullrequestreview-144566565
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
> @@ -104,11 +117,39 @@ protected CreateResourcesThenCreateNodes(CreateNodeWithGroupEncodedIntoName addN
String regionId = template.getLocation().getId();
ECSServiceTemplateOptions options = template.getOptions().as(ECSServiceTemplateOptions.class);
+ Optional<SecurityGroup> securityGroupOptional = tryFindSecurityGroupInRegion(regionId, options.getGroups(), options.getInboundPorts());
+
+ if (securityGroupOptional.isPresent()) {
+ Optional<VSwitch> vSwitchOptional = tryFindVSwitchInVPC(regionId, securityGroupOptional.get().vpcId(), options.getVSwitchId());
+ if (!vSwitchOptional.isPresent()) {
+ throw new IllegalStateException("Cannot find a vSwitch in the VPC " + securityGroupOptional.get().vpcId() + " of the security group with id: " + securityGroupOptional.get().id());
+ }
+ options.securityGroups(securityGroupOptional.get().id());
+ checkArgument(securityGroupOptional.get().vpcId() != null, "Security group must be in a VPC");
This does not make sense here. You are already reading this value before.
> @@ -104,11 +117,39 @@ protected CreateResourcesThenCreateNodes(CreateNodeWithGroupEncodedIntoName addN
String regionId = template.getLocation().getId();
ECSServiceTemplateOptions options = template.getOptions().as(ECSServiceTemplateOptions.class);
+ Optional<SecurityGroup> securityGroupOptional = tryFindSecurityGroupInRegion(regionId, options.getGroups(), options.getInboundPorts());
+
+ if (securityGroupOptional.isPresent()) {
+ Optional<VSwitch> vSwitchOptional = tryFindVSwitchInVPC(regionId, securityGroupOptional.get().vpcId(), options.getVSwitchId());
+ if (!vSwitchOptional.isPresent()) {
+ throw new IllegalStateException("Cannot find a vSwitch in the VPC " + securityGroupOptional.get().vpcId() + " of the security group with id: " + securityGroupOptional.get().id());
+ }
+ options.securityGroups(securityGroupOptional.get().id());
+ checkArgument(securityGroupOptional.get().vpcId() != null, "Security group must be in a VPC");
+ checkArgument(securityGroupOptional.get().vpcId().equals(vSwitchOptional.get().vpcId()), "Security group and vSwitch must belong to the same VPC");
This looks redundant, as the condition will always be met unless their API is buggy?
> }
- private List<String> getSecurityGroupIdsUsedByNode(RegionAndId regionAndId) {
- List<String> securityGroupIds = Lists.newArrayList();
- PaginatedCollection<Instance> instances = api.instanceApi().list(regionAndId.regionId(), ListInstancesOptions.Builder.instanceIds(regionAndId.id()));
- if (instances.isEmpty()) return securityGroupIds;
-
- Instance instance = Iterables.get(instances, 0);
- if (instance != null && !instance.securityGroupIds().isEmpty()) {
- securityGroupIds = instance.securityGroupIds().values().iterator().next();
- }
- return securityGroupIds;
+ public boolean cleanupSecurityGroupIfOrphaned(final String regionId, String securityGroupId) {
+ return api.securityGroupApi().delete(regionId, securityGroupId) != null;
This does not take into account if the security group is orphaned.
> }
+ public boolean cleanupVSwitchIfOrphaned(final String regionId, String vSwitchId) {
+ return api.vSwitchApi().delete(regionId, vSwitchId) != null;
This does not take into account if the vSwitch is orphaned.
> - public boolean apply(@javax.annotation.Nullable Tag input) {
- return input.key().equalsIgnoreCase("owner") && input.value().equalsIgnoreCase("jclouds");
- }
- }).transform(new Function<Tag, Boolean>() {
- @Override
- public Boolean apply(@javax.annotation.Nullable Tag input) {
- Request request = api.securityGroupApi().delete(regionId, securityGroupId);
- return request != null;
- }
- }).or(Boolean.FALSE);
- } catch (AuthorizationException e) {
- logger.error(">> security group: (%s) can not be deleted.\nReason: %s", securityGroupId, e.getMessage());
- return Boolean.FALSE;
- }
+ public boolean cleanupVPCIfOrphaned(final String regionId, String vpcId) {
+ return api.vpcApi().delete(regionId, vpcId) != null;
This does not take into account if the VPC is orphaned.
> }
- return securityGroupId;
+ return Optional.absent();
+ }
+
+ private Optional<VSwitch> tryFindVSwitchInRegion(String regionId, String vSwitchId) {
+ if (Strings.isNullOrEmpty(vSwitchId)) {
+ return api.vSwitchApi().list(regionId).concat().firstMatch(Predicates.<VSwitch>notNull());
Is it correct to connect our nodes to _any_ vSwitch we found, if none was provided? We could be just connecting things to existing isolated networks that have a very concrete purpose. I wouldn't do this and always create a vSwitch for jclouds if none was provided.
> - return securityGroupId;
+ return Optional.absent();
+ }
+
+ private Optional<VSwitch> tryFindVSwitchInRegion(String regionId, String vSwitchId) {
+ if (Strings.isNullOrEmpty(vSwitchId)) {
+ return api.vSwitchApi().list(regionId).concat().firstMatch(Predicates.<VSwitch>notNull());
+ } else {
+ return api.vSwitchApi().list(regionId, ListVSwitchesOptions.Builder.vSwitchId(vSwitchId)).firstMatch(Predicates.<VSwitch>notNull());
+ }
+ }
+
+ private Optional<VSwitch> tryFindVSwitchInVPC(String regionId, String vpcId, String vSwitchId) {
+ checkNotNull(vpcId, "vpcId");
+ ListVSwitchesOptions listVSwitchesOptions = Strings.isNullOrEmpty(vSwitchId) ?
+ ListVSwitchesOptions.Builder.vpcId(vpcId) :
Same here. We should always have a vSwitchId present when calling this?
> @@ -23,6 +23,10 @@
@AutoValue
public abstract class Tag {
+ public static final String DEFAULT_OWNER_KEY = "owner";
+ public static final String DEFAULT_OWNER_VALUE = "jclouds";
+ public static final String GROUP = "group";
Do we need a key for the vSwitch tag?
> + * 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.aliyun.ecs.domain;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class UserCidr {
+
+ UserCidr() {}
No properties?
> +
+ public abstract String description();
+
+ public abstract String regionId();
+
+ public abstract String status();
+
+ public abstract Map<String, List<UserCidr>> userCidrs();
+
+ public abstract String vRouterId();
+
+ public abstract Map<String, List<String>> vSwitchIds();
+
+ public abstract String id();
+
+ public abstract String name();
This has many properties. Add a builder.
> +
+ public abstract Date creationTime();
+
+ public abstract String description();
+
+ public abstract String zoneId();
+
+ public abstract String status();
+
+ public abstract int availableIpAddressCount();
+
+ public abstract String vpcId();
+
+ public abstract String id();
+
+ public abstract String name();
Add a builder.
> + */
+ public CreateVPCOptions description(String description) {
+ queryParameters.put(DESCRIPTION_PARAM, description);
+ return this;
+ }
+
+ /**
+ * Configures a client token used to guarantee the idempotence of request.
+ */
+ public CreateVPCOptions clientToken(String clientToken) {
+ queryParameters.put(CLIENT_TOKEN_PARAM, clientToken);
+ return this;
+ }
+
+ /**
+ * Configures the internet max bandwidth in of the instances to be returned.
Wrong javadoc.
> +
+public class CreateVSwitchOptions extends BaseHttpRequestOptions {
+ public static final String VSWITCH_NAME_PARAM = "VSwitchName";
+ public static final String DESCRIPTION_PARAM = "Description";
+ public static final String CLIENT_TOKEN_PARAM = "ClientToken";
+
+ /**
+ * Configures the name of the VSwitch.
+ */
+ public CreateVSwitchOptions vSwitchName(String vSwitchName) {
+ queryParameters.put(VSWITCH_NAME_PARAM, vSwitchName);
+ return this;
+ }
+
+ /**
+ * Configures the description of the VPC.
Wrong javadoc.
> + @Override
+ public IterableWithMarker<VPC> apply(Object input) {
+ ListVPCsOptions options = original == null ?
+ ListVPCsOptions.Builder.paginationOptions(PaginationOptions.class.cast(input)) :
+ original.paginationOptions(PaginationOptions.class.cast(input));
+ return api.vpcApi().list(regionId, options);
+ }
+ };
+ }
+ }
+ }
+
+ @Named("vpc:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVpc")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
_sigh_
Remove fallbacks in POST.
> + }
+ };
+ }
+ }
+ }
+
+ @Named("vpc:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVpc")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+ VPCRequest create(@QueryParam("RegionId") String region);
+
+ @Named("vpc:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVpc")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Remove
> + @Override
+ public IterableWithMarker<VSwitch> apply(Object input) {
+ ListVSwitchesOptions options = original == null ?
+ ListVSwitchesOptions.Builder.paginationOptions(PaginationOptions.class.cast(input)) :
+ original.paginationOptions(PaginationOptions.class.cast(input));
+ return api.vSwitchApi().list(regionId, options);
+ }
+ };
+ }
+ }
+ }
+
+ @Named("vswitch:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVSwitch")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Remove.
> + }
+ }
+ }
+
+ @Named("vswitch:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVSwitch")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+ VSwitchRequest create(@QueryParam("ZoneId") String zone,
+ @QueryParam("CidrBlock") String cidrBlock,
+ @QueryParam("VpcId") String vpcId);
+
+ @Named("vswitch:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateVSwitch")
+ @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Remove.
> + checkTagsInNodeEquals(node, tags);
+
+ getAnonymousLogger().info(
+ format("<< available node(%s) os(%s) in %ss", node.getId(), node.getOperatingSystem(), createSeconds));
+
+ watch.reset().start();
+
+ client.runScriptOnNode(nodeId, AdminAccess.builder().adminUsername("web").build(), nameTask("admin-web"));
+
+ long configureSeconds = watch.elapsed(TimeUnit.SECONDS);
+
+ getAnonymousLogger().info(
+ format(
+ "<< configured node(%s) in %ss",
+ nodeId,
+ configureSeconds));
This is not really honoring the purpose of the method, as it is not creating a service. What is the real problem with the base test? Jetty? Can we then change and run a different service? Something like a `python -m SimpleHTTPServer`?
I don't like this kind of overrides in tests. They alter the jclouds ComputeService impl contract, and it is unlikely we are revisiting this file once we fix some stuff in core/compute.
> +@Test(groups = "live", testName = "VPCApiLiveTest")
+public class VPCApiLiveTest extends BaseECSComputeServiceApiLiveTest {
+
+ public static final String VPC_NAME = "jclouds-vpc";
+
+ private String vpcId;
+
+ @BeforeClass
+ public void setUp() {
+ VPCRequest vpcRequest = api().create(Regions.EU_CENTRAL_1.getName(), CreateVPCOptions.Builder.vpcName(VPC_NAME));
+ assertNotNull(vpcRequest.getRequestId());
+ assertNotNull(vpcRequest.getVpcId());
+ vpcId = vpcRequest.getVpcId();
+ }
+
+ @AfterClass
always run.
> +
+ public void testListVPCsWithOptions() throws InterruptedException {
+ server.enqueue(jsonResponse("/vpcs-first.json"));
+ IterableWithMarker<VPC> vpcs = api.vpcApi().list(Regions.EU_CENTRAL_1.getName(), paginationOptions(pageNumber(1).pageSize(5)));
+ assertEquals(size(vpcs), 1);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeVpcs", ImmutableMap.of("RegionId", Regions.EU_CENTRAL_1.getName()), 1);
+ }
+
+ public void testListVPCsWithOptionsReturns404() throws InterruptedException {
+ server.enqueue(response404());
+ Iterable<VPC> vpcs = api.vpcApi().list(Regions.EU_CENTRAL_1.getName(), paginationOptions(pageNumber(2)));
+ assertTrue(isEmpty(vpcs));
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeVpcs", ImmutableMap.of("RegionId", Regions.EU_CENTRAL_1.getName()), 2);
+ }
Many mock tests missing.
> +
+ @BeforeClass
+ public void setUp() {
+ VPCRequest preRequisite = api.vpcApi().create(Regions.EU_CENTRAL_1.getName());
+ vpcId = preRequisite.getVpcId();
+ VSwitchRequest vpcRequest = api().create(
+ Regions.EU_CENTRAL_1.getName() + "a",
+ DEFAULT_CIDR_BLOCK,
+ vpcId,
+ CreateVSwitchOptions.Builder.vSwitchName(VSWITCH_NAME));
+ assertNotNull(vpcRequest.getRequestId());
+ assertNotNull(vpcRequest.getVSwitchId());
+ vSwitchId = vpcRequest.getVSwitchId();
+ }
+
+ @AfterClass
always run
> +
+ public void testList() {
+ final AtomicInteger found = new AtomicInteger(0);
+ assertTrue(Iterables.all(api().list(Regions.EU_CENTRAL_1.getName()).concat(), new Predicate<VSwitch>() {
+ @Override
+ public boolean apply(VSwitch input) {
+ found.incrementAndGet();
+ return !isNullOrEmpty(input.id());
+ }
+ }), "All vSwitches must have at least the 'id' field populated");
+ assertTrue(found.get() > 0, "Expected some vSwitch to be returned");
+ }
+
+ private VSwitchApi api() {
+ return api.vSwitchApi();
+ }
Many live tests missing.
> +
+ public void testList() {
+ final AtomicInteger found = new AtomicInteger(0);
+ assertTrue(Iterables.all(api().list(Regions.EU_CENTRAL_1.getName()).concat(), new Predicate<VPC>() {
+ @Override
+ public boolean apply(VPC input) {
+ found.incrementAndGet();
+ return !isNullOrEmpty(input.id());
+ }
+ }), "All vpcs must have at least the 'id' field populated");
+ assertTrue(found.get() > 0, "Expected some vpc to be returned");
+ }
+
+ private VPCApi api() {
+ return api.vpcApi();
+ }
Many live tests missing.
> +
+ public void testListVSwitchesWithOptions() throws InterruptedException {
+ server.enqueue(jsonResponse("/vswitches-first.json"));
+ IterableWithMarker<VSwitch> vSwitches = api.vSwitchApi().list(Regions.EU_CENTRAL_1.getName(), paginationOptions(pageNumber(1).pageSize(5)));
+ assertEquals(size(vSwitches), 1);
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeVSwitches", ImmutableMap.of("RegionId", Regions.EU_CENTRAL_1.getName()), 1);
+ }
+
+ public void testListVSwitchesWithOptionsReturns404() throws InterruptedException {
+ server.enqueue(response404());
+ IterableWithMarker<VSwitch> vSwitches = api.vSwitchApi().list(Regions.EU_CENTRAL_1.getName(), paginationOptions(pageNumber(2)));
+ assertTrue(isEmpty(vSwitches));
+ assertEquals(server.getRequestCount(), 1);
+ assertSent(server, "GET", "DescribeVSwitches", ImmutableMap.of("RegionId", Regions.EU_CENTRAL_1.getName()), 2);
+ }
Many mock tests missing.
> + .resourceGroupId("resourceGroupId")
+ .osType("osType")
+ .osName("osName")
+ .instanceNetworkType("instanceNetworkType")
+ .hostname("hostname")
+ .creationTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T05:16:45.784120972Z"))
+ .status(Instance.Status.RUNNING)
+ .clusterId("clusterId")
+ .recyclable(false)
+ .gpuSpec("")
+ .dedicatedHostAttribute(DedicatedHostAttribute.create("id", "name"))
+ .instanceChargeType("instanceChargeType")
+ .gpuAmount(1)
+ .expiredTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T09:16:45.784120972Z"))
+ .innerIpAddress(ImmutableMap.<String, List<String>>of("IpAddress", ImmutableList.of("192.168.0.1")))
+ .publicIpAddress(ImmutableMap.<String, List<String>>of("IpAddress", ImmutableList.of("47.254.152.220")))
Not yet addressed.
--
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/443#pullrequestreview-144747514
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> @@ -54,6 +46,7 @@ protected Properties setupProperties() {
Properties properties = super.setupProperties();
vpcId = setIfTestSystemPropertyPresent(properties, provider + ".vpcId");
vSwitchId = setIfTestSystemPropertyPresent(properties, provider + ".vSwitchId");
+ group = "jclouds"; // otherwise jclouds will use provider name as group but `aliyun` is a forbidden prefix for tags
Sounds good. Preconfigure that and remove this from here.
--
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/443#discussion_r207868271
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
Good point @danielestevez unfortunately tagApi don't work for VPC or VSwitch, only instances, images and few others. Trying to use the `description` although super ugly
--
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/443#discussion_r208706820
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
801266b addressing more comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/3205bbfc559eeeda59f2f7994f1aa179d16057e0..801266bac92ae8d6c54cdec540b1860b95f8cb94
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> public class BaseECSComputeServiceApiLiveTest extends BaseApiLiveTest<ECSComputeServiceApi> {
+ protected static final String TEST_REGION = Regions.EU_CENTRAL_1.getName();
ok done
--
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/443#discussion_r211107120
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
@nacx wdyt? I'm not completely sure about this default behaviour
--
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/443#pullrequestreview-144485695
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +// expect(serverApi.getServer(serverId)).andReturn(server);
+// }
+//
+// private void applyWithExpectedErrorMessage(String expectedErrorMessage) {
+// try {
+// cleanupServer.apply(serverId);
+// } catch (IllegalStateException e) {
+// assertEquals(expectedErrorMessage, e.getMessage());
+// }
+// }
+//
+// private void networkApiExpectations() {
+// expect(api.getNetworkApi()).andReturn(networkApi);
+// }
+//
+//}
still working on this, forget to exclude from commit, sorry
--
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/443#discussion_r207828836
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> @@ -23,6 +23,10 @@
@AutoValue
public abstract class Tag {
+ public static final String DEFAULT_OWNER_KEY = "owner";
+ public static final String DEFAULT_OWNER_VALUE = "jclouds";
+ public static final String GROUP = "group";
not sure we need that, as I'm adding the vswitchId tag using this code
```
Map<String, String> tags = ComputeServiceUtils.metadataAndTagsAsValuesOfEmptyString(templateOptions);
tags = new ImmutableMap.Builder()
.putAll(tags)
.put(vSwitchId, "")
.build();
```
--
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/443#discussion_r211107039
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
--
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/443#pullrequestreview-153710254
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ String securityGroupId = null;
+ if (!options.getGroups().isEmpty()) {
+ Iterable<String> securityGroupNames = api.securityGroupApi().list(location.getId()).concat().transform(new Function<SecurityGroup, String>() {
+ @Override
+ public String apply(SecurityGroup input) {
+ return input.name();
+ }
+ });
+ for (String securityGroupName : options.getGroups()) {
+ checkState(Iterables.contains(securityGroupNames, securityGroupName), "Cannot find security group with name " + securityGroupName + ". \nSecurity groups available are: \n" + Iterables.toString(securityGroupNames)); // {
+ }
+ } else if (options.getInboundPorts().length > 0) {
+ String name = namingConvention.create().sharedNameForGroup(group);
+ SecurityGroupRequest securityGroupRequest = api.securityGroupApi().create(location.getId(),
+ CreateSecurityGroupOptions.Builder.securityGroupName(name).vpcId(options.getVpcId()));
ok for validating them, not entirely sure about create a vSwitch if not available
--
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/443#discussion_r207199172
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List<String> getInstanceTypeId(String regionId) {
+ List<String> instanceTypeIds = Lists.newArrayList();
+ for (AvailableZone availableZone : api.instanceApi().listInstanceTypesByAvailableZone(regionId)) {
+ for (AvailableResource availableResource : availableZone.availableResources().get("AvailableResource")) {
+ for (SupportedResource supportedResource : availableResource.supportedResources()
+ .get("SupportedResource")) {
+ if ("Available".equals(supportedResource.status())) {
ok
--
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/443#discussion_r207133392
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +import org.jclouds.http.HttpRetryHandler;
+import org.jclouds.http.HttpUtils;
+import org.jclouds.http.annotation.ClientError;
+import org.jclouds.http.handlers.BackoffLimitedRetryHandler;
+import org.jclouds.json.Json;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.util.Set;
+
+import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream;
+
+/**
+ * Handles Retryable responses with error codes in the 4xx range
+ */
+public class ECSErrorRetryHandler implements HttpRetryHandler {
@nacx I think this implementation reflects better your suggestion, but please advice if it is not what you meant! thanks
--
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/443#pullrequestreview-148517914
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
Re-run the live tests, slightly better
Tests run: 55, Failures: 9, Errors: 0, Skipped: 6
`SshKeyPairApiLiveTest.testImport` - I'm afraid the API is broken
`ECSComputeServiceLiveTest.testCorrectAuthException` returns `ResourceNotFoundException` instead of `AuthorizationException` not entirely sure how to treat that.
the others are mostly related to java installation on centos7 failure.
```
Results :
Failed tests:
SshKeyPairApiLiveTest.testImport:58 » IllegalArgument {"RequestId":"4C152F57-E...
ECSComputeServiceLiveTest.testCorrectAuthException » Test
Method BaseComputeS...
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testCreateAndRunAService:747 » NoSuchElement
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testGet:553 » NoSuchElement
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testListSizes:888->BaseComputeServiceLiveTest.checkVolumes:893 {id=ecs.sn1.medium, providerId=ecs.sn1.medium, name=ecs.sn1.medium, processors=[{cores=2.0, speed=2.0}], ram=4096, hypervisor=none, supportsImage=Predicates.alwaysTrue()}
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testOptionToNotBlock:873 » Authorization
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testAScriptExecutionAfterBootWithBasicTemplate:249->BaseComputeServiceLiveTest.checkNodes:532->BaseComputeServiceLiveTest.sshPing:928->BaseComputeServiceLiveTest.doCheckJavaIsInstalledViaSsh:949 {output=bash: java: command not found
, error=, exitStatus=127}
```
--
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/443#issuecomment-416260449
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String group, String name, Template template) {
+ String instanceType = template.getHardware().getId();
+ String regionId = template.getLocation().getId();
+ String imageId = template.getImage().getId();
+
+ ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+ String keyPairName = templateOptions.getKeyPairName();
+ String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+ String vSwitchId = templateOptions.getVSwitchId();
We already do that in Azure. See [here](https://github.com/jclouds/jclouds/blob/master/providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CreateResourcesThenCreateNodes.java#L136). If the network does not exist, we create one with default CIDR values, and we let users override those defaults via properties. If creating a VPC is mostly about configuring the network and address space, I'd suggest following the same approachn here.
--
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/443#discussion_r207273170
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
88d0852 address comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/39fef5305d6cdfb660ecee32244eba7e7f5f68b5..88d085280a99fcc7bb799721b873661fbb3feaeb
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + transform(listLocations(), new Function<Region, String>() {
+ @Override
+ public String apply(Region location) {
+ return location.id();
+ }
+ }));
+
+ for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate<InstanceType>() {
+ @Override
+ public boolean apply(@Nullable InstanceType input) {
+ return contains(instanceTypeIds.build(), input.instanceTypeId());
thx
--
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/443#discussion_r207133342
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +// public String apply(Map.Entry<Integer, Integer> input) {
+// return String.format(PORT_RANGE_FORMAT, input.getKey(), input.getValue());
+// }
+// }));
+//
+// for (SecurityGroup securityGroup : api.securityGroupApi().list(regionId).concat()) {
+// List<Permission> permissions = api.securityGroupApi().get(regionId, securityGroup.id());
+// if (permissions.size() == portRangesAsString.size()) {
+// for (Permission permission : permissions) {
+// if (permission.ipProtocol() == IpProtocol.TCP && permission.sourceCidrIp().equals(INTERNET)) {
+// portRangesAsString.remove(permission.portRange());
+// }
+// }
+// if (portRangesAsString.isEmpty()) return Optional.of(securityGroup);
+// }
+// }
sorry removed
--
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/443#discussion_r211587632
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
flaky, indeed!
From the doc:
- costs: I think it's usage is for free
- Maximum number of VPCs per region: 10 (Submit a ticket to apply for more quota)
- other concerns: although fast, creating/destroying VPC adds a delay, but not crucial
--
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/443#discussion_r208855282
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
Closed #443.
--
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/443#event-1840127835
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroupId);
+ }
+ }
+
+ return instanceDeleted;
+ }
+
+ private List<String> getSecurityGroupIdsUsedByNode(RegionAndId regionAndId) {
+ List<String> securityGroupIds = Lists.newArrayList();
+ PaginatedCollection<Instance> instances = api.instanceApi().list(regionAndId.regionId(), ListInstancesOptions.Builder.instanceIds(regionAndId.id()));
+ if (instances.isEmpty()) return securityGroupIds;
+
+ Instance instance = Iterables.get(instances, 0);
+ if (instance != null && !instance.securityGroupIds().isEmpty()) {
+ securityGroupIds = instance.securityGroupIds().values().iterator().next();
again it's a list ;)
--
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/443#discussion_r207136327
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
This is a similar case as this https://github.com/jclouds/jclouds/pull/1202 (still unfinished) PR in ARM provider.
Basically we should be able to identify if those VPC/vSwitch were created by the jclouds providers and delete them only if empty AND created by us. We use tags in the ARM to check this, is there anything similar in Aliyun API?
--
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/443#discussion_r208698367
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId, name, instanceType,
+ CreateInstanceOptions.Builder
+ .vSwitchId(vSwitchId)
+ .internetChargeType("PayByTraffic")
+ .internetMaxBandwidthOut(5)
+ .instanceChargeType("PostPaid")
+ .instanceName(name)
+ .keyPairName(keyPairName)
+ .tagOptions(tagOptions)
+ );
+ String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+ instanceSuspendedPredicate.apply(regionAndInstanceId);
+ api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+ api.instanceApi().powerOn(instanceRequest.getInstanceId());
+ instanceRunningPredicate.apply(regionAndInstanceId);
I think we may skip it, thanks
--
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/443#discussion_r207117579
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ public void testList() {
+ final AtomicInteger found = new AtomicInteger(0);
+ assertTrue(Iterables.all(api().list(Regions.EU_CENTRAL_1.getName()).concat(), new Predicate<VSwitch>() {
+ @Override
+ public boolean apply(VSwitch input) {
+ found.incrementAndGet();
+ return !isNullOrEmpty(input.id());
+ }
+ }), "All vSwitches must have at least the 'id' field populated");
+ assertTrue(found.get() > 0, "Expected some vSwitch to be returned");
+ }
+
+ private VSwitchApi api() {
+ return api.vSwitchApi();
+ }
added missing
--
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/443#discussion_r211981309
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
here's the live tests result
```
Results :
Failed tests:
SshKeyPairApiLiveTest.testImport:58 » IllegalArgument {"RequestId":"4084B85E-D...
ECSComputeServiceLiveTest.testCorrectAuthException » Test
Method BaseComputeS...
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testCreateAndRunAService:747 » NoSuchElement
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testGet:553 » NoSuchElement
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testImageById:211 expected [{id=eu-central-1/centos_7_04_64_20G_alibase_20180419.vhd, providerId=centos_7_04_64_20G_alibase_20180419.vhd, name=centos_7_04_64_20G_alibase_20180419.vhd, location={scope=REGION, id=eu-central-1, description=欧洲中部 1 (法兰克福), parent=alibaba-ecs}, os={family=centos, name=CentOS 7.4 64位, version=7.4, description=, is64Bit=true}, description=, status=AVAILABLE, loginUser=root}] but found [null]
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testListSizes:888->BaseComputeServiceLiveTest.checkVolumes:893 {id=ecs.sn1.medium, providerId=ecs.sn1.medium, name=ecs.sn1.medium, processors=[{cores=2.0, speed=2.0}], ram=4096, hypervisor=none, supportsImage=Predicates.alwaysTrue()}
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testOptionToNotBlock:866 » RunNodes
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testWeCanCancelTasks:282 » RunNodes
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testAScriptExecutionAfterBootWithBasicTemplate:227 » RunNodes
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testConcurrentUseOfComputeServiceToCreateNodes:498 » Execution
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testCreateTwoNodesWithOneSpecifiedName:383 » RunNodes
ECSComputeServiceLiveTest>BaseComputeServiceLiveTest.testCreateAnotherNodeWithANewContextToEnsureSharedMemIsntRequired:435 » RunNodes
Tests run: 55, Failures: 12, Errors: 0, Skipped: 7
```
`SshKeyPairApiLiveTest` is a quite weird as by the doc it should work
I'll concentrate on `ECSComputeServiceLiveTest` failures of `{testGet, testImageById, testListSizes}` as they should be fine. `testCreateAndRunAService` fails because of jetty on centos7 issue
--
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/443#issuecomment-415086300
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.Enums;
+import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * The type of the ECS resource. All values must be lowercase.
done
--
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/443#discussion_r211981107
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.jetbrains</groupId>
+ <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.jetbrains</groupId>
+ <artifactId>annotations-java5</artifactId>
+ <version>RELEASE</version>
+ <scope>compile</scope>
+ </dependency>
where this crap come from? :(
--
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/443#discussion_r207828506
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
I can see an option to decide if jclouds would create AND delete resources needed for a deployment... something like a `invasive mode = on` but not sure about the implications.
Maybe as a different PR and keeping this with the automatic mode?
--
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/443#discussion_r208739432
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> + * 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.aliyun.ecs.domain;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class UserCidr {
+
+ UserCidr() {}
cannot get from API nor from doc
--
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/443#discussion_r209216400
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
0673eb8 fix NPE in InstanceToNodeMetadata
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/443/files/c7d38ec7172a488d785365e9d8d47953687fc3ef..0673eb83f306f8b4aa9463fa0bf4d34941280aa7
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
+1
I was thinking about giving the opportunity to decide whether to keep a VPC created by jclouds or delete it for every deployment.
--
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/443#discussion_r208738195
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +// public String apply(Map.Entry<Integer, Integer> input) {
+// return String.format(PORT_RANGE_FORMAT, input.getKey(), input.getValue());
+// }
+// }));
+//
+// for (SecurityGroup securityGroup : api.securityGroupApi().list(regionId).concat()) {
+// List<Permission> permissions = api.securityGroupApi().get(regionId, securityGroup.id());
+// if (permissions.size() == portRangesAsString.size()) {
+// for (Permission permission : permissions) {
+// if (permission.ipProtocol() == IpProtocol.TCP && permission.sourceCidrIp().equals(INTERNET)) {
+// portRangesAsString.remove(permission.portRange());
+// }
+// }
+// if (portRangesAsString.isEmpty()) return Optional.of(securityGroup);
+// }
+// }
Why this?
--
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/443#pullrequestreview-145563966
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s", deadNode);
}
+
+ List<SecurityGroup> securityGroups = cleanupResources.findOrphanedSecurityGroups(regionAndId.regionId(), deadNode.getGroup());
+ for (SecurityGroup securityGroup : securityGroups) {
+ logger.debug(">> destroying security group %s ...", securityGroup.id());
+ if (cleanupResources.cleanupSecurityGroupIfOrphaned(regionAndId.regionId(), securityGroup.id())) {
+ logger.debug(">> security group: (%s) has been deleted.", securityGroup.id());
+ } else {
+ logger.warn(">> security group: (%s) has not been deleted.", securityGroup.id());
+ }
+ }
+
+ // FIXME not sure it is correct to always delete vSwitch and VPC
thinking about it more, regardless of the tags, I think the question is whether is right to destroy a VPC every time jclouds deletes a group of nodes or not. I honestly don't know the answer as I can see value in both cases, so maybe an extra property configurable by the user?
--
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/443#discussion_r208719830
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +
+ for (String regionId : availableLocationNames) {
+ images.addAll(api.imageApi().list(regionId).concat());
+ }
+ return images.build();
+ }
+
+ @Override
+ public Image getImage(final String id) {
+ Optional<Image> firstInterestingImage = Iterables.tryFind(listImages(), new Predicate<Image>() {
+ public boolean apply(Image input) {
+ return input.id().equals(id);
+ }
+ });
+ if (!firstInterestingImage.isPresent()) {
+ throw new IllegalStateException("Cannot find image with the required slug " + id);
ok
--
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/443#discussion_r207133407