Support direct upload/download to Image I/O daemon on oVirt node #35

ganto · 2020-05-02T15:41:04Z

Fixes #29 and therefore fixes image down-/upload on oVirt 4.4 engines also see RHBZ 1801710.

I verified this code with the following testing playbook:

---                                                                                                                                                                                                                                                                                                                   
- hosts: localhost
  connection: local                   
  gather_facts: False
                                                                               
  collections:
    - ovirt.ovirt

  tasks:
    - ovirt_auth:
        url: https://ovirt.example.com/ovirt-engine/api
        username: admin@internal
        password: password

    - ovirt_disk:
        auth: '{{ ovirt_auth }}'
        name: testdisk
        size: 10GiB
        format: cow
        upload_image_path: /tmp/testdisk.img
        storage_domain: vmstore

    - ovirt_disk:
        auth: '{{ ovirt_auth }}'
        name: testdisk
        download_image_path: /tmp/testdisk-1.img
        storage_domain: vmstore

    - ovirt_auth:
        ovirt_auth: '{{ ovirt_auth }}'
        state: absent

Before this patch this playbook would fail with the following error when run against a oVirt 4.4 API:

TASK [ovirt_disk] ******************************************************************************************************************************************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ConnectionRefusedError: [Errno 111] Connection refused
fatal: [localhost]: FAILED! => {"changed": false, "msg": "[Errno 111] Connection refused"}

I also needed to remove the authentication header from the download request. Otherwise I would get an error like:

The full traceback is:                                                                                                                                                                                                                                                                                                        
Traceback (most recent call last):                                                                                                                                                                                                                                                                                            
  File "/tmp/ansible_ovirt_disk_payload_wyxbh_tl/ansible_ovirt_disk_payload.zip/ansible_collections/ovirt/ovirt/plugins/modules/ovirt_disk.py", line 753, in main                                                                                                                                                             
  File "/tmp/ansible_ovirt_disk_payload_wyxbh_tl/ansible_ovirt_disk_payload.zip/ansible_collections/ovirt/ovirt/plugins/modules/ovirt_disk.py", line 454, in download_disk_image                                                                                                                                              
  File "/tmp/ansible_ovirt_disk_payload_wyxbh_tl/ansible_ovirt_disk_payload.zip/ansible_collections/ovirt/ovirt/plugins/modules/ovirt_disk.py", line 396, in transfer                                                                                                                                                         
  File "/tmp/ansible_ovirt_disk_payload_wyxbh_tl/ansible_ovirt_disk_payload.zip/ansible_collections/ovirt/ovirt/plugins/modules/ovirt_disk.py", line 435, in _transfer                                                                                                                                                        
  File "/usr/lib/python3.7/http/client.py", line 1262, in request                                                                                                                                                                                                                                                             
    self._send_request(method, url, body, headers, encode_chunked)                                                                                                                                                                                                                                                            
  File "/usr/lib/python3.7/http/client.py", line 1303, in _send_request                                                                                                                                                                                                                                                       
    self.putheader(hdr, value)                                                                                                                                                                                                                                                                                                
  File "/usr/lib/python3.7/http/client.py", line 1239, in putheader                                                                                                                                                                                                                                                           
    if _is_illegal_header_value(values[i]):                                                                                                                                                                                                                                                                                   
TypeError: expected string or bytes-like object

I don't don't understand what is going wrong here. Maybe someone can make some sense of this?

Unfortunately I don't have a oVirt 4.3 environment around for testing the use_proxy flag. ~~Also not sure if this should be enabled or disabled by default. Is there some way to autodetect this and do the right thing for 4.3 and 4.4?~~

ovirt-infra · 2020-05-02T15:41:06Z

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

Type ci test please on this PR to trigger automated tests for it.
Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow ovirt dynamic plugin improvements #3 option )
If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

mnecas · 2020-05-04T06:48:44Z

ci test please

mnecas · 2020-05-04T07:01:40Z

Thank you for the PR!
I'll jump on it and try to test it with 4.3 and 4.4 and review it.
@nirs please also check the PR.

mnecas · 2020-05-04T12:52:44Z

when trying your changes on 4.4 I get

TASK [ovirt_disk] **************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: Exception: Error occurred while uploading image. The transfer is in finalizing_failure
fatal: [10-37-137-37.rhev.lab.eng.brq.redhat.com]: FAILED! => {"changed": false, "msg": "Error occurred while uploading image. The transfer is in finalizing_failure"}

Will do more investigation.

mnecas

Resolved my env issues. And tested it on 4.4 and 4.3.
Another solution is to use the imageio client but that would add another dependency.
LGTM

nirs

Looks like good change in the right direction.

But the upload code should be dropped, it is not efficient and not correct. Not detecting zeroes in uploaded image, sending zeroes over the wire, and always creating preallocated images even when the user asked to create sparse volume.

All these issues will be solved if you will drop the upload code and replace it with imageio client.

Bottom line is that this ansible script should not be used by anyone until it is modified to use imageio client.

plugins/modules/ovirt_disk.py

mnecas · 2020-05-08T15:40:12Z

Thank you for the info, I will create another PR with the imageio client.

mnecas · 2020-05-21T07:39:27Z

ci test please

plugins/modules/ovirt_disk.py

mnecas · 2020-05-21T16:48:50Z

ci test please

nirs

Nice!

plugins/modules/ovirt_disk.py

nirs · 2020-05-21T18:09:15Z

plugins/modules/ovirt_disk.py

@@ -347,6 +347,25 @@ def _search_by_lun(disks_service, lun_id):
    return res[0] if res else None


+def create_transfer_connection(module, transfer, context, connect_timeout=10, read_timeout=60):


It can be useful to expose the timeouts to the user so they can override the timeouts if needed.

yeah thought about that but we have default timeout 3min and not sure if it would be best to wait that long

I would this to future work.

plugins/modules/ovirt_disk.py

nirs · 2020-05-21T18:15:50Z

plugins/modules/ovirt_disk.py

@@ -355,7 +355,8 @@ def create_transfer_connection(module, transfer, context, connect_timeout=10, re
        connection.connect()
    except OSError as e:
        # Typically ConnectionRefusedError or socket.gaierror.
-        module.warn("Cannot connect to %s, trying %s: %s", transfer.transfer_url, transfer.proxy_url, e)
+        module.warn("Cannot connect to {}, trying {}: {}"
+                    .format(transfer.transfer_url, transfer.proxy_url, e))


Formatting messages for logs is bad idea. Formatting is expensive and should be done only when the log message is actually logged. For warnings this is usually the case, but for debug message it is always not and having 2 ways to log in the same code is bad.

I hope this is not the way ansible scripts use logging. If it is I guess we want to keep this consistent with other logs here.

This is not for logging it just shows a user warning message.
And around formatting, I don't mind to use it as it was before.

nirs · 2020-05-21T19:01:47Z

plugins/modules/ovirt_disk.py

@@ -355,8 +355,7 @@ def create_transfer_connection(module, transfer, context, connect_timeout=10, re
        connection.connect()
    except OSError as e:
        # Typically ConnectionRefusedError or socket.gaierror.
-        module.warn("Cannot connect to {}, trying {}: {}"
-                    .format(transfer.transfer_url, transfer.proxy_url, e))
+        module.warn("Cannot connect to %s, trying %s: %s" % (transfer.transfer_url, transfer.proxy_url, e))


This is the same as using {}. Please check module.warn() interface - does it work like Pyhon logging, accepting
fmt, *args or it a single srting?

Then check how other code in this area generate messages and use the same style.

Oh that was your question it only accepts one string no other args.
And it is only message the warning styling is done automatically.

mnecas

Tested on 4.3 and 4.4, merging so we can get it to 2.9 if there is something to change we can fix it later.

Support direct upload/download to Image I/O daemon on oVirt node

f281406

ovirt_disk: auto-detect proxy connection based on engine version

af575d4

mnecas approved these changes May 8, 2020

View reviewed changes

nirs reviewed May 8, 2020

View reviewed changes

plugins/modules/ovirt_disk.py Outdated Show resolved Hide resolved

plugins/modules/ovirt_disk.py Outdated Show resolved Hide resolved

plugins/modules/ovirt_disk.py Outdated Show resolved Hide resolved

mnecas mentioned this pull request May 8, 2020

Use imageio client when uploading disk #39

Closed

Use proxy as fallback

36446c7

nirs suggested changes May 21, 2020

View reviewed changes

plugins/modules/ovirt_disk.py Outdated Show resolved Hide resolved

mnecas added 2 commits May 21, 2020 18:31

add create_transfer_connection

7c90057

correct warn msg

c6da921

nirs reviewed May 21, 2020

View reviewed changes

update transfer names

0fdab53

nirs reviewed May 21, 2020

View reviewed changes

mnecas approved these changes May 25, 2020

View reviewed changes

mnecas merged commit a80de15 into oVirt:master May 25, 2020

mnecas mentioned this pull request May 25, 2020

Backport: Support direct upload/download to Image I/O daemon on oVirt node ansible/ansible#69691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support direct upload/download to Image I/O daemon on oVirt node #35

Support direct upload/download to Image I/O daemon on oVirt node #35

ganto commented May 2, 2020 •

edited

Loading

ovirt-infra commented May 2, 2020

mnecas commented May 4, 2020

mnecas commented May 4, 2020

mnecas commented May 4, 2020

mnecas left a comment

nirs left a comment

mnecas commented May 8, 2020

mnecas commented May 21, 2020

mnecas commented May 21, 2020

nirs left a comment

nirs May 21, 2020

mnecas May 21, 2020

nirs May 21, 2020

nirs May 21, 2020

mnecas May 21, 2020 •

edited

Loading

nirs May 21, 2020

mnecas May 21, 2020

mnecas left a comment

		@@ -347,6 +347,25 @@ def _search_by_lun(disks_service, lun_id):
		return res[0] if res else None


		def create_transfer_connection(module, transfer, context, connect_timeout=10, read_timeout=60):

Support direct upload/download to Image I/O daemon on oVirt node #35

Support direct upload/download to Image I/O daemon on oVirt node #35

Conversation

ganto commented May 2, 2020 • edited Loading

ovirt-infra commented May 2, 2020

mnecas commented May 4, 2020

mnecas commented May 4, 2020

mnecas commented May 4, 2020

mnecas left a comment

Choose a reason for hiding this comment

nirs left a comment

Choose a reason for hiding this comment

mnecas commented May 8, 2020

mnecas commented May 21, 2020

mnecas commented May 21, 2020

nirs left a comment

Choose a reason for hiding this comment

nirs May 21, 2020

Choose a reason for hiding this comment

mnecas May 21, 2020

Choose a reason for hiding this comment

nirs May 21, 2020

Choose a reason for hiding this comment

nirs May 21, 2020

Choose a reason for hiding this comment

mnecas May 21, 2020 • edited Loading

Choose a reason for hiding this comment

nirs May 21, 2020

Choose a reason for hiding this comment

mnecas May 21, 2020

Choose a reason for hiding this comment

mnecas left a comment

Choose a reason for hiding this comment

ganto commented May 2, 2020 •

edited

Loading

mnecas May 21, 2020 •

edited

Loading