ovirt_vms: add ability to specify storage domain #24012

tareqalayan · 2017-04-26T12:55:22Z

When creatinf a new VM from template, you can specify the storage domain
name and disk format where to copy all the template disks

For example if you want to create a VM from template into specific
storage domain you can do the following:

ovirt_vms:
name: vm_on_my_storage_domain
cluster: my_cluster
template: my_template
operating_system: other_linux
type: server
cpu_cores: 1
cpu_sockets: 1
state: stopped
clone: True
storage_domain: my_nfs_storage
format: COW

SUMMARY

ISSUE TYPE

Feature Pull Request
New Module Pull Request
Bugfix Pull Request
Docs Pull Request

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot · 2017-04-26T13:04:08Z

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E309 version_added for new option (disk_format) should be 2.4. Currently 0.0
lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E309 version_added for new option (storage_domain) should be 2.4. Currently 0.0

click here for bot help

ansibot · 2017-04-26T13:04:08Z

cc @machacekondra
click here for bot help

ansibot · 2017-04-26T13:16:29Z

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E309 version_added for new option (disk_format) should be 2.4. Currently 0.0
lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E309 version_added for new option (storage_domain) should be 2.4. Currently 0.0

click here for bot help

gundalow · 2017-04-26T13:41:39Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+    storage_domain:
+        description:
+            - "Name of the storage domain where all template disks should be copied."
+            - "This parameter is considered only when template is provided."


Please add
version_added: "2.4"

gundalow · 2017-04-26T13:41:43Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+    disk_format:
+        description:
+            - "COW or RAW; default: is COW"
+            - "This parameter is considered only when template and storage domain is provided."


Please add
version_added: "2.4"

gundalow · 2017-04-26T13:43:00Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+            - "This parameter is considered only when template is provided."
+    disk_format:
+        description:
+            - "COW or RAW; default: is COW"


Please remove this line and replace with

choices: ['COW, 'RAW'] default: COW

See http://docs.ansible.com/ansible/dev_guide/developing_modules_documenting.html#documentation-block for more info

gundalow · 2017-04-26T13:46:07Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

@@ -1004,6 +1060,7 @@ def main():
        template=dict(default=None),
        template_version=dict(default=None, type='int'),
        use_latest_template_version=dict(default=None, type='bool'),
+        storage_domain=dict(default=None),
        disks=dict(default=[], type='list'),


Missing

disk_format=dict(default='COW', choices='COW, 'RAW'),

This makes me think that changing the disk_format may not have been tested...

tareqalayan · 2017-04-26T14:55:29Z

cc @machacekondra

ansibot · 2017-04-26T15:03:19Z

The test ansible-test sanity --test ansible-doc --python 2.6 failed with the following error:

Command "ansible-doc ovirt_vms" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/ovirt/ovirt_vms.py
ERROR! module ovirt_vms missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 2.7 failed with the following error:

Command "ansible-doc ovirt_vms" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/ovirt/ovirt_vms.py
ERROR! module ovirt_vms missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.5 failed with the following error:

Command "ansible-doc ovirt_vms" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/ovirt/ovirt_vms.py
ERROR! module ovirt_vms missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test ansible-doc --python 3.6 failed with the following error:

Command "ansible-doc ovirt_vms" returned exit status 1.
>>> Standard Error
[ERROR]: unable to parse /root/src/github.com/ansible/ansible/lib/ansible/modu
les/cloud/ovirt/ovirt_vms.py
ERROR! module ovirt_vms missing documentation (or could not parse documentation): Parsing produced an empty object.

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E302 DOCUMENTATION is not valid YAML. Line 77 column 26

click here for bot help

machacekondra · 2017-04-26T15:29:58Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

@@ -66,6 +66,17 @@
            - "Specify if latest template version should be used, when running a stateless VM."
            - "If this parameter is set to I(true) stateless VM is created."
        version_added: "2.3"
+    storage_domain:
+        description:
+            - "Name of the storage domain where all template disks should be copied."


s/copied/created

Please document if it's ideppotent or not.

machacekondra · 2017-04-26T15:30:48Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+        version_added: "2.4"
+    disk_format:
+        description:
+            - "This parameter is considered only when template and storage domain is provided."


please describe what the disk format is

Please also use proper doc formatting like: "C(template) and C(storage_domain) parameters are provided"

Also please say it's not idempotent.

machacekondra · 2017-04-26T15:32:51Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

@@ -541,13 +552,61 @@ def __get_template_with_version(self):

        return template

+    def __get_storage_domain_and_all_template_disks(self, template):
+
+        storage_domain = self._module.params.get('storage_domain')


you can use:

storage_domain = self.param('storage_domain')

machacekondra · 2017-04-26T15:34:16Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

@@ -1004,6 +1063,8 @@ def main():
        template=dict(default=None),
        template_version=dict(default=None, type='int'),
        use_latest_template_version=dict(default=None, type='bool'),
+        storage_domain=dict(default=None),
+        disk_format=dict(choices=['COW','RAW'], default='COW'),


hmm, most of the things in other modules are in lower case, better to be consistent

machacekondra · 2017-04-26T15:36:33Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

    def build_entity(self):
        template = self.__get_template_with_version()
+
+        disks_attachments = None


Can you move this logic to __get_storage_domain_and_all_template_disks and just call:

return otypes.Vm( ... disk_attachments=self.__get_storage_domain_and_all_template_disks() ... )

machacekondra · 2017-04-26T15:37:43Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+                )
+            )
+
+        return storage_domain_obj, disks


not sure why it returns storage_domain_obj?

machacekondra · 2017-04-26T15:38:28Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+                        format=otypes.DiskFormat.COW,
+                        storage_domains=[
+                            otypes.StorageDomain(
+                                id=storage_domain_obj.id


use here just get_id_by_name the code above would not be needed then?

machacekondra · 2017-04-26T15:38:45Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+                otypes.DiskAttachment(
+                    disk=otypes.Disk(
+                        id=disk.id,
+                        format=otypes.DiskFormat.COW,


please use here otypes.DiskFormat(self.param('disk_format'))

machacekondra · 2017-04-26T15:39:36Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+            disks.append(
+                otypes.DiskAttachment(
+                    disk=otypes.Disk(
+                        id=disk.id,


I think it's disk.disk.id

done
btw they both work why?

machacekondra · 2017-04-26T15:40:00Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+        else:
+            format = otypes.DiskFormat.COW
+
+        for disk in disk_attachments:


maybe rename disk to attachment or att to make it more clear?

ansibot · 2017-04-26T17:13:59Z

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/cloud/ovirt/ovirt_vms.py:0:0: E311 EXAMPLES is not valid YAML. Line 497 column 3

click here for bot help

machacekondra

Some minor comments.

machacekondra · 2017-04-27T10:48:33Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+            - "Name of the storage domain where all template disks should be created."
+            - "This parameter is considered only when C(template) is provided."
+            - "C(**IMPORTANT**)"
+            - "This parameter is idempotent, template disks works as expected.


But won't be moved if the storage domain is changed, no? The note should be added only if the parameter is NOT idempotent. The default is meant to be idempotent.

machacekondra · 2017-04-27T10:51:45Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+        version_added: "2.4"
+    disk_format:
+        description:
+            - "File format for disk image files: I(cow) stands for copy on write,


I think you can copy paste what's already in ovirt_disks module:

- Specify format of the disk. - If (cow) format is used, disk will by created as sparse, so space will be allocated for the volume as needed, also known as I(thin provision). - If (raw) format is used, disk storage will be allocated right away, also known as I(preallocated). - Note that this option isn't idempotent as it's not currently possible to change format of the disk via API.

machacekondra · 2017-04-27T12:02:21Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+  cpu_sockets: 1
+  state: stopped
+  clone: True
+  timeout: 600


Maybe I would remove the parameters which are not relevant to the example, to make it more clear?

machacekondra · 2017-04-27T12:07:08Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

@@ -541,13 +574,52 @@ def __get_template_with_version(self):

        return template

+    def __get_storage_domain_and_all_template_disks(self, template):
+
+        if self._module.params.get('template') is None:


if self._module.params.get('template') is None:
->
if self.param('template') is None:

machacekondra · 2017-04-27T12:08:06Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+        if self.param('storage_domain') is None:
+            return None
+
+        storage_domain = self.param('storage_domain')


I think it's not worth to create the temp var here, maybe just pass the self.param('storage_domain')
directly into the get_id_by_name ?

machacekondra · 2017-04-27T12:10:03Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

+
+        storage_domain = self.param('storage_domain')
+
+        disk_attachments = (


I think it's not worth to create the temp var here, maybe just pass the self._connection.follow_link(template.disk_attachments) directly in for, also it's not strict to use 80 chars/line and I think here the newline makes it less readable, so if you want to use temp var then I think it would be more readeble, to not use new line, but up to you.

done moved to be used directly in the for loop

machacekondra · 2017-04-27T12:10:39Z

lib/ansible/modules/cloud/ovirt/ovirt_vms.py

        return otypes.Vm(
            name=self.param('name'),
            cluster=otypes.Cluster(
                name=self.param('cluster')
            ) if self.param('cluster') else None,
+            disk_attachments=disk_attachments if disk_attachments else None,


I think the if isn't needed here anymore as it's the job of __get_storage_domain_and_all_template_disks to handle it, no?

When creatinf a new VM from template, you can specify the storage domain name and disk format where to copy all the template disks For example if you want to create a VM from template into specific storage domain you can do the following: ovirt_vms: name: vm_on_my_storage_domain cluster: my_cluster template: my_template operating_system: other_linux type: server cpu_cores: 1 cpu_sockets: 1 state: stopped clone: True storage_domain: my_nfs_storage format: COW

machacekondra

shipit

myakove

shipit

mwperina

👍

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from 3e53401 to 542c5e8 Compare April 26, 2017 13:06

gundalow requested changes Apr 26, 2017

View reviewed changes

gundalow removed the needs_triage Needs a first human triage before being processed. label Apr 26, 2017

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from 542c5e8 to 31c6b1f Compare April 26, 2017 14:54

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 26, 2017

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from 31c6b1f to 0f8b24c Compare April 26, 2017 15:03

machacekondra suggested changes Apr 26, 2017

View reviewed changes

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from 0f8b24c to 8575add Compare April 26, 2017 16:47

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 26, 2017

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from 8575add to eafb648 Compare April 26, 2017 17:31

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 26, 2017

machacekondra reviewed Apr 27, 2017

View reviewed changes

tareqalayan force-pushed the vm-from-template-on-specific-sd branch from eafb648 to 4410be9 Compare April 27, 2017 14:08

machacekondra approved these changes Apr 28, 2017

View reviewed changes

gundalow approved these changes Apr 28, 2017

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 28, 2017

myakove approved these changes May 11, 2017

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 11, 2017

mwperina approved these changes Jun 8, 2017

View reviewed changes

ryansb merged commit 013f759 into ansible:devel Jun 8, 2017

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018

dagwieers added the ovirt oVirt and RHV community label Feb 20, 2019

dagwieers removed virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) labels Feb 28, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019


		storage_domain = self.param('storage_domain')

		disk_attachments = (

ovirt_vms: add ability to specify storage domain #24012

ovirt_vms: add ability to specify storage domain #24012

Conversation

tareqalayan commented Apr 26, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Apr 26, 2017

ansibot commented Apr 26, 2017

ansibot commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tareqalayan commented Apr 26, 2017

ansibot commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Apr 26, 2017

machacekondra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machacekondra left a comment

Choose a reason for hiding this comment

myakove left a comment

Choose a reason for hiding this comment

mwperina left a comment

Choose a reason for hiding this comment