Implement SXM with snapshots for SMAPIv3#7137
Conversation
When reverting a snapshot, XAPI clones the snapshot disk via VDI.clone to create a new active VDI. The vdi_clone_impl in xapi-storage-script called Volume_client.clone but forgot to write the vdi-type key into the new volume's key store. vdi_create_impl does write this key, but vdi_clone_impl never did. After the revert, the next SR.scan reads back the volume metadata and gets ty = "" for the new active VDI because the key is missing. This empty type ends up in the XAPI database and breaks any logic that checks VDI type, including the snapshot migration flow. The fix adds the missing update_keys step to vdi_clone_impl. Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
this failure is in qcow-stream-tool, not touched by this PR — likely an opam version mismatch from the upstream qcow2 work. |
Yes, this is because xs-opam was updated and the code in this branch is incompatible |
355e8ad to
da929c8
Compare
Sync master to feature, pass the check now. |
LunfanZhang
changed the title
|
Is there a way to test the patch ? Do you have some smapiv3 plugins that support migration? |
Of course, there is SMAPIv3 plugin to support the migration, and which has been verified. |
Do you have a link to download it? |
No link, It`s an internal repo. but the document of SMAPIv3 is opensource, you can access it from XAPI project. |
We have some SMAPIv3 plugins (such as https://github.com/xcp-ng/xcp-ng-xapi-storage), but AFAIK migration is not supported. I read the SMAPIv3 documentation, but verifying the functionality would be easier with a real plugin that supports migration. |
da929c8 to
ad50d8b
Compare
| >>>= (fun sr -> | ||
| clone ~dbg ~sr | ||
| ~vdi:(Storage_interface.Vdi.string_of vdi_info.Storage_interface.vdi) | ||
| >>>= update_keys ~dbg ~sr ~key:_vdi_type_key |
There was a problem hiding this comment.
From https://xapi-project.github.io/xapi-storage/#volume-method-clone, "Note the name and description are copied but any extra metadata associated by [set] is not copied."
Is this key included in the metadata? If so, it seems to me that this should be done in revert code logic as clone is only a step of revert.
There was a problem hiding this comment.
Yes -vdi-type is exactly one of those set keys, so per the API doc at https://xapi-project.github.io/xapi-storage/#volume-method-clone, invoke clone does not carry it to the new volume, thats reason for adding this set here, and storage side behavior will not handle. I dont think put it into the revert process is good solution as MAPIv3 has no dedicated revert primitive, and this is a general bug for clone process. so clone_impl is the single choke point.
There was a problem hiding this comment.
The commit message says: "When reverting a snapshot" while the "VDI.clone" doc says it will not set any metadata. This makes me think that there are other use of VDI.clone with the contract that metadata will not be set by VDI.clone. Will breaking the contract not break other uses if there are any?
There was a problem hiding this comment.
Will breaking the contract not break other uses if there are any?
I am not sure, for safety, I can move it out to the revert process.
But I cannot understand why clone a VDI without a type or a VDI with different type from it clone to.
| let rpc = of_sr sr | ||
| end)) in | ||
| C.VDI.set_snapshot_metadata dbg sr snapshot leaf snapshot_time true | ||
| with e -> |
There was a problem hiding this comment.
Is it really proper to swallow the error? It's local operation. The failure would suggest severe error. And it would be worse given the backend metadata is the main copy.
There was a problem hiding this comment.
For the set_* calls to the local xapi DB, I think it's fine to raise an error. But for the sync to storage the behaviour differs: for SMAPIv3, there's a split DB so this sync is required; whereas for SMAPIv1, all the VDI and SR records are stored in xapi, so this sync isn't necessary. So I think a better option might be to narrow down the scope of the try..except and only protect the sync-to-storage function with a D.log_and_ignore_exn, to avoid it breaking the migration when it's v1.
| (** SXM v3 (SMAPIv3) pre-flight: the destination reconstructs the base chain | ||
| from the VM-snapshot tree, so refuse snapshot shapes that tree cannot | ||
| express - hidden VDI.snapshots and orphan snapshot VDIs. *) | ||
| let assert_sxm_v3_migratable ~__context ~vm_uuid ~vms_vdis ~snapshots_vdis = |
There was a problem hiding this comment.
I would suggest to keep ocaml/xapi/xapi_vm_migrate.ml clean from the specific SMAPI version logic. It stands for the common steps of migration for SMAPIv1 and SMAPIv3.
There was a problem hiding this comment.
Move the assert_migratable to storage_smapiv3_migrate.ml as it is SMAPIv3 specific.
…ation When migrating from SMAPIv1 to SMAPIv3, the snapshot relationship fields (snapshot_of, snapshot_time, is_a_snapshot) were only updated in the XAPI database, not on the destination backend storage. A subsequent SR.scan then read the stale custom keys back from the volume metadata and overwrote the XAPI database, which removed the relation between the snapshots and their leaf VDI (observed on GFS2 SRs). This adds a new VDI.set_snapshot_metadata SMAPIv2 operation that writes these fields into the volume key store. It is declared in storage_interface, defaulted in storage_skeleton, implemented in xapi-storage-script (SMAPIv3), and wired through storage_mux. SMAPIv1 implements it as a no-op since its snapshot metadata is managed by the XAPI database only. SR.update_snapshot_info_dest now calls it best-effort after updating the database (errors are logged and swallowed, the XAPI DB remains authoritative) so the backend metadata stays consistent across an SR.scan. Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
During SXM mirroring the DATA.stat polling loop repeatedly calls VDI.stat, which issues a Volume.stat RPC to the storage script on every poll. This adds unnecessary load and latency while waiting for a mirror to complete. Introduce a lightweight per-(SR, VDI) cache in DATAImpl: [mirror] populates it at mirror start, [stat] serves from it on a cache hit instead of calling Volume.stat, and the entry is evicted when the mirror completes or fails. The cache needs no mutex because the module runs in a cooperative Lwt async context. Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
…->SMAPIv3 SXM
V1 receive_finalize_common takes a "dummy" VDI snapshot of the dest leaf
(storage_smapiv1_migrate.ml:657) so that VDI.compose can later merge the
mirrored leaf onto its parent. On SMAPIv1 SRs the dummy must be destroyed
explicitly after compose; on SMAPIv3 SRs Volume.compose silently consumes
the dummy backend volume but leaves the XAPI DB row behind, so the
subsequent VDI.destroy raises Volume_does_not_exist (currently swallowed
by log_and_ignore_exn) and the orphan DB row persists. Any subsequent
SXM v3 migration then aborts in assert_smapiv3_snapshot_topology_migratable
because the orphan looks like a hidden vdi-snapshot of the active disk.
Catch the V3-specific Backend_error_with_backtrace("SR_BACKEND_FAILURE",
_ :: "Volume_does_not_exist" :: uuid :: _) pattern (with uuid match
against r.dummy_vdi) and trigger SR.scan via the public XenAPI to
reconcile the orphan DB row. All other failures preserve the historical
V1 best-effort tolerate-and-log behaviour, so SMAPIv1 destinations are
unaffected.
reconcile_sr_after_v3_destroy is placed in storage_migrate_helper so the
in-flight SMAPIv3-native migration engine can reuse it.
Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
ad50d8b to
d208caf
Compare
| that are unspecified go to the suspend_sr_ref defined above. | ||
| SMAPIv3 snapshots are excluded because they are mirrored during send_start. *) | ||
| let copyable_snapshots = | ||
| List.filter (fun vconf -> not (is_smapiv3_sr vconf.sr)) snapshots_vdis |
There was a problem hiding this comment.
The logic for SMAPIv1 and SMAPIv3 diverse dramatically here 😞
There was a problem hiding this comment.
Now I understand how different SMAPIv1 and SMAPIv3 will be. Two alternatives:
- if SMAPIv3 could fit into the existing pattern of
with_many+vdi_copy_fun, it would be the best; - otherwise, move the pattern to SMAPIv1 specific part.
There was a problem hiding this comment.
SMAPIv3 is nearly impossible to fit into with_many + vdi_copy_fun as the vdi_copy_fun is rely on the similar_content + content_id to recreate relation and rely on the sparsse_dd to only copy the diff of vhd.
Current new SMAPIv3 re-use with_many to handle all the leaf node but exclude all the snapshots, and base on the leaf node it walks the whole tree to obtain the full relationship.
| 3. walking the live VM's snapshot tree and projecting each snapshot VM | ||
| onto the lineage. | ||
| Returns root nodes sorted by [snapshot_time] (oldest first). *) | ||
| let get_snapshot_tree ~dbg ~vdi = |
There was a problem hiding this comment.
The root of the tree is deduced from the VM snapshots. Is it possible that under the storage's hood, it has actually a parent?
If so, then:
- it should be copied/mirrored with all data up to the actually root
- the migration will cut off the relationship between the root and its parent on destination side. So when migrate it back, it will be a independent tree.
There was a problem hiding this comment.
I don't think relying on the storage to provide the parent is a perfect solution either.
In the old SMAPIv1, it depends on the storage layer to write a vhd-parent key on the storage side to express the chain layering (the parent-child relationship); this also ties SXMv1 to a particular disk format — although, admittedly, all current SMAPIv1 SRs are VHD-based.
For SMAPIv3 we may not want to bind ourselves to qcow2 either, since InfoScale appears to take a different approach that doesn't rely on the disk format to determine the relationship.
Long-winded way of saying: we may want a more general mechanism that doesn't depend on a specific SR type or a specific key to reconstruct the relationship.
There was a problem hiding this comment.
I mean the root is not actually a root in storage layer. We can't know it from XAPI's point of view.
There was a problem hiding this comment.
There could be a case that VDI has taken a VDI level snapshots, which will generate a root which is cannot know from XAPI, but for this case assert_sxm_v3_migratable can filter it out and prevent it before migration. otherwise, I do not find an exception here for now.
gthvn1
left a comment
gthvn1
left a comment
There was a problem hiding this comment.
I request changes to ping you but it is more a question.
| List.concat_map | ||
| (fun child -> | ||
| let branch_vdi, branch_dp, branch_nbd_uri, cleanup = | ||
| prepare_branch_vdi ~ctx ~dest_snapshot |
There was a problem hiding this comment.
In prepare_branch_vdi you are cloning the snapshot. And the cleanup is returned to remove it. But if a remote call raised an exception (for example Remote.VDI.attach3), the it has been cloned but the cleanup is not called. Don't we have a potential VDI leak here?
There was a problem hiding this comment.
Good catch, update to move the internal VDI.activate VDI.attach operation to be protected with a try. Exception and trigger clean up when any of these calls raise an exception.
There was a problem hiding this comment.
Hi @gthvn1 The code has been updated, any more reason to keep request changes?
There was a problem hiding this comment.
Sorry for the delay. I will have a look this afternoon but I think there will be no more reason ;)
There was a problem hiding this comment.
@LunfanZhang I have the same issue when building:
make[1]: Leaving directory '/home/gthvn1/xen-api/_build/default/ocaml/forkexecd/helper'
File "ocaml/qcow-stream-tool/qcow_stream_tool.ml", line 17, characters 8-24:
17 | data_cluster_map
^^^^^^^^^^^^^^^^
Error: This expression has type Qcow_mapping.t
but an expression was expected of type 'a Qcow_types.Cluster.Map.t
make: *** [Makefile:17: build] Error 1
I have updated my gloabl switch with last xs-opam. Do you remember how you fixed that? I always have trouble with xs-opam...
d208caf to
3d62217
Compare
Implement the new SXM v3 storage-layer migration for SMAPIv3 SRs, enabling a VDI to be mirrored together with its snapshot chain, including reverted/nested snapshot structures. storage_smapiv3_migrate gains snapshot-tree discovery (get_snapshot_tree), the DFS walk that mirrors/clones each node while keeping the storage chain advancing on the same leaf, NBD proxy export, and the send_start orchestration that drives the per-node mirroring. storage_migrate_helper.State is extended with a snapshot_relation type and set/get/remove_snapshot_mappings so the source->destination snapshot pairings recorded during mirroring can be retrieved later to restore snapshot metadata on the destination. Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
Drive the new SMAPIv3 snapshot-aware mirroring from the VM migration flow. `vdi_copy_fun` now retrieves the snapshot relations recorded during send_start (via Storage_migrate_helper.State.get_snapshot_mappings), builds a mirror record for each mirrored snapshot, and feed the leaf mirror record together with the per-snapshot mirror records into the post-mirror continuation as a single list; the mappings are cleared afterwards. migrate_send' excludes SMAPIv3 snapshots from the set of copyable snapshots because they are already mirrored during send_start. Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
3d62217 to
ee5153e
Compare
4 participants
The new SXM v3 storage-layer engine drives the migration from the source side. For each disk being migrated, it discovers the snapshot tree from the source VM, then DFS-walks the tree and mirrors each node over an NBD proxy onto the destination. Inactive (reverted) branches are mirrored onto cloned working VDIs so they don't affect the active leaf, while the active path keeps advancing on the same destination leaf — so when the walk bottoms out, the destination leaf is correctly positioned for the live-leaf mirror that follows. The source→destination snapshot pairings are recorded so the toolstack can reconstruct the snapshot relationships on the destination once the mirror completes.
The result: a VM with snapshots on a SMAPIv3 SR can now be live-migrated cleanly, with its full snapshot tree (including reverted branches) reproduced on the destination.
Design refer: https://github.com/xapi-project/xen-api/blob/feature/sxm-v3/doc/content/xapi/storage/sxm/sxm-v3-with-snapshot.md