Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ovirt_vm: fix cd_iso get all disks from storage domains #66

Merged
merged 8 commits into from
Jul 28, 2020

Conversation

mnecas
Copy link
Member

@mnecas mnecas commented Jul 22, 2020

Fixes: #62
Issue: in 4.3 the disks from ISO storage domain were not able to get from disks_service to make it backward compatible did need to gather all storage domains
@mwperina @dangel101

Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@mwperina
Copy link
Member

Eyal, could you please also take a look?

@shenitzky

@n0p90
Copy link

n0p90 commented Jul 22, 2020

@mnecas Thanks for the patch.
Unfortunately, this does not work on oVirt 4.3. It crashes with the following stack trace:

Traceback (most recent call last):
  File "/tmp/ansible_ovirt_vm_payload_t_4z7zcm/ansible_ovirt_vm_payload.zip/ansible/modules/cloud/ovirt/ovirt_vm.py", line 2488, in main
  File "/tmp/ansible_ovirt_vm_payload_t_4z7zcm/ansible_ovirt_vm_payload.zip/ansible/modules/cloud/ovirt/ovirt_vm.py", line 1632, in post_present
  File "/tmp/ansible_ovirt_vm_payload_t_4z7zcm/ansible_ovirt_vm_payload.zip/ansible/modules/cloud/ovirt/ovirt_vm.py", line 1712, in _attach_cd
  File "/tmp/ansible_ovirt_vm_payload_t_4z7zcm/ansible_ovirt_vm_payload.zip/ansible/modules/cloud/ovirt/ovirt_vm.py", line 1697, in __get_cd_id
  File "/tmp/ansible_ovirt_vm_payload_t_4z7zcm/ansible_ovirt_vm_payload.zip/ansible/modules/cloud/ovirt/ovirt_vm.py", line 1686, in __get_cds_from_sds
  File "/usr/local/lib64/python3.7/site-packages/ovirt_engine_sdk_python-4.3.3-py3.7-linux-x86_64.egg/ovirtsdk4/__init__.py", line 769, in follow_link
    href = obj.href
AttributeError: 'NoneType' object has no attribute 'href'
  • sd.disks is an empty ovirtsdk4.List for Data domains, despite containing many disks, including ISO
  • sd.disks is None for ISO domains, which causes the crash

@mnecas
Copy link
Member Author

mnecas commented Jul 23, 2020

yeah currently this is work in progress
there are 2 other issues

@mnecas
Copy link
Member Author

mnecas commented Jul 23, 2020

@n0p90 could you please try it on 4.3?
Thanks for your help!

@n0p90
Copy link

n0p90 commented Jul 23, 2020

@mnecas
sd.disks is None for ISO domains. This means that the following exception will always be raised when trying to use an ISO from an ISO storage:

raise ValueError('Was not able to find disk with name or id "{}".'.format(self.param('cd_iso')))

Here is a patch to make it work on oVirt 4.3, I'll let you check if it also works on oVirt 4.4:

diff --git a/plugins/modules/ovirt_vm.py b/plugins/modules/ovirt_vm.py
index 23a939b..8bcee47 100644
--- a/plugins/modules/ovirt_vm.py
+++ b/plugins/modules/ovirt_vm.py
@@ -1662,10 +1662,17 @@ class VmsModule(BaseModule):
 
     def __get_cds_from_sds(self, sds):
         for sd in sds:
-            if sd.disks is None:
+            if sd.type == otypes.StorageDomainType.ISO:
+                disks = sd.files
+                is_iso = lambda x: True
+            elif sd.type == otypes.StorageDomainType.DATA:
+                disks = sd.disks
+                is_iso = lambda x: x.content_type == otypes.DiskContentType.ISO
+            else:
+                # is there any other storage type from which ISO can be loaded?
                 continue
-            disks = list(filter(lambda x: (x.name == self.param('cd_iso') or x.id == self.param('cd_iso')) and x.content_type == otypes.DiskContentType.ISO,
-                                self._connection.follow_link(sd.disks)))
+            disks = list(filter(lambda x: (x.name == self.param('cd_iso') or x.id == self.param('cd_iso')) and is_iso(x),
+                                self._connection.follow_link(disks)))
             if disks:
                 return disks
 

I've never used the ovirt-image-repository which has a type = otypes.StorageDomainType.IMAGE, so I don't know if it's possible to have ISOs in it. If this is the case, then the above patch needs some modifications.

Thanks!

@mnecas
Copy link
Member Author

mnecas commented Jul 24, 2020

Done some investigation and turned up that my env was broken.
There was no change between 4.3 and 4.4 around the ISO domain type.
Will do backport for ansible 2.9
Thanks a lot!

Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@mnecas mnecas merged commit d744492 into oVirt:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ovirt_vm cd_iso by name results in a 404 error
3 participants