[Libguestfs] [PATCH v7 6/6] v2v: Add -o rhv-upload output mode (RHBZ#1557273).
Richard W.M. Jones
rjones at redhat.com
Sun Mar 25 11:41:44 UTC 2018
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 at 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/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/blockVolume.py#L328
>
> https://github.com/oVirt/vdsm/blob/3de6f326d7e338b064b11d2a2269500a3857098b/lib/vdsm/storage/volume.py#L666
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/bff845c86dcb12c720b38fc60dcdaa5a10373081
> [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
More information about the Libguestfs
mailing list