Repository: ansible/ansible
Pull request: ovirt vm clone from snapshot
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
ovirt vm clone from snapshot
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
Fixes #55382 
##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
ovirt
##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below

```


Problem statement:
Extend ovirt_vm module to be able to clone VM from snapshot
##### SUMMARY
Extend ovirt_vm module to be able to clone VM from existing snapshot of another VM.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
ovirt_vm module

##### ADDITIONAL INFORMATION
There is no ability to clone VM's snapshot in any of existing ovirt modules. That is why I propose to modify existing ovirt_vm module to add such a function. I believe this function will be useful for others.
Two new parameters need to be added:
- snapshot: Description of (existing) snapshot to clone;
- srcvm: Name of VM with this snapshot.

Usage:
```
# Clone VM from snapshot
- name: Clone VM from snapshot
  ovirt_vm:
    srcvm: myvm
    snapshot: myvm_snap
    name: myvm_clone
    state: present
```
Patch for ansible 2.5:
```
--- /usr/lib/python2.7/site-packages/ansible/modules/cloud/ovirt/ovirt_vm.py    2019-03-14 22:03:03.000000000 +0300
+++ modules/ovirt_vm.py    2019-04-16 12:26:36.020684314 +0300
@@ -552,6 +552,16 @@
             - "C(headless_mode) - If I(true) disable the graphics console for this virtual machine."
             - "C(protocol) - Graphical protocol, a list of I(spice), I(vnc), or both."
         version_added: "2.5"
+    snapshot:
+        description:
+            - "Snapshot to clone VM from."
+            - "Snapshot with description specified should exist."
+            - "VM having this snapshot should be specified by C(srcvm)."
+    srcvm:
+        description:
+            - "Source VM to clone VM from."
+            - "VM should have snapshot specified by C(snapshot)."
+            - "If C(snapshot) specified C(srcvm) is required."
 notes:
     - If VM is in I(UNASSIGNED) or I(UNKNOWN) state before any operation, the module will fail.
       If VM is in I(IMAGE_LOCKED) state before any operation, we try to wait for VM to be I(DOWN).
@@ -888,6 +898,14 @@
       - name: pci_0000_00_08_0
         state: present
 
+# Clone VM from snapshot
+- name: Clone VM from snapshot
+  ovirt_vm:
+    srcvm: myvm
+    snapshot: myvm_snap
+    name: myvm_clone
+    state: present
+
 '''
 
 
@@ -1000,10 +1018,41 @@
 
         return disks
 
+    def __get_snapshot(self):
+        if self.param('srcvm') is None:
+            return None
+
+        if self.param('snapshot') is None:
+            return None
+
+        vms_service = self._connection.system_service().vms_service()
+        vm = search_by_name(vms_service, self.param('srcvm'))
+        if not vm:
+            raise ValueError(
+                "VM with name '%s' was not found'" % (
+                    self.param('srcvm'),
+                )
+            )
+
+        vm_service = vms_service.vm_service(vm.id)
+        snaps_service = vm_service.snapshots_service()
+        snaps = snaps_service.list()
+        snap = next(
+            (s for s in snaps if s.description == self.param('snapshot')),
+            None
+        )
+
+        cluster = self._connection.system_service().clusters_service().cluster_service(vm.cluster.id).get()
+
+        self._module.params['cluster'] = cluster.name
+
+        return snap
+
     def build_entity(self):
         template = self.__get_template_with_version()
 
         disk_attachments = self.__get_storage_domain_and_all_template_disks(template)
+        snapshot = self.__get_snapshot()
 
         return otypes.Vm(
             id=self.param('id'),
@@ -1143,6 +1192,7 @@
                 ) for cp in self.param('custom_properties') if cp
             ] if self.param('custom_properties') is not None else None,
             initialization=self.get_initialization() if self.param('cloud_init_persist') else None,
+            snapshots=[otypes.Snapshot(id=snapshot.id)] if snapshot else None,
         )
 
     def update_check(self, entity):
@@ -1985,6 +2035,8 @@
         watchdog=dict(type='dict'),
         host_devices=dict(type='list'),
         graphical_console=dict(type='dict'),
+        snapshot=dict(type='str'),
+        srcvm=dict(type='str'),
     )
     module = AnsibleModule(
         argument_spec=argument_spec,
```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/cloud/ovirt/ovirt_vm.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/ovirt/ovirt_vm.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @machacekondra @mnecas @mwperina
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: ovirt snapshot update cluster

Patch to review:
diff --git a/lib/ansible/modules/cloud/ovirt/ovirt_vm.py b/lib/ansible/modules/cloud/ovirt/ovirt_vm.py
index 247051b50dae90..52edaf06067311 100644
--- a/lib/ansible/modules/cloud/ovirt/ovirt_vm.py
+++ b/lib/ansible/modules/cloud/ovirt/ovirt_vm.py
@@ -784,6 +784,18 @@
             - NOTE - If there are multiple next run configuration changes on the VM, the first change may get reverted if this option is not passed.
         version_added: "2.8"
         type: bool
+    snapshot:
+        description:
+            - "Snapshot to clone VM from."
+            - "Snapshot with description specified should exist."
+            - "VM having this snapshot should be specified by C(srcvm)."
+        version_added: "2.9"
+    srcvm:
+        description:
+            - "Source VM to clone VM from."
+            - "VM should have snapshot specified by C(snapshot)."
+            - "If C(snapshot) specified C(srcvm) is required."
+        version_added: "2.9"
 
 notes:
     - If VM is in I(UNASSIGNED) or I(UNKNOWN) state before any operation, the module will fail.
@@ -1141,6 +1153,13 @@
         host: myhost
         filename: myvm.ova
         directory: /tmp/
+
+- name: Clone VM from snapshot
+  ovirt_vm:
+    srcvm: myvm
+    snapshot: myvm_snap
+    name: myvm_clone
+    state: present
 '''
 
 
@@ -1254,8 +1273,43 @@ def __get_storage_domain_and_all_template_disks(self, template):
 
         return disks
 
+    def __get_snapshot(self):
+
+        if self.param('srcvm') is None:
+            return None
+
+        if self.param('snapshot') is None:
+            return None
+
+        vms_service = self._connection.system_service().vms_service()
+        vm = search_by_name(vms_service, self.param('srcvm'))
+        if not vm:
+            raise ValueError(
+                "VM with name '%s' was not found'" % (
+                    self.param('srcvm'),
+                )
+            )
+
+        vm_service = vms_service.vm_service(vm.id)
+        snaps_service = vm_service.snapshots_service()
+        snaps = snaps_service.list()
+        snap = next(
+            (s for s in snaps if s.description == self.param('snapshot')),
+            None
+        )
+
+        return snap
+
     def build_entity(self):
         template = self.__get_template_with_version()
+        cluster = self.param('cluster')
+
+        if self.param('cluster') is None and self.param('snapshot') is not None and self.param('srcvm') is not None:
+            vms_service = self._connection.system_service().vms_service()
+            vm = search_by_name(vms_service, self.param('srcvm'))
+            cluster = self._connection.system_service().clusters_service().cluster_service(vm.cluster.id).get().name
+
+        snapshot = self.__get_snapshot()
 
         disk_attachments = self.__get_storage_domain_and_all_template_disks(template)
 
@@ -1263,8 +1317,8 @@ def build_entity(self):
             id=self.param('id'),
             name=self.param('name'),
             cluster=otypes.Cluster(
-                name=self.param('cluster')
-            ) if self.param('cluster') else None,
+                name=cluster
+            ) if cluster else None,
             disk_attachments=disk_attachments,
             template=otypes.Template(
                 id=template.id,
@@ -1400,6 +1454,7 @@ def build_entity(self):
                 ) for cp in self.param('custom_properties') if cp
             ] if self.param('custom_properties') is not None else None,
             initialization=self.get_initialization() if self.param('cloud_init_persist') else None,
+            snapshots=[otypes.Snapshot(id=snapshot.id)] if snapshot is not None else None,
         )
 
     def _get_export_domain_service(self):
@@ -2264,6 +2319,8 @@ def main():
         force_migrate=dict(type='bool'),
         migrate=dict(type='bool', default=None),
         next_run=dict(type='bool'),
+        snapshot=dict(type='str'),
+        srcvm=dict(type='str'),
     )
     module = AnsibleModule(
         argument_spec=argument_spec,


Code review comments:
diff_hunk: @@ -1254,17 +1273,52 @@ def __get_storage_domain_and_all_template_disks(self, template):
 
         return disks
 
+    def __get_snapshot(self):
+
+        if self.param('srcvm') is None:
+            return None
+
+        if self.param('snapshot') is None:
+            return None
+
+        vms_service = self._connection.system_service().vms_service()
+        vm = search_by_name(vms_service, self.param('srcvm'))
+        if not vm:
+            raise ValueError(
+                "VM with name '%s' was not found'" % (
+                    self.param('srcvm'),
+                )
+            )
+
+        vm_service = vms_service.vm_service(vm.id)
+        snaps_service = vm_service.snapshots_service()
+        snaps = snaps_service.list()
+        snap = next(
+            (s for s in snaps if s.description == self.param('snapshot')),
+            None
+        )
+
+        return snap
+
     def build_entity(self):
         template = self.__get_template_with_version()
+        cluster = self.param('cluster')
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
Can you please create method `__get_cluster()` and add there the logic on line 1307 - 1310

diff_hunk: @@ -2264,6 +2319,8 @@ def main():
         force_migrate=dict(type='bool'),
         migrate=dict(type='bool', default=None),
         next_run=dict(type='bool'),
+        snapshot=dict(type='str'),
+        srcvm=dict(type='str'),
     )
     module = AnsibleModule(
         argument_spec=argument_spec,
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
Please add after `required_if` folllowing:

```
required_together=[
  [ 'snapshot_name', 'snapshot_vm' ]
]
```

diff_hunk: @@ -1254,17 +1273,52 @@ def __get_storage_domain_and_all_template_disks(self, template):
 
         return disks
 
+    def __get_snapshot(self):
+
+        if self.param('srcvm') is None:
+            return None
+
+        if self.param('snapshot') is None:
+            return None
+
+        vms_service = self._connection.system_service().vms_service()
+        vm = search_by_name(vms_service, self.param('srcvm'))
+        if not vm:
+            raise ValueError(
+                "VM with name '%s' was not found'" % (
+                    self.param('srcvm'),
+                )
+            )
+
+        vm_service = vms_service.vm_service(vm.id)
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
You can replace line 1284-1293 by:

```python
vms_service = self._connection.system_service().vms_service()
vm_id = get_id_by_name(vms_service, self.param('srcvm'))
vm_service = vms_service.vm_service(vm_id)
```

diff_hunk: @@ -784,6 +784,18 @@
             - NOTE - If there are multiple next run configuration changes on the VM, the first change may get reverted if this option is not passed.
         version_added: "2.8"
         type: bool
+    snapshot:
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
Please rename this to `snapshot_name`.

diff_hunk: @@ -784,6 +784,18 @@
             - NOTE - If there are multiple next run configuration changes on the VM, the first change may get reverted if this option is not passed.
         version_added: "2.8"
         type: bool
+    snapshot:
+        description:
+            - "Snapshot to clone VM from."
+            - "Snapshot with description specified should exist."
+            - "VM having this snapshot should be specified by C(srcvm)."
+        version_added: "2.9"
+    srcvm:
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
Please rename this to `snapshot_vm`

diff_hunk: @@ -784,6 +784,18 @@
             - NOTE - If there are multiple next run configuration changes on the VM, the first change may get reverted if this option is not passed.
         version_added: "2.8"
         type: bool
+    snapshot:
+        description:
+            - "Snapshot to clone VM from."
+            - "Snapshot with description specified should exist."
+            - "VM having this snapshot should be specified by C(srcvm)."
path: lib/ansible/modules/cloud/ovirt/ovirt_vm.py
text: @user1:
Please change to:
`You have to specify C(snapshot_vm) parameter with virtual machine name of this snapshot.`