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
