jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrea Turli <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1386 1&1 Baremetal servers (#431)
Date Tue, 03 Apr 2018 15:53:38 GMT
andreaturli requested changes on this pull request.

Hi @alibazlamit thanks for the effort!

There are still some comments unaddressed but more importantly looks like bare metal and VM
have a too different work path so worth considering 2 different jclouds APIs, one for 1&1
VM and one for 1&1 bare metal. Code path would be easier to follow maybe?

This seems an idea worth investigating especially if image formats for VMs and for bare metals
have a completely different syntax/semantic (ie: imageId structurally different and so on)

--- 
Finally the liveTests results are extremely important so we need to figure out where this
`web` user is coming from and fix it

---

Thanks for your help!

>     }
 
    @Override
    public NodeAndInitialCredentials<Server> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
 
-      final String dataCenterId = template.getLocation().getId();
+      final String dataCenterId ="908DC2072407C94C8054610AD5A53B8C";// template.getLocation().getId();

why this hardcoded value?

>  
    @Inject
    OneandoneComputeServiceAdapter(OneAndOneApi api, CleanupResources cleanupResources,
+           GenerateHardwareRequest generateHardwareRequest,
            @Named(POLL_PREDICATE_SERVER) Predicate<Server> waitServerUntilAvailable,
            PasswordGenerator.Config passwordGenerator) {

as you have removed it from the constructor, why do you still need to inject it?

>     }
 
    @Override
    public NodeAndInitialCredentials<Server> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
 
-      final String dataCenterId = template.getLocation().getId();
+      final String dataCenterId ="908DC2072407C94C8054610AD5A53B8C";// template.getLocation().getId();
       Hardware hardware = template.getHardware();
       TemplateOptions options = template.getOptions();
       Server updateServer = null;

null not needed, maybe remove it?

> -            float minHddSize = volume.getSize();
-            if (volume.isBootDevice()) {
-               SingleServerAppliance appliance = api.serverApplianceApi().get(image.getId());
-               if (appliance.minHddSize() > volume.getSize()) {
-                  minHddSize = appliance.minHddSize();
-               }
-            }
-            Hdd.CreateHdd hdd = Hdd.CreateHdd.create(minHddSize, volume.isBootDevice());
-            hdds.add(hdd);
-         } catch (Exception ex) {
-            throw Throwables.propagate(ex);
-
+      String imageId = image.getId();
+      Hardware hardwareModel = generateHardwareRequest.isFlavor(hardware.getId());
+      boolean isBaremetal = hardware.getName() == null ? false : hardware.getName().contains(baremetalModelsKey);
+      ServerAppliance workingImage = null;

null not needed, maybe remove it?

> -         ram = ram / 1024;
+      //configuring Firewall rules
+      Map<Integer, Integer> portsRange = getPortRangesFromList(inboundPorts);
+      List<FirewallPolicy.Rule.CreatePayload> rules = new ArrayList<FirewallPolicy.Rule.CreatePayload>();
+
+      for (Map.Entry<Integer, Integer> range : portsRange.entrySet()) {
+         FirewallPolicy.Rule.CreatePayload rule = FirewallPolicy.Rule.CreatePayload.builder()
+                 .portFrom(range.getKey())
+                 .portTo(range.getValue())
+                 .protocol(Types.RuleProtocol.TCP)
+                 .build();
+         rules.add(rule);
+      }
+      String firewallPolicyId = "";
+      if (inboundPorts.length > 0) {
+         FirewallPolicy firewallPolicy = api.firewallPolicyApi().create(FirewallPolicy.CreateFirewallPolicy.create(name
+ " firewall policy", "desc", rules));

what happens when you ask to create multiple nodes? does `firewallPolicyApi` handles the concurrency
correctly?

jclouds usually takes care of fw rules for a group of node extending `CreateNodesWithGroupEncodedIntoNameThenAddToSet`
-- see `ApplyNovaTemplateOptionsCreateNodesWithGroupEncodedIntoNameThenAddToSet` in openstack-nova
api for an example

> -               SingleServerAppliance appliance = api.serverApplianceApi().get(image.getId());
-               if (appliance.minHddSize() > volume.getSize()) {
-                  minHddSize = appliance.minHddSize();
-               }
-            }
-            Hdd.CreateHdd hdd = Hdd.CreateHdd.create(minHddSize, volume.isBootDevice());
-            hdds.add(hdd);
-         } catch (Exception ex) {
-            throw Throwables.propagate(ex);
-
+      String imageId = image.getId();
+      Hardware hardwareModel = generateHardwareRequest.isFlavor(hardware.getId());
+      boolean isBaremetal = hardware.getName() == null ? false : hardware.getName().contains(baremetalModelsKey);
+      ServerAppliance workingImage = null;
+
+      //choose the correct image based on the server type baremetal or cloud

sorry can't understand this.

Can you elaborate a bit more the rationale of this `findWorkingImage` method?

>           }
       }
 
-      // provision server
-      Server server = null;
-      Double cores = ComputeServiceUtils.getCores(hardware);
-      Double ram = (double) hardware.getRam();
-      if (ram < 1024) {
-         ram = 0.5;
-      } else {
-         ram = ram / 1024;
+      //configuring Firewall rules
+      Map<Integer, Integer> portsRange = getPortRangesFromList(inboundPorts);

Is this change strictly related to bare metal support? seems to me that this PR is trying
to change too many things, would it be possible for you to split up into multiple smaller
PR so to have a better/easier review?

-- 
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/431#pullrequestreview-109002015
Mime
View raw message