Skip to content

Resolve blockers for SMAPIv3 snapshot-aware migration - #7216

Open
LunfanZhang wants to merge 4 commits into
xapi-project:feature/sxm-v3from
LunfanZhang:private/luzhan/sxm-v3-p1-blockers
Open

Resolve blockers for SMAPIv3 snapshot-aware migration#7216
LunfanZhang wants to merge 4 commits into
xapi-project:feature/sxm-v3from
LunfanZhang:private/luzhan/sxm-v3-p1-blockers

Conversation

@LunfanZhang

Copy link
Copy Markdown
Collaborator

This PR does not contain the SXM v3 snapshot-migration itself. It lands the prerequisite fixes (and one optimization) that currently block it, so they can be reviewed independently. The SXM v3 and its wiring follow in two subsequent PRs.

  • Snapshot metadata consistency for SMAPIv1 to SMAPIv3 migration
  • Mirror polling optimization with cache
  • Orphan dummy VDI not clean up after SMAPIv3 compose in SMAPIv1->SMAPIv3 SXM
  • VDI type lost after snapshot revert on SMAPIv3 SR

Design refer: https://github.com/xapi-project/xen-api/blob/feature/sxm-v3/doc/content/xapi/storage/sxm/sxm-v3-with-snapshot.md

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>
…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>
@LunfanZhang

Copy link
Copy Markdown
Collaborator Author

This PR is not new work — it is the first slice of #7137, for saving your time, Nothing was added, removed or amended in the split.

@gthvn1

gthvn1 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

can you rebase on master to be able to build it? I still have:

File "ocaml/qcow-stream-tool/qcow_stream_tool.ml", line 17, characters 8-24:
17 |         data_cluster_map
             ^^^^^^^^^^^^^^^^

@LunfanZhang

Copy link
Copy Markdown
Collaborator Author

can you rebase on master to be able to build it? I still have:

File "ocaml/qcow-stream-tool/qcow_stream_tool.ml", line 17, characters 8-24:
17 |         data_cluster_map
             ^^^^^^^^^^^^^^^^

I once hit this issue, and I have sync the master to feature branch to resolve this, so I think you can take try to build on feature branch after it get merged.

@gthvn1

gthvn1 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

In fact I rebased it on master and I was able to build it. But there were minors conflicts when rebasing so that is why I asked to rebase it, to have something clean. But it is ok for me for reviewing no worries ;)

)
|> wrap

let vdi_clone_impl dbg sr vdi_info =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we also add the update_keys logic in "vdi_snapshot_impl" ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should set the keys in vdi_snapshot_impl, and it already set in current impl.

@gthvn1 gthvn1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the commit message for CA-428535 about catching the specific "Volume_does_not_exist" still accurate? I don't see it in the code.

…1->SMAPIv3 SXM

receive_finalize_common destroys the "dummy" VDI that receive_start_common
snapshotted for VDI.compose. On SMAPIv1 that drops the backend volume and
the dest DB row together; on SMAPIv3 Volume.compose has already consumed the
volume, so the destroy fails (swallowed by log_and_ignore_exn) and the row
survives as an orphan snapshot of the active disk, breaking any later SXM v3
migration of that disk.

Add forget_orphan_dest_vdi in storage_migrate_helper, called from
receive_finalize_common and receive_cancel2. It forgets the row through the
XenAPI session embedded in the SXM url - going via the dest session rather
than the local DB is what makes it correct cross-pool. Best-effort, only for
is_a_snapshot rows, and a no-op on SMAPIv1 where the row is already gone.

Signed-off-by: Lunfan Zhang[Lunfan.Zhang] <Lunfan.Zhang@cloud.com>
@LunfanZhang
LunfanZhang force-pushed the private/luzhan/sxm-v3-p1-blockers branch from 165ae40 to 3bad06b Compare August 12, 2026 02:05
@LunfanZhang

Copy link
Copy Markdown
Collaborator Author

is the commit message for CA-428535 about catching the specific "Volume_does_not_exist" still accurate? I don't see it in the code.

Yep, matching Volume_does_not_exist is a legacy way which I have dropped, updated to reflect current solution now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants