<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, Mar 25, 2018 at 2:41 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">On Sat, Mar 24, 2018 at 10:36:06PM +0000, Nir Soffer wrote:<br>
> On Thu, Mar 22, 2018 at 5:25 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com" target="_blank">rjones@redhat.com</a>><br>
> wrote:<br>
><br>
> > PROBLEMS:<br>
> > - -of qcow2 does not work, with multiple problems<br>
> > * needs to set NBD size to something larger than virtual size<br>
> ><br>
><br>
> How is this related to the actual file size specified when you create a<br>
> disk?<br>
><br>
> In block storage, qcow2 image is always on a thin provisioned disk, which<br>
> in oVirt is a regular logical volume, created at the requested<br>
> "initial_size":<br>
[...]<br>
> initial_size=image_size,<br>
> provisioned_size=image_info["virtual-size"],<br>
<br>
Can you describe exactly what these two sizes mean?<br></blockquote><div><br></div><div>- provisioned_size is the virtual size of the image.</div><div>- initial_size is used only for thin provisioned disk on block storage. This is</div><div>the size of the logical volume we created for this disk.</div><div><br></div><div>On thin disk on block storage you will not be able to write or zero more</div><div>then initial_size bytes.</div><div><br></div><div>When a vm is running, vdsm monitor the allocated space and ask the SPM</div><div>host to allocated more space when the highest write offset is too high, so</div><div>the disk looks like a thin provisioned disk to the vm. This mechanism is</div><div>not available for storage operations, and all of them specify initial_size</div><div>when converting images to qcow2 format.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Remember that virt-v2v works by streaming. At the point where we are<br>
calling this API virt-v2v only knows the virtual size. We never know<br>
the size of the final qcow2 file.<br></blockquote><div><br></div><div>If you don't have a way to estimate the final size you need to allocated</div><div>the entire image when you create a disk.</div><div><br></div><div>But I don't understand why you don't have access to the image. I understood</div><div>that the flow is:</div><div><br></div><div>image file -> qemu-img convert -> nbdkit -> ovirt-imageio -> ovirt disk<br></div><div><br></div><div>In this flow you can estimate the size using the image file before starting</div><div>the streaming process.</div><div><br></div><div>If there is no way to estimate the size and you must allocate the entire</div><div>image, we can add a shrink step in upload finalization, to shrink the image</div><div>size to optimal size. We are already doing this in several flows that</div><div>cannot estimate the final disk size and do over allocation.</div><div><br></div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > + log = logging.getLogger(),<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > + insecure = params['insecure'],<br>
> ><br>
><br>
> If ca_file cannot be None, then insecure is not needed, based<br>
> on Ondra review from earlier version.<br>
<br>
Is this really true? My reading of the code is that the insecure flag<br>
verifies the server to the client, whereas the certificate bundle is<br>
for verifying the client to the server.<br></blockquote><div><br></div><div>Maybe, I'm not familiar with the SDK.</div><div><br></div><div>What is the motivation for disabling this flag?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> [snipped]<br>
><br>
> > + # 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>
> ><br>
><br>
> This must be the virtual size.<br>
><br>
> You don't specify initial_size - in this case you get 1G, and most<br>
> images will fail to upload.<br>
<br>
This works for me, and I'm using a guest which is larger than 1G. As<br>
above can you explain what these numbers are supposed to be, and note<br>
that we only know the virtual size (params['disk_size']). We cannot<br>
know any other information because we're streaming the data, so<br>
anything that requires us to know the final size of the image is a<br>
non-starter.<br></blockquote><div><br></div><div>Discussed above.</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>
> ><br>
> The user cannot configure that. This must be based on the image<br>
> format. The current coded may create images in unsupported<br>
> combinations, e.g. raw/sparse on block storage, or fail validation<br>
> in engine.<br>
<br>
In virt-v2v this can be configured using ‘-oa’. What are the possible<br>
combinations?<br></blockquote><div><br></div><div>I could not find documentation for this in the ovirt engine sdk.</div><div>There is <a href="http://ovirt.github.io/ovirt-engine-sdk/master/">http://ovirt.github.io/ovirt-engine-sdk/master/</a> but it is<br></div><div>not very useful for anything.</div><div><br></div><div>The ovirt-engine REST API has real documentation here:</div><div><a href="http://ovirt.github.io/ovirt-engine-api-model/master">http://ovirt.github.io/ovirt-engine-api-model/master</a></div><div><br></div><div>But I could not find any documentation about creating disks there, so</div><div>the above is based on current system behavior.</div><div><br></div><div>When creating disks from the UI, the user select the storage domain</div><div>(implicitly selecting the file/block), and the allocation policy</div><div>(thin/preallocated). The user cannot select the image format when</div><div>creating disk, ovirt will choose the image format and sparse for you.</div><div><br></div><div><font face="monospace">storage type allocation | image format sparse creating</font></div><div><font face="monospace">---------------------------|-----------------------------------------------------------------</font></div><div><font face="monospace">file thin | raw true sparse file of provisioned_size bytes </font></div><div><font face="monospace">file preallocated | raw false preallocated file of provisioned_size bytes</font></div><div><font face="monospace">block thin | qcow2 true logical volume of initial_size bytes</font></div><div><font face="monospace">block preallocated | raw false logical volume of provisioned_size bytes</font></div><div><br></div><div>When uploading disk, the user select a file, implicitly selecting the</div><div>image format (raw/qcow2), and the storage domain, implicitly selecting</div><div>the storage type (file/block). oVirt selects the allocation and sparse</div><div>value for you.</div><div><br></div><div><font face="monospace">storage type image format | allocation sparse creating</font></div><div><font face="monospace">---------------------------|---------------------------------------------------------------</font></div><div><div><font face="monospace">file qcow2 | thin true sparse file of image size bytes</font></div><div><font face="monospace">file raw | thin true sparse file of provisioned_size bytes</font></div><div><font face="monospace">block qcow2 | thin true </font><span style="font-family:monospace">logical volume of initial_size bytes</span></div><div><font face="monospace">block raw | preallocated false logical volume of provisioned_size bytes</font></div></div><div><br></div><div>Notes:</div><div>- file,qcow2 cannot be created from the UI.</div><div>- no way to create preallocated uploading from the UI (looks like a bug to me)</div><div><br></div><div>When using the sdk, you can select both the image format and sparse, so you</div><div>can create invalid combinations. oVirt selects the allocation for you.</div><div><br></div><div><div><font face="monospace">storage type image format sparse | allocation creating</font></div><div><font face="monospace">-----------------------------------|--------------------------------------------------------</font></div><div><div><font face="monospace">file qcow2 true | thin sparse file of image size bytes</font></div><div><font face="monospace">file raw false | preallocated preallocated file of provisioned_size bytes</font></div><div><font face="monospace">file raw true | thin sparse file of </font><span style="font-family:monospace">provisioned_size bytes</span></div><div><div><font face="monospace">file qcow2 false | - unsupported</font></div></div><div><font face="monospace">block qcow2 true | thin </font><span style="font-family:monospace">logical volume of initial_size bytes</span></div><div><span style="font-family:monospace">block raw false | preallocated logical volume of provisioned_size bytes</span><br></div></div></div><div><div><div><div><div><font face="monospace">block qcow2 false | - unsupported</font></div></div><div><font face="monospace">block raw true | - unsupported</font></div></div></div></div><div><br></div><div>The only case when selecting the sparse value is useful is raw file</div><div>on file based storage domain, allowing creation of sparse or preallocated</div><div>disk.</div><div><br></div><div>I guess you can check if a storage domain is file based or block basedu</div><div>using the SDK, but I'm not familiar with it.</div><div><br></div><div>Daniel, do we expose this info?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> [snipped]<br>
><br>
> > +# Can we issue zero, trim or flush requests?<br>
> ><br>
> +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()) </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > + 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><div>In older proxy, we do support OPTIONS, used for allowing access</div><div>from enigne UI, but it does not return any content. Fortunately, we </div><div>return httplib.NO_CONTENT so it is easy to detect.</div><br></div><div>So we need to handle gracefully r.status = 204</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> ><br>
><br>
> If we got here, we are working with new daemon or proxy, and both<br>
> of them do not need auth, so we can set 'needs_auth' to False<br>
> if OPTIONS returned 200 OK.<br>
<br>
Which is what this code does, unless I'm misunderstanding things.<br></blockquote><div><br></div><div>Accessing readonly ticket will return empty features list, and h['needs_auth']</div><div>will be True, while this server does not need auth. Yes, it cannot happen</div><div>with this code but it is not the right check.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> [snipped]<br>
><br>
> +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>
> ><br>
><br>
> "?flush=n" has effect only if h["can_flush"] is True. Older daemon/proxy<br>
> do not know support disabling flushing. The docs mention that this<br>
> query string will be added in 1.3, we are now at 1.2.<br>
<br>
But it doesn't seem to break the old imageio?<br></blockquote><div><br></div><div>Old imageio just ignores this, so it is safe to use as is, the issue is again</div><div>confusing the reader that this has some effect on the server.</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > + # 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>
> ><br>
><br>
> Only GET and PUT on a server that does not implement OPTIONS need auth.<br>
><br>
> This will work if h['needs_auth'] is set correctly, but I think it is<br>
> better to include<br>
> this check only in pread() and pwrite(), and add a comment there that this<br>
> is<br>
> need to support legacy versions.<br>
<br>
So I think you mean that we can remove the if h['needs_auth']<br>
and following line completely?<br></blockquote><div><br></div><div>Yes, probably can be removed.</div><div> </div><div>[snipped]</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > + buf = json.dumps({'op': "flush",<br>
> > + 'offset': 0,<br>
> > + 'size': 0,<br>
> > + 'flush': True})<br>
> ><br>
><br>
> Should be (discussed in v6)<br>
><br>
> buf = json.dumps({"op": "flush"})<br>
<br>
This contradicts the documentation, but I can change the code.<br></blockquote><div><br></div><div>Agree, I'll fix the docs.</div><div><br></div><div>Nir</div></div></div>