6:41 a.m.
On Sat, Mar 24, 2018 at 10:36:06PM +0000, Nir Soffer wrote:
On Thu, Mar 22, 2018 at 5:25 PM Richard W.M. Jones
<rjones(a)redhat.com>
wrote:
> PROBLEMS:
> - -of qcow2 does not work, with multiple problems
> * needs to set NBD size to something larger than virtual size
>
How is this related to the actual file size specified when you create a
disk?
In block storage, qcow2 image is always on a thin provisioned disk, which
in oVirt is a regular logical volume, created at the requested
"initial_size":
[...]
initial_size=image_size,
provisioned_size=image_info["virtual-size"],
Can you describe exactly what these two sizes mean?
Remember that virt-v2v works by streaming. At the point where we are
calling this API virt-v2v only knows the virtual size. We never know
the size of the final qcow2 file.
sparse=disk_format == types.DiskFormat.COW,
storage_domains=[
types.StorageDomain(
name='mydata'
)
]
)
)
Internally we do allocated 10% more then the requested initial_size
to leave room for qcow2 metadata.
10% more than what?
See:
https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a385709...
https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a385709...
This leave me more confused than before :-(
> - Cannot choose the datacenter.
The storage domain belongs to some data center, so I don't think you
need to select a data center.
OK I didn't know this.
> +from http.client import HTTPSConnection
> +from urllib.parse import urlparse
>
These will not work in python 2, but you can use six.moves to have
code that works on both 2 and 3.
For RHEL I'm applying a second downstream-only patch which converts
the Python 3 code to Python 2:
https://github.com/libguestfs/libguestfs/commit/bff845c86dcb12c720b38fc60...
[snipped]
> + # Connect to the server.
> + connection = sdk.Connection(
> + url = params['output_conn'],
> + username = username,
> + password = password,
> + ca_file = params['rhv_cafile'],
>
Can this be None?
We could allow that, but in the current code it must be present.
> + log = logging.getLogger(),
> + insecure = params['insecure'],
>
If ca_file cannot be None, then insecure is not needed, based
on Ondra review from earlier version.
Is this really true? My reading of the code is that the insecure flag
verifies the server to the client, whereas the certificate bundle is
for verifying the client to the server.
[snipped]
> + # Create the disk.
> + disks_service = system_service.disks_service()
> + if params['disk_format'] == "raw":
> + disk_format = types.DiskFormat.RAW
> + else:
> + disk_format = types.DiskFormat.COW
> + disk = disks_service.add(
> + disk = types.Disk(
> + name = params['disk_name'],
> + description = "Uploaded by virt-v2v",
> + format = disk_format,
> + provisioned_size = params['disk_size'],
>
This must be the virtual size.
You don't specify initial_size - in this case you get 1G, and most
images will fail to upload.
This works for me, and I'm using a guest which is larger than 1G. As
above can you explain what these numbers are supposed to be, and note
that we only know the virtual size (params['disk_size']). We cannot
know any other information because we're streaming the data, so
anything that requires us to know the final size of the image is a
non-starter.
> + sparse = params['output_sparse'],
>
The user cannot configure that. This must be based on the image
format. The current coded may create images in unsupported
combinations, e.g. raw/sparse on block storage, or fail validation
in engine.
In virt-v2v this can be configured using ‘-oa’. What are the possible
combinations?
[snipped]
> +# Can we issue zero, trim or flush requests?
>
+def get_options(h):
> + if h['got_options']:
> + return
> + h['got_options'] = True
> +
> + http = h['http']
> + transfer=h['transfer']
> +
> + http.putrequest("OPTIONS", h['path'])
> + http.putheader("Authorization", transfer.signed_ticket)
> + http.endheaders()
> +
> + r = http.getresponse()
> + if r.status == 200:
> + j = json.loads(r.read())
> + h['can_zero'] = "zero" in j['features']
> + h['can_trim'] = "trim" in j['features']
> + h['can_flush'] = "flush" in j['features']
> +
> + # If can_zero is true then it means we're using the new imageio
> + # which never needs the Authorization header.
> + if h['can_zero']:
> + h['needs_auth'] = False
>
If we got here, we are working with new daemon or proxy, and both
of them do not need auth, so we can set 'needs_auth' to False
if OPTIONS returned 200 OK.
Which is what this code does, unless I'm misunderstanding things.
> + # Old imageio servers returned 405 Method Not Allowed. If
> + # we see that we just say that no operations are allowed and
> + # we will emulate them.
> + elif r.status == 405:
> + h['can_zero'] = False
> + h['can_trim'] = False
> + h['can_flush'] = False
>
I would set all the defaults when creating the sate dict. This ensures
that we don't forget needs_auth or other keys and crash later with
KeyError, and make it easier to understand what is the state managed
by the plugin.
Yup, strong typing FTW ...
[snipped]
+def pwrite(h, buf, offset):
> + http = h['http']
> + transfer=h['transfer']
> + transfer_service=h['transfer_service']
> +
> + count = len(buf)
> + h['highestwrite'] = max(h['highestwrite'], offset+count)
> +
> + http.putrequest("PUT", h['path'] + "?flush=n")
>
"?flush=n" has effect only if h["can_flush"] is True. Older
daemon/proxy
do not know support disabling flushing. The docs mention that this
query string will be added in 1.3, we are now at 1.2.
But it doesn't seem to break the old imageio?
Eventually we'll have nbdkit > 1.1.25 which will support pwrite-with-
FUA-flag. Then we'll be able to set flush=(n|y) correctly. However
this is not likely to happen until after RHEL 7.6.
> +def zero(h, count, offset, may_trim):
> + http = h['http']
> + transfer=h['transfer']
> + transfer_service=h['transfer_service']
> +
> + # Unlike the trim and flush calls, there is no 'can_zero' method
> + # so nbdkit could call this even if the server doesn't support
> + # zeroing. If this is the case we must emulate.
> + if not h['can_zero']:
> + emulate_zero(h, count, offset)
> + return
> +
> + # Construct the JSON request for zeroing.
> + buf = json.dumps({'op': "zero",
> + 'offset': offset,
> + 'size': count,
> + 'flush': False})
> +
> + http.putrequest("PATCH", h['path'])
> + http.putheader("Content-Type", "application/json")
> + if h['needs_auth']:
> + http.putheader("Authorization", transfer.signed_ticket)
>
Only GET and PUT on a server that does not implement OPTIONS need auth.
This will work if h['needs_auth'] is set correctly, but I think it is
better to include
this check only in pread() and pwrite(), and add a comment there that this
is
need to support legacy versions.
So I think you mean that we can remove the if h['needs_auth']
and following line completely?
> + http.putheader("Content-Length", len(buf))
> + http.endheaders()
> + http.send(buf)
> +
> + r = http.getresponse()
> + if r.status != 200:
> + transfer_service.pause()
> + h['failed'] = True
> + raise RuntimeError("could not zero sector (%d, %d): %d: %s" %
> + (offset, count, r.status, r.reason))
> +
> +# qemu-img convert starts by trying to zero/trim the whole device.
> +# Since we've just created a new disk it's safe to ignore these
> +# requests as long as they are smaller than the highest write seen.
> +# After that we must emulate them with writes.
>
I think this comment is not related to this code. Maybe it belongs to
write() where we compute the highwrite?
It relates to the conditional in this function:
> +def emulate_zero(h, count, offset):
> + if offset+count < h['highestwrite']:
> + http.putrequest("PUT", h['path'] + "?flush=n")
>
This is is used only on old daemon/proxy, the flush has no effect.
OK.
> +def flush(h):
> + http = h['http']
> + transfer=h['transfer']
> + transfer_service=h['transfer_service']
> +
> + # Construct the JSON request for flushing. Note the offset
> + # and count must be present but are ignored by imageio.
> + buf = json.dumps({'op': "flush",
> + 'offset': 0,
> + 'size': 0,
> + 'flush': True})
>
Should be (discussed in v6)
buf = json.dumps({"op": "flush"})
This contradicts the documentation, but I can change the code.
> +def close(h):
> + http = h['http']
> + connection = h['connection']
> +
> + http.close()
> +
> + # If we didn't fail, then finalize the transfer.
> + if not h['failed']:
> + disk = h['disk']
> + transfer_service=h['transfer_service']
> +
> + transfer_service.finalize()
>
If this raises, we never clean up
Understood, I'll try to make this function a bit more robust.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW