Kami commented on code in PR #1983:
URL: https://github.com/apache/libcloud/pull/1983#discussion_r1581833632


##########
libcloud/compute/drivers/kubevirt.py:
##########
@@ -179,125 +245,103 @@ def destroy_node(self, node):
         except Exception:
             raise
 
-    # only has container disk support atm with no persistency
-    def create_node(
-        self,
-        name,
-        image,
-        location=None,
-        ex_memory=128,
-        ex_cpu=1,
-        ex_disks=None,
-        ex_network=None,
-        ex_termination_grace_period=0,
-        ports=None,
-    ):
+    def _create_node_with_template(self, name: str, template: dict, 
namespace="default"):
         """
-        Creating a VM with a containerDisk.
-        :param name: A name to give the VM. The VM will be identified by
-                     this name and atm it cannot be changed after it is set.
+        Creating a VM defined by the template.
+
+        The template must be a dictionary of kubernetes object that defines the
+        KubeVirt VM. Following are the keys:
+
+        - ``apiVersion``: ``str``
+        - ``kind``: ``str``
+        - ``metadata``: ``dict``
+        - ``spec``: ``dict``
+            - ``domain``: ``dict``
+            - ``volumes``: ``list``
+            - ...
+        - ...
+
+        See also:
+
+        - https://kubernetes.io/docs/concepts/overview/working-with-objects/
+        - https://kubevirt.io/api-reference/
+
+        :param name: A name to give the VM. The VM will be identified by this
+                     name, and it must be the same as the name in the
+                     ``template["metadata"]["name"]``.
+                     Atm, it cannot be changed after it is set.
         :type name: ``str``
 
-        :param image: Either a libcloud NodeImage or a string.
-                      In both cases it must point to a Docker image with an
-                      embedded disk.
-                      May be a URI like `kubevirt/cirros-registry-disk-demo`,
-                      kubevirt will automatically pull it from
-                      https://hub.docker.com/u/URI.
-                      For more info visit:
-                      
https://kubevirt.io/user-guide/docs/latest/creating-virtual-machines/disks-and-volumes.html#containerdisk
-        :type image: `str`
+        :param template: A dictionary of kubernetes object that defines the VM.
+        :type template: ``dict`` with keys:
+                        ``apiVersion: str``, ``kind: str``, ``metadata: dict``,
+                        ``spec: dict`` etc.
 
-        :param location: The namespace where the VM will live.
+        :param namespace: The namespace where the VM will live.
                           (default is 'default')
-        :type location: ``str``
+        :return:
+        """
+        # k8s object checks
+        if template.get("apiVersion", "") != "kubevirt.io/v1alpha3":
+            raise ValueError("The template must have an apiVersion: 
kubevirt.io/v1alpha3")
+        if template.get("kind", "") != "VirtualMachine":
+            raise ValueError("The template must contain kind: VirtualMachine")
+        if name != template.get("metadata", {}).get("name"):
+            raise ValueError(
+                "The name of the VM must be the same as the name in the 
template. "
+                "(name={}, template.metadata.name={})".format(
+                    name, template.get("metadata", {}).get("name")
+                )
+            )
+        if template.get("spec", {}).get("running", False):
+            warnings.warn(
+                "The VM will be created in a stopped state, and then started. "
+                "Ignoring the `spec.running: True` in the template."
+            )
+            # assert "spec" in template and "running" in template["spec"]
+            template["spec"]["running"] = False
 
-        :param ex_memory: The RAM in MB to be allocated to the VM
-        :type ex_memory: ``int``
+        vm = template
 
-        :param ex_cpu: The amount of cpu to be allocated in miliCPUs
-                    ie: 400 will mean 0.4 of a core, 1000 will mean 1 core
-                    and 3000 will mean 3 cores.
-        :type ex_cpu: ``int``
+        method = "POST"
+        data = json.dumps(vm)
+        req = KUBEVIRT_URL + "namespaces/" + namespace + "/virtualmachines/"
+        try:
+            self.connection.request(req, method=method, data=data)
+        except Exception:
+            raise
 
-        :param ex_disks: A list containing disk dictionaries.
-                             Each dictionaries should have the
-                             following optional keys:
-                             -bus: can be "virtio", "sata", or "scsi"
-                             -device: can be "lun" or "disk"
-                             The following are required keys:
-                             -disk_type: atm only "persistentVolumeClaim"
-                                         is supported
-                             -name: The name of the disk configuration
-                             -claim_name: the name of the
-                                         Persistent Volume Claim
-
-                            If you wish a new Persistent Volume Claim can be
-                            created by providing the following:
-                            required:
-                            -size: the desired size (implied in GB)
-                            -storage_class_name: the name of the storage class 
to # NOQA
-                                               be used for the creation of the
-                                               Persistent Volume Claim.
-                                               Make sure it allows for
-                                               dymamic provisioning.
-                             optional:
-                            -access_mode: default is ReadWriteOnce
-                            -volume_mode: default is `Filesystem`,
-                                         it can also be `Block`
-
-        :type ex_disks: `list` of `dict`. For each `dict` the types
-                            for its keys are:
-                            -bus: `str`
-                            -device: `str`
-                            -disk_type: `str`
-                            -name: `str`
-                            -claim_name: `str`
-                            (for creating a claim:)
-                            -size: `int`
-                            -storage_class_name: `str`
-                            -volume_mode: `str`
-                            -access_mode: `str`
-
-        :param ex_network: Only the pod type is supported, and in the
-                               configuration masquerade or bridge are the
-                               accepted values.
-                               The parameter must be a tuple or list with
-                               (network_type, interface, name)
-        :type ex_network: `iterable` (tuple or list) [network_type, interface, 
name]
-                      network_type: `str` | only "pod" is accepted atm
-                      interface: `str` | "masquerade" or "bridge"
-                      name: `str`
-
-        :param ports: A dictionary with keys: 'ports_tcp' and 'ports_udp'
-                      'ports_tcp' value is a list of ints that indicate
-                      the ports to be exposed with TCP protocol,
-                      and 'ports_udp' is a list of ints that indicate
-                      the ports to be exposed with UDP protocol.
-        :type  ports: `dict` with keys
-                      'ports_tcp`: `list` of `int`
-                      'ports_udp`: `list` of `int`
+        # check if new node is present
+        # But why not just use the resp from the POST request?
+        # Or self.get_node()?
+        # I don't think a for loop over list_nodes is necessary.

Review Comment:
   Yeah, I agree, either using `list_nodes()` or `get_node()` which calls 
`list_nodes()` underneath is not really great and efficient...



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@libcloud.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to