fixup! doc: update better VM revert proposal

Author: psafont

doc/content/design/snapshot-revert.md

@@ -8,9 +8,10 @@ status: confirmed
 
 ## Overview
 
-Currently there is a XenAPI `VM.revert` which reverts a VM to the state it was
-in when a VM-level snapshot was taken. Because there is no `VDI.revert`,
-`VM.revert` uses `VDI.clone` on the snapshot to change the state of the disks.
+XenAPI allows users to rollback the state of a VM to a previous state, which is
+stored in a snapshot, using the call `VM.revert`. Because there is no
+`VDI.revert` call, `VM.revert` uses `VDI.clone` on the snapshot to duplicate
+the contents of that disk and then use the new clone as the storage for the VM.
 
 Because `VDI.clone` creates new VDI refs and uuids, some problematic
 behaviours arise:
@@ -20,21 +21,44 @@ behaviours arise:
   logic to keep track of the disks they are actively managing
 - Because the snapshot is cloned and the original vdi is deleted, VDI
   references to the VDI become invalid, like `VDI.snapshot_of`. This means
-  there the database has to be combed through to change these references. 
-  Because the database doesn't support transaction this operation is not atomic
+  that the database has to be combed through to change these references. 
+  Because the database doesn't support transactions this operation is not atomic
   and can produce inconsistent database states.
 
+Additionally, some filesystems support snapshots natively, doing the clone
+procedure is much costlier than allowing the filesystem to do the revert.
+
 We will fix these problems by:
 
-- defining a new storage operation `VDI.revert` in storage_interface and
-  calling this from `VM.revert`
+- introducing the new feature `VDI_REVERT` in SM interface (`xapi_smint`). This
+  allows backends to advertise that they support the new functionality
+- defining a new storage operation `VDI.revert` in storage_interface, which is
+  gated by the feature `VDI_REVERT`
 - proxying the storage operation to SMAPIv3 and SMAPv1 backends accordingly
-- changing the Xapi implementation of `VM.revert` to first try the
-  `VDI.revert`, and fall back to `VDI.clone` if that fails
+- adding `VDI.revert` to xapi_vdi which will call the storage operation if the
+  backend advertises it, and fallback to the previous method that uses
+  `VDI.clone` if it doesn't advertise it, or issues are detected at runtime
+  that prevent it
+- changing the Xapi implementation of `VM.revert` to use `VDI.revert`
 - implement `vdi_revert` for common storage types, including File and LVM-based
   SRs
 - adding unit and quick tests to xapi to test that `VM.revert` does not regress
 
+## Current VM.revert behaviour
+
+The code that reverts the state of storage is located in 
+[update_vifs_vbds_vgpus_and_vusbs](https://github.com/xapi-project/xen-api/blob/bc0ba4e9dc8dc4b85b7cbdbf3e0ba5915b4ad76d/ocaml/xapi/xapi_vm_snapshot.ml#L211).
+The steps it does is:
+1. destroys the VM's VBDs (both disks and CDs)
+2. destroys the VM's VDI (disks only), referenced by the snapshot's VDIs using
+   `snapshot_of`; as well as the suspend VDI.
+3. clones the snapshot's VDIs (disks and CDs), if one clone fails none remain.
+4. searches the database for all `snapshot_of` references to the deleted VDIs
+   and replaces them with the referenced of the newly cloned snapshots.
+5. clones the snapshot's resume VDI
+6. creates copies of all the cloned VBDs and associates them with the cloned VDIs
+7. assigns the new resume VDI to the VM
+
 ## XenAPI design
 
 ### API
@@ -43,16 +67,17 @@ The function `VDI.revert` will be added, with arguments:
 
 - in: `snapshot: Ref(VDI)`: the snapshot to which we want to revert
 - in: `driver_params: Map(String,String)`: optional extra parameters
-- out: `Ref(VDI)` the new VDI
-
-The function will look up the VDI which this is a `snapshot_of`, check that
-the value is not invalid nor null, and use that VDI reference to call SM to
-change the VDI to have the same contents as the snapshot. The snapshot object
-will not be modified, and the reference returned is the reference in
-`snapshot_of`.
-If anything impedes the successful finish of an in-place revert, the previous
-method of using `VDI.clone` is used as fallback, and the reference returned
-refers to the fresh VDI created by the `VDI.clone` fallback.
+- out: `Ref(VDI)` reference to the new VDI with the reverted contents
+
+The function will extract the reference of VDI whose contents need to be
+replaced. This is the snapshot's `snapshot_of` field, then it will call the
+storage function function `VDI.revert` to have its contents replaced with the
+snapshot's. The VDI object will not be modified, and the reference returned is
+the VDI's original reference.
+If anything impedes the successful finish of an in-place revert, like the SM
+backend does not advertising the feature `VDI_REVERT`, not implement the
+feature, or the `snapshot_of` reference is invalid; an exception will be
+raised.
 
 ### Xapi Storage
 
@@ -62,7 +87,6 @@ The function `VDI.revert` is added, with the following arguments:
 - in: `sr`: SR where the new VDI must be created
 - in: `snapshot_info`: metadata of the snapshot, the contents of which must be
        made available in the VDI indicated by the `snapshot_of` field
-- out: `vdi_info`: metadata of the resulting VDI
 
 #### SMAPIv1
 
@@ -87,14 +111,16 @@ following arguments:
 - in: `sr`: the UUID of the SR containing both the VDI and the snapshot
 - in: `snapshot`: the UUID of the snapshot whose contents must be duplicated
 - in: `vdi`: the UUID of the VDI whose contents must be replaced
-- 
+
 ### Xapi
 
-- use `VDI.revert` in the `VM.revert` code-path and fall back to `VDI.clone` if
-  a `Not_implemented` exception is thrown, or the snapshot_of contains an
-  invalid reference
+- add the capability `VDI_REVERT` so backends can advertise it
+- use `VDI.revert` in the `VM.revert` after the VDIs have been destroyed, and
+  before the snapshot's VDIs have been cloned. If any of the reverts fail
+  because a `Not_implemented` exception is thrown, or the `snapshot_of`
+  contains an invalid reference, add the affected VDIs to the list to be cloned
+  and recovered, using the existing method
 - expose a new `xe vdi-revert` CLI command
-- implement the `VDI.revert` 
 
 ## SM changes