Supported image formats#6712
Conversation
|
I would like to see more explicitly documented and checked the supported image formats. These appear to be some mysterious strings but the user of neither API or CLI should have to guess them, |
Yes, in my mind the user should first check using |
63a9fe1 to
173e37b
Compare
dbfc215 to
ee5b3d6
Compare
|
@psafont could you take another look? |
|
There's an issue with vdi migration when qcow2 is involved. Guillaume is investigating this and see whether the issue is in this PR, or on SM's side |
|
It should be fixed by @AnthoineB . So I will run another another series of manual testing with several all combinations (qcow2 -> vhd, vhd -> qcow2, vhd -> vhd and qcow2 -> qcow2). I will also try to check with different kind of SR (shared and not). |
ee5b3d6 to
98e3219
Compare
|
For information I tried to do several VDI pool migration on our xcp-ng with qcow2 enabled. I was able to migrate from one EXT SR to another switching between format (vhd -> qcow2 -> vhd). I also tried with wrong format like this: I will run more tests using VM migration (I already tried a few migrations successfully). |
|
This looks good, but needs a rebase. I think Guillaume wanted to retest with the newest xcp-ng builds |
|
Sure, I will rebase and I will rebuild it based on our last builds to run our CI with this modification. |
97b33f9 to
3151fa3
Compare
|
As spotted by @psafont the issue with SMAPIv3 driver was that the plugin in xapi-storage-script generator required a default parameter (I added a default one for xapi but not for the generator). |
|
For your information, I was able to run all the storage tests on our CI with no errors. Of course our CI doesn't use the new field but at least this allows to validate that it works with current configurations. |
psafont
left a comment
There was a problem hiding this comment.
I thought this would be almost ready to go, but I've seen that there's a TODO, and the generator for smapiv3 plugins has not been modified, which is not what I was expecting, can you expand on those? The latter may be left for later because it shouldn't break anything, just not add nice support for the feature in smapiv3. but I don't understand the implication in the mirror call
| let schema_major_vsn = 5 | ||
|
|
||
| let schema_minor_vsn = 792 | ||
| let schema_minor_vsn = 793 |
There was a problem hiding this comment.
| let schema_minor_vsn = 793 | |
| let schema_minor_vsn = 794 |
There was a problem hiding this comment.
Are you sure? Currently it is 792 right?
There was a problem hiding this comment.
Not after this is merged: https://github.com/xapi-project/xen-api/pull/6820/changes ;)
Mmm strange because I see the default value that was missing previously Oh damned yes I forgot about this TODO in the xapi-storage-script. As we don't really use smapiv3 I just ensure that it doesn't break things but I don't know either the impact on mirroring. I will check that... |
3151fa3 to
d6757e8
Compare
|
While checking for smapiv3 plugin, I looked at what has been run in our CI, and in fact we are only testing the creation and deletion of SRs, as well as VMs on the SR. It appears that we are not doing any migration tests because the drivers we are currently shipping do not support migration. I will look into how to proceed further. |
|
Thanks for looking into this |
|
Is this still active or could be moved to draft if not? |
|
This still needs to be tested on smapiv3, which currently is not possible since there's no backend available for testing that supports migration, |
d6757e8 to
69c4d60
Compare
|
I started adding some quick tests for storage migration. I can add more tests, or I can propose it independently of the supported image format, since the pool migration currently uses an extra parameter. |
a2c6a3c to
97d0fcd
Compare
As there are no SMAPIv3 plugins that support live migration, we are using an empty list for now, and it cannot be modified at this time. |
97d0fcd to
60e1b90
Compare
|
| ) | ||
| ) | ||
|
|
||
| let receive_start _ctx ~dbg:_ ~sr:_ ~vdi_info:_ ~id:_ ~similar:_ = |
There was a problem hiding this comment.
Are you sure it's safe to modify calls like this? Could you check the reason there are 3 different receive_start calls? We might need newer ones. I think it might be related to backwards compatibility when doing RPUs
There was a problem hiding this comment.
Yes I will check. But you are correct that there are different flavor for backwards compatibility.
There was a problem hiding this comment.
After testing migrations from a host with a supported image format to another host (in a different pool) that does not support it (in both directions) I didn’t encounter any issues.
As I understand it, receive_start is called on the source and prepares the mirroring. In the end, it sends a VDI.create XML-RPC call (and some others) to the destination, but I don’t see any issue with receive_start itself.
The process will fail if you explicitly try to use the image format, because the source checks whether the destination SR uses an SM type that supports that format. However, if the option is not set, an empty list is used and no checks are performed against the destination SR, so it should work.
That said, even if I think that it is safe to modify the call, I agree that it doesn’t really make sense for a deprecated API.
I’ll also check intra-pool migration, but as far as I remember, VDI migration behaves the same way.
There was a problem hiding this comment.
So it looks like there is an issue if you want to do a RPU (but it is not related to receive_start). The scenario is with two host A (master) and B:
- vdi pool migrate from A to B (ok)
- update A (so now A supports image format)
- reboot A
- we want to update B so we need to move the VM back to A but:
# xe vdi-pool-migrate uuid=4701fa33-c63d-49ff-baad-80e008dd6ab3 sr-uuid=21c7d26b-1ce7-04ea-1685-ba45e50c4191
You tried to call a method with the incorrect number of parameters. The fully-qualified method name that you used, and the number of received and expected parameters are returned.
method: VDI.pool_migrate
expected: 3
received: 4
There is an issue with the database because the new field is only available on the new host so B complains.
Apr 22 11:01:45 xcp-gtn-ip13 xapi: [ warn||326 HTTPS 10.1.38.12->:::80|host.request_backup D:d98b24f2a7cf|Xapi_database__Db_xml] no lifetime information about SM.supported_image_formats, ignoring
Apr 22 11:01:45 xcp-gtn-ip13 xapi: [ warn||326 HTTPS 10.1.38.12->:::80|host.request_backup D:d98b24f2a7cf|xmlrpc_client] stunnel pid: 347171 caught Db_exn.DBCache_NotFound("missing column", "SM", "supported_image_formats")
Apr 22 11:01:45 xcp-gtn-ip13 xapi: [error||326 :::80|dispatch:host.request_backup D:6671e841a40d|backtrace] host.request_backup D:d98b24f2a7cf failed with exception Db_exn.DBCache_NotFound("missing column", "SM", "supported_image_formats")
Apr 22 11:01:45 xcp-gtn-ip13 xapi: [error||326 :::80|dispatch:host.request_backup D:6671e841a40d|backtrace] Raised Db_exn.DBCache_NotFound("missing column", "SM", "supported_image_formats")
Apr 22 11:01:45 xcp-gtn-ip13 xapi: [error||326 :::80|dispatch:host.request_backup D:6671e841a40d|backtrace] 1/21 xapi Raised at file ocaml/database/schema.ml, line 190
I'm investigating that...
There was a problem hiding this comment.
See the comment below for the explanation
When running `xe sm-list params=all` you will now have the info of supported image formats if the SM plugin specified it in its DRIVER_INFO. The field is called `supported-image-formats`. If the plugin doesn't provide the info the field will be empty. This patch modifies the datamodel and add a new field to store this information into the SM object. Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
This patch allows specifying the destination format for individual VDIs mapped to a destination SR. It adds a new parameter to `VM.migrate_send` and `VM.assert_can_migrate` API. It also adds a new parameter to XE CLI. The format to specify the image format is `image-format:<source VDI UUID>=<destination image format>`. If the given image format cannot be validated, an error is returned. It also adds a new parameter to `VDI.pool-migrate`. This new parameter allows to provide a string that is the destination format. This string is used to check whether the destination SR supports the expected format. If the check fails or cannot be performed due to missing information on the destination SR, an error is returned. Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
Update VM.MigrateSend call to include new VdiFormatMap parameter. Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
A new field supported_image_format and new parameters have been added for: - VM.migrate_send - VM.assert_can_migrate - VDI.pool_migrate Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
Introduce a new quicktest covering local VDI migration between two Storage Repositories (SRs). Add a `migration_path` filter that injects a `(src, dst)` SR pair derived from an `SR.srs` constraint. The filter selects a single valid migration path (if available) and generates one test case. If fewer than two compatible SRs exist, no test is produced. The test: - Creates a VDI on the source SR - Attaches it to a temporary VM - Calls VDI.pool_migrate to the destination SR - Verifies that the VDI's SR has changed accordingly - Cleans up safely, tracking ownership transfer when migration replaces the original VDI Signed-off-by: Guillaume <guillaume.thouvenin@vates.tech>
|
For information I'm currently running these scenarios:
# xe vdi-pool-migrate uuid=882cedbd-e8ef-4c5d-9a57-b99b91516b49 sr-uuid=d835d019-1b97-0080-1bc0-eb52542db3f5 dest-img-format=raw
The server failed to handle your request, due to an internal error. The given message may give details useful for debugging the problem.
message: Storage_error ([S(Internal_error);S(Storage_error ([S(Migration_preparation_failure);S(Storage_error ([S(Unimplemented);S(VDI.snapshot)]))]))]) |
60e1b90 to
d3f45c6
Compare
| "Migrate a VDI to a specified SR, while the VDI is attached to a \ | ||
| running guest." | ||
| "Migrate a VDI to a specified SR, while it is attached to a running \ | ||
| guest. You can specify the image format for the destination." |
There was a problem hiding this comment.
Add what format is allowed: RAW, VHD and QCow2
| ; (Map (String, String), "options", "Other parameters") | ||
| ; ( Map (String, String) | ||
| , "options" | ||
| , "Extra parameters. Supports: \"dest-img-format\" (raw|vhd|qcow2) \ |
There was a problem hiding this comment.
We probably want to be consistent with VM.pool-migrate that uses "img-format" ?
|
This PR implements the supported image format mechanism proposed in this design document: https://xapi-project.github.io/new-docs/design/sm-supported-image-formats/index.html
DRIVER_INFO.This feature is particularly useful because XCP-ng is adding support for the Qcow2 format in SMAPI to allow VDIs larger than 2TB. So in the near future (we're currently releasing the beta version), some SRs will support multiple formats such as VHD, RAW, and Qcow2.
With this patch, it becomes possible to migrate a VM with VHD disks on one SR to another SR with Qcow2 disks. If an SM plugin does not provide information about the supported image formats, the behavior remains unchanged.
For more details see the specification.