Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
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)
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)
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)
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)
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)
@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)
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)
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)
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)
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)
andreaturli commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials
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)
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() {
+ @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)
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() {
+ @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)
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)
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)
@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)
@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)
andreaturli commented on this pull request.
> +// public String apply(Map.Entry input) {
+//return String.format(PORT_RANGE_FORMAT, input.getKey(),
input.getValue());
+// }
+// }));
+//
+// for (SecurityGroup securityGroup :
api.securityGroupApi().list(regionId).concat()) {
+// List 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)
andreaturli commented on this pull request.
> public class BaseECSComputeServiceApiLiveTest extends
> BaseApiLiveTest {
+ 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)
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 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)
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)
nacx commented on this pull request.
> +// public String apply(Map.Entry input) {
+//return String.format(PORT_RANGE_FORMAT, input.getKey(),
input.getValue());
+// }
+// }));
+//
+// for (SecurityGroup securityGroup :
api.securityGroupApi().list(regionId).concat()) {
+// List 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)
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.
+ *
+ */
+@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 parseError;
+
+ @Inject
+ ECSRetryableErrorHandler(ParseJson 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 {
+ 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)
andreaturli commented on this pull request. > + + public abstract String description(); + + public abstract String regionId(); + + public abstract String status(); + + public abstract Map> userCidrs(); + + public abstract String vRouterId(); + + public abstract Map> 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)
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)
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)
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)
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)
@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)
@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)
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)
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 securityGroupOptional =
tryFindSecurityGroupInRegion(regionId, options.getGroups(),
options.getInboundPorts());
+
+ if (securityGroupOptional.isPresent()) {
+ Optional 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 securityGroupOptional =
tryFindSecurityGroupInRegion(regionId, options.getGroups(),
options.getInboundPorts());
+
+ if (securityGroupOptional.isPresent()) {
+ Optional 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 getSecurityGroupIdsUsedByNode(RegionAndId regionAndId)
{
- List securityGroupIds = Lists.newArrayList();
- PaginatedCollection 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() {
-@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 tryFindVSwitchInRegion(String regionId, String
vSwitchId) {
+ if (Strings.isNullOrEmpty(vSwitchId)) {
+ return
api.vSwitchApi().list(regionId).concat().firstMatch(Predicates.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;
+
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
nacx commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
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)
danielestevez commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
andreaturli commented on this pull request.
> } catch (Exception ex) {
logger.warn(ex, "Error cleaning up resources for node %s",
deadNode);
}
+
+ List 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)
@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)
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)
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)
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)
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)
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)
andreaturli commented on this pull request. > +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + 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)
nacx requested changes on this pull request.
> +annotations-java5
+RELEASE
+compile
+
+
+org.jetbrains
+annotations-java5
+RELEASE
+compile
+
+
+org.jetbrains
+annotations-java5
+RELEASE
+compile
+
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 firstInterestingImage = Iterables.tryFind(listImages(),
new Predicate() {
- 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.>of("IpAddress", ImmutableList.of("192.168.0.1")))
+ .publicIpAddress(ImmutableMap.>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
privateIpAddresses, List publicIpAddresses) {
+ assertNotNull(result);
+
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
@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)
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)
nacx commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials
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)
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)
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)
andreaturli commented on this pull request.
> +
+ String securityGroupId = null;
+ if (!options.getGroups().isEmpty()) {
+ Iterable securityGroupNames =
api.securityGroupApi().list(location.getId()).concat().transform(new
Function() {
+@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)
andreaturli commented on this pull request.
> + } else {
+logger.warn(">> security group: (%s) has not been deleted.",
securityGroupId);
+ }
+ }
+
+ return instanceDeleted;
+ }
+
+ private List getSecurityGroupIdsUsedByNode(RegionAndId regionAndId)
{
+ List securityGroupIds = Lists.newArrayList();
+ PaginatedCollection 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)
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)
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)
andreaturli commented on this pull request.
> + private final Supplier> hardwares;
+ private final Supplier> locations;
+ private final Function
toPortableStatus;
+ private final GroupNamingConvention groupNamingConvention;
+ @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger
logger = Logger.NULL;
+
+ @Inject
+ public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,
+ Supplier> images,
+ Supplier>
hardwares,
+ @Memoized Supplier>
locations,
+ Function 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)
andreaturli commented on this pull request.
> + @Override
+ public boolean apply(@Nullable InstanceType input) {
+return contains(instanceTypeIds.build(),
input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List getInstanceTypeId(String regionId) {
+ List 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)
andreaturli commented on this pull request.
> + transform(listLocations(), new Function() {
+ @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 listNodesByIds(final Iterable ids) {
+ return filter(listNodes(), new Predicate() {
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)
andreaturli commented on this pull request.
> + for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List instanceTypes =
FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate() {
+ @Override
+ public boolean apply(@Nullable InstanceType input) {
+return contains(instanceTypeIds.build(),
input.instanceTypeId());
+ }
+ }).toList();
+
+ return instanceTypes;
+ }
+
+ private List 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)
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 firstInterestingImage = Iterables.tryFind(listImages(),
new Predicate() {
+ 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
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request.
> + transform(listLocations(), new Function() {
+ @Override
+ public String apply(Region location) {
+return location.id();
+ }
+ }));
+
+ for (String regionId : availableLocationNames) {
+ instanceTypeIds.addAll(getInstanceTypeId(regionId));
+ }
+
+ List instanceTypes =
FluentIterable.from(api.instanceApi().listTypes())
+ .filter(new Predicate() {
+ @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)
andreaturli commented on this pull request.
> +}
+ }
+ }
+ return instanceTypeIds;
+ }
+
+ @Override
+ public Iterable listImages() {
+ final ImmutableList.Builder images = ImmutableList.builder();
+ final List availableLocationNames = newArrayList(
+ transform(listLocations(), new Function() {
+ @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)
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)
andreaturli commented on this pull request.
> + this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+ this.regionIds = regionIds;
+ this.cleanupResources = cleanupResources;
+ }
+
+ @Override
+ public NodeAndInitialCredentials
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)
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
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 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 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 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
[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
