<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Mar 22, 2018 at 5:25 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">PROBLEMS:<br>
- -of qcow2 does not work, with multiple problems<br>
* needs to set NBD size to something larger than virtual size<br></blockquote><div><br></div><div>How is this related to the actual file size specified when you create a disk?</div><div><br></div><div>In block storage, qcow2 image is always on a thin provisioned disk, which</div><div>in oVirt is a regular logical volume, created at the requested "initial_size":</div><div><br></div><div>From:</div><div><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><a href="https://github.com/oVirt/ovirt-engine-sdk/blob/aba2a83ec94ecac1594adfff62d3cfcfdbdef416/sdk/examples/upload_disk.py#L113">https://github.com/oVirt/ovirt-engine-sdk/blob/aba2a83ec94ecac1594adfff62d3cfcfdbdef416/sdk/examples/upload_disk.py#L113</a></pre></div><div><br></div><div><pre style="word-wrap:break-word;white-space:pre-wrap">if image_info["format"] == "qcow2":
disk_format = types.DiskFormat.COW
else:
disk_format = types.DiskFormat.RAW
disks_service = connection.system_service().disks_service()
disk = disks_service.add(
disk=types.Disk(
name=os.path.basename(image_path),
description='Uploaded disk',
format=disk_format,
initial_size=image_size,
provisioned_size=image_info["virtual-size"],
sparse=disk_format == types.DiskFormat.COW,
storage_domains=[
types.StorageDomain(
name='mydata'
)
]
)
)
</pre>Internally we do allocated 10% more then the requested initial_size</div><div>to leave room for qcow2 metadata.</div><div><br></div><div>See:</div><div><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><a href="https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/blockVolume.py#L328">https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/blockVolume.py#L328</a></pre><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><br></pre><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><br></pre></div><div><br></div><div><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><a href="https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/volume.py#L666">https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/volume.py#L666</a></pre><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><br></pre><pre style="box-sizing:border-box;font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;margin-top:0px;margin-bottom:0px;color:rgb(36,41,46);width:1px;height:1px"><br></pre></div><div><br></div><div><br></div><div>Do we have other problems?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Cannot choose the datacenter.<br></blockquote><div><br></div><div>The storage domain belongs to some data center, so I don't think you</div><div>need to select a data center.</div><div><br></div><div>[snipped]</div><div><br></div><div> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
new file mode 100644<br>
index 000000000..979cbd63b<br>
--- /dev/null<br>
+++ b/v2v/rhv-upload-plugin.py<br>
@@ -0,0 +1,414 @@<br>
+# -*- python -*-<br>
+# oVirt or RHV upload nbdkit plugin used by ‘virt-v2v -o rhv-upload’<br>
+# Copyright (C) 2018 Red Hat Inc.<br>
+#<br>
+# This program is free software; you can redistribute it and/or modify<br>
+# it under the terms of the GNU General Public License as published by<br>
+# the Free Software Foundation; either version 2 of the License, or<br>
+# (at your option) any later version.<br>
+#<br>
+# This program is distributed in the hope that it will be useful,<br>
+# but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
+# GNU General Public License for more details.<br>
+#<br>
+# You should have received a copy of the GNU General Public License along<br>
+# <a href="https://maps.google.com/?q=with+this+program&entry=gmail&source=g">with this program</a>; if not, write to the Free Software Foundation, Inc.,<br>
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.<br>
+<br>
+import builtins<br>
+import json<br>
+import logging<br>
+import ovirtsdk4 as sdk<br>
+import ovirtsdk4.types as types<br></blockquote><div><br></div><div>It is good practice to separate stdlib imports and 3rd party imports</div><div>like ovirtsdk, helps to understand the dependencies in modules.</div><div> </div><div>[snipped] </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+from http.client import HTTPSConnection<br>
+from urllib.parse import urlparse<br></blockquote><div><br></div><div>These will not work in python 2, but you can use six.moves to have</div><div>code that works on both 2 and 3.</div><div> </div><div>[snipped] <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+ # Connect to the server.<br>
+ connection = sdk.Connection(<br>
+ url = params['output_conn'],<br>
+ username = username,<br>
+ password = password,<br>
+ ca_file = params['rhv_cafile'], <br></blockquote><div><br></div><div>Can this be None?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ log = logging.getLogger(),<br>
+ insecure = params['insecure'],<br></blockquote><div><br></div><div>If ca_file cannot be None, then insecure is not needed, based</div><div>on Ondra review from earlier version.</div><div> </div><div>[snipped] <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+ # Create the disk.<br>
+ disks_service = system_service.disks_service()<br>
+ if params['disk_format'] == "raw":<br>
+ disk_format = types.DiskFormat.RAW<br>
+ else:<br>
+ disk_format = types.DiskFormat.COW<br>
+ disk = disks_service.add(<br>
+ disk = types.Disk(<br>
+ name = params['disk_name'],<br>
+ description = "Uploaded by virt-v2v",<br>
+ format = disk_format,<br>
+ provisioned_size = params['disk_size'],<br></blockquote><div><br></div><div>This must be the virtual size.</div><div><br></div><div>You don't specify initial_size - in this case you get 1G, and most</div><div>images will fail to upload.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ sparse = params['output_sparse'],<br></blockquote><div><br></div><div>The user cannot configure that. This must be based on the image</div><div>format. The current coded may create images in unsupported </div><div>combinations, e.g. raw/sparse on block storage, or fail validation</div><div>in engine.</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+# Can we issue zero, trim or flush requests?<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+def get_options(h):<br>
+ if h['got_options']:<br>
+ return<br>
+ h['got_options'] = True<br>
+<br>
+ http = h['http']<br>
+ transfer=h['transfer']<br>
+<br>
+ http.putrequest("OPTIONS", h['path'])<br>
+ http.putheader("Authorization", transfer.signed_ticket)<br>
+ http.endheaders()<br>
+<br>
+ r = http.getresponse()<br>
+ if r.status == 200:<br>
+ j = json.loads(r.read())<br>
+ h['can_zero'] = "zero" in j['features']<br>
+ h['can_trim'] = "trim" in j['features']<br>
+ h['can_flush'] = "flush" in j['features']<br>
+<br>
+ # If can_zero is true then it means we're using the new imageio<br>
+ # which never needs the Authorization header.<br>
+ if h['can_zero']:<br>
+ h['needs_auth'] = False<br></blockquote><div><br></div><div>If we got here, we are working with new daemon or proxy, and both</div><div>of them do not need auth, so we can set 'needs_auth' to False </div><div>if OPTIONS returned 200 OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ # Old imageio servers returned 405 Method Not Allowed. If<br>
+ # we see that we just say that no operations are allowed and<br>
+ # we will emulate them.<br>
+ elif r.status == 405:<br>
+ h['can_zero'] = False<br>
+ h['can_trim'] = False<br>
+ h['can_flush'] = False<br></blockquote><div><br></div><div>I would set all the defaults when creating the sate dict. This ensures </div><div>that we don't forget needs_auth or other keys and crash later with</div><div>KeyError, and make it easier to understand what is the state managed</div><div>by the plugin.</div><div> </div><div>[snipped]</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+def pwrite(h, buf, offset):<br>
+ http = h['http']<br>
+ transfer=h['transfer']<br>
+ transfer_service=h['transfer_service']<br>
+<br>
+ count = len(buf)<br>
+ h['highestwrite'] = max(h['highestwrite'], offset+count)<br>
+<br>
+ http.putrequest("PUT", h['path'] + "?flush=n")<br></blockquote><div><br></div><div>"?flush=n" has effect only if h["can_flush"] is True. Older daemon/proxy</div><div>do not know support disabling flushing. The docs mention that this</div><div>query string will be added in 1.3, we are now at 1.2.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if h['needs_auth']:<br>
+ http.putheader("Authorization", transfer.signed_ticket) </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ # The oVirt server only uses the first part of the range, and the<br>
+ # content-length.</blockquote><div> </div><div>[snipped]</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+def zero(h, count, offset, may_trim):<br>
+ http = h['http']<br>
+ transfer=h['transfer']<br>
+ transfer_service=h['transfer_service']<br>
+<br>
+ # Unlike the trim and flush calls, there is no 'can_zero' method<br>
+ # so nbdkit could call this even if the server doesn't support<br>
+ # zeroing. If this is the case we must emulate.<br>
+ if not h['can_zero']:<br>
+ emulate_zero(h, count, offset)<br>
+ return<br>
+<br>
+ # Construct the JSON request for zeroing.<br>
+ buf = json.dumps({'op': "zero",<br>
+ 'offset': offset,<br>
+ 'size': count,<br>
+ 'flush': False})<br>
+<br>
+ http.putrequest("PATCH", h['path'])<br>
+ http.putheader("Content-Type", "application/json")<br>
+ if h['needs_auth']:<br>
+ http.putheader("Authorization", transfer.signed_ticket)<br></blockquote><div><br></div><div>Only GET and PUT on a server that does not implement OPTIONS need auth.</div><div><br></div><div>This will work if h['needs_auth'] is set correctly, but I think it is better to include</div><div>this check only in pread() and pwrite(), and add a comment there that this is</div><div>need to support legacy versions.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ http.putheader("Content-Length", len(buf))<br>
+ http.endheaders()<br>
+ http.send(buf)<br>
+<br>
+ r = http.getresponse()<br>
+ if r.status != 200:<br>
+ transfer_service.pause()<br>
+ h['failed'] = True<br>
+ raise RuntimeError("could not zero sector (%d, %d): %d: %s" %<br>
+ (offset, count, r.status, r.reason))<br>
+<br>
+# qemu-img convert starts by trying to zero/trim the whole device.<br>
+# Since we've just created a new disk it's safe to ignore these<br>
+# requests as long as they are smaller than the highest write seen.<br>
+# After that we must emulate them with writes.<br></blockquote><div><br></div><div>I think this comment is not related to this code. Maybe it belongs to </div><div>write() where we compute the highwrite?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+def emulate_zero(h, count, offset):<br>
+ if offset+count < h['highestwrite']:<br>
+ http.putrequest("PUT", h['path'] + "?flush=n")<br></blockquote><div><br></div><div>This is is used only on old daemon/proxy, the flush has no effect.</div><div> </div><div>[snipped]</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+def trim(h, count, offset):<br>
+ http = h['http']<br>
+ transfer=h['transfer']<br>
+ transfer_service=h['transfer_service']<br>
+<br>
+ # Construct the JSON request for trimming.<br>
+ buf = json.dumps({'op': "trim",<br>
+ 'offset': offset,<br>
+ 'size': count,<br>
+ 'flush': False})<br>
+<br>
+ http.putrequest("PATCH", h['path'])<br>
+ http.putheader("Content-Type", "application/json")<br>
+ if h['needs_auth']:<br>
+ http.putheader("Authorization", transfer.signed_ticket)<br></blockquote><div><br></div><div>Never needed.</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+def flush(h):<br>
+ http = h['http']<br>
+ transfer=h['transfer']<br>
+ transfer_service=h['transfer_service']<br>
+<br>
+ # Construct the JSON request for flushing. Note the offset<br>
+ # and count must be present but are ignored by imageio.<br>
+ buf = json.dumps({'op': "flush",<br>
+ 'offset': 0,<br>
+ 'size': 0,<br>
+ 'flush': True})<br></blockquote><div><br></div><div>Should be (discussed in v6)</div><div><br></div><div> buf = json.dumps({"op": "flush"})</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ http.putrequest("PATCH", h['path'])<br>
+ http.putheader("Content-Type", "application/json")<br>
+ if h['needs_auth']:<br>
+ http.putheader("Authorization", transfer.signed_ticket)<br></blockquote><div><br></div><div>Never needed.</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+def close(h):<br>
+ http = h['http']<br>
+ connection = h['connection']<br>
+<br>
+ http.close()<br>
+<br>
+ # If we didn't fail, then finalize the transfer.<br>
+ if not h['failed']:<br>
+ disk = h['disk']<br>
+ transfer_service=h['transfer_service']<br>
+<br>
+ transfer_service.finalize()<br></blockquote><div><br></div><div>If this raises, we never clean up</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+ # Write the disk ID file. Only do this on successful completion.<br>
+ with builtins.open(params['diskid_file'], 'w') as fp:<br>
+ fp.write(<a href="http://disk.id" rel="noreferrer" target="_blank">disk.id</a>)<br></blockquote><div><br></div><div>If this raises, we will not remove the disk, or close the connection.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ # Otherwise if we did fail then we should delete the disk.<br>
+ else:<br>
+ disk_service = h['disk_service']<br>
+ disk_service.remove()<br>
+<br>
+ connection.close()<br></blockquote><div><br></div><div>[snipped]</div><div><br></div><div>I did not check all the SDK related code, I'm not very familiar with the SDK.</div><div><br></div><div>Thanks for creating this, and sorry for the bad documentation on our side :-)</div><div><br></div><div>Nir</div></div></div>