jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
Date Thu, 09 Aug 2018 09:27:00 GMT
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
Mime
View raw message