[smapiv3 merge] Rewrite using rpclib

[gaborigloi]

I've moved the feature/REQ477/master branch to the base commit where I started from, so that all my changes are visible here.

---

This change is 

[gaborigloi]

These PRs have to be merged together:

• [smapiv3 merge] Add new attach interface from SMAPIv3 xcp-idl#229
• xapi-storage-plugins
• [smapiv3 merge] Rewrite using new xapi-storage bindings based on rpclib xapi-storage-script#70
• [smapiv3 merge] Rewrite using rpclib #85
• [smapiv3 merge] Implement & move to VDI.attach2 SMAPIv2 call, deprecate VDI.attach xen-api#3633
• [smapiv3 merge] Move to new VDI.attach2 SMAPIv2 call  xenopsd#519
• qemu-trad.pg

[gaborigloi]

This can be merged at any time, because currently it's not pinned to the target branch.

[gaborigloi]

We'll have to reset master to this branch, because they have diverged 😮

[edwintorok]

Reviewed 25 of 44 files at r1, 1 of 1 files at r2, 9 of 11 files at r3, 3 of 6 files at r4, 1 of 3 files at r5, 120 of 121 files at r6, 3 of 6 files at r7.
Review status: 162 of 167 files reviewed, 8 unresolved discussions (waiting on @gaborigloi)

---

update_gh_pages.sh, line 34 at r7 (raw file):

git clone --quiet --branch=slate https://${GH_TOKEN}@github.com/xapi-project/xapi-storage $DOCDIR > /dev/null 2>&1
rm -rf $DOCDIR/source/includes/*
_build/default/generator/src/main.exe gen_markdown --path=$DOCDIR/source/includes

BTW you can use jbuilder exec here and that'll keep working even if you use jbuild workspaces (the workspace usually puts things one directory level up).

---

generator/lib/common.ml, line 20 at r7 (raw file):

let task_id = Param.mk ~name:"task_id" Types.string

type uri = string [@@deriving rpcty] [@@doc

BTWS is it worth to keep using @@doc or can we just use a regular OCaml comment, will the PPX consider both to be the same?
Might be easier to write and format comments if you don't need to quote each line.

---

generator/lib/control.ml, line 10 at r7 (raw file):

  | Volume_does_not_exist of string (** The specified volume could not be found in the SR *)
  | Unimplemented of string (** The operation has not been implemented *)
  | Cancelled of string (** The operation has not been implemented *)

Docstring is wrong, it should say the 'operation has been canceled'

---

generator/lib/control.ml, line 289 at r7 (raw file):

  let length = Param.mk ~name:"length" ~description:
      ["The length of the extent for which changed blocks should be computed"]
      Types.int

BTW should we use int64 here too for consistency?

---

generator/lib/control.ml, line 295 at r7 (raw file):

       "have changed between [volume1] and [volume2] as a base64-encoded";
       "bitmap string"]
      (dbg @-> sr @-> key @-> key2 @-> offset @-> length @-> returning changed_blocks errors)

Why are the types different for volume1 and volume2, are they named parameters?

---

generator/lib/control.ml, line 307 at r7 (raw file):

type configuration = (string * string) list [@@deriving rpcty]
(** Plugin-specific configuration which describes where and;
    how to locate the storage repository. This may include; the

drop the ;

---

generator/lib/control.ml, line 308 at r7 (raw file):

(** Plugin-specific configuration which describes where and;
    how to locate the storage repository. This may include; the
    physical block device name, a remote NFS server and; path

is ; needed here?

---

generator/lib/data.ml, line 27 at r7 (raw file):

  path: string;
  (** Path to the system local block device. This is equivalent to the SMAPIv1 params. *)
  dummy: unit;

What is the dummy for?

---

Comments from Reviewable

[gaborigloi]

update_gh_pages.sh, line 34 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

BTW you can use jbuilder exec here and that'll keep working even if you use jbuild workspaces (the workspace usually puts things one directory level up).

Thanks, fixed!

---

Comments from Reviewable

[gaborigloi]

generator/lib/common.ml, line 20 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

BTWS is it worth to keep using @@doc or can we just use a regular OCaml comment, will the PPX consider both to be the same?
Might be easier to write and format comments if you don't need to quote each line.

Yes, we can use ocaml comments most of the time, but not always - I'm planning to investigate & convert these later.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 10 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

Docstring is wrong, it should say the 'operation has been canceled'

Well spotted! Fixed it.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 289 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

BTW should we use int64 here too for consistency?

I used int here because users are not supposed to list changed blocks for such a large extent length that would require an int64 - that would result in significant memory usage. (CA-290243)

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 295 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

Why are the types different for volume1 and volume2, are they named parameters?

Exactly, the params have to be named for the Python generation to work (they are put into json dictionaries), that's why we need two different params with two different names.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 307 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

drop the ;

Well spotted!

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 308 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

is ; needed here?

Well spotted!

---

Comments from Reviewable

[gaborigloi]

generator/lib/data.ml, line 27 at r7 (raw file):

Previously, edwintorok (Török Edwin) wrote…

What is the dummy for?

Good point, now we can drop this! There was a bug in rpclib which caused a fatal warning for structs with only one field.

---

Comments from Reviewable

[gaborigloi]

generator/lib/data.ml, line 27 at r7 (raw file):

Previously, gaborigloi wrote…

Good point, now we can drop this! There was a bug in rpclib which caused a fatal warning for structs with only one field.

See mirage/ocaml-rpc#102

---

Comments from Reviewable

[edwintorok]

Review status: 161 of 167 files reviewed, all discussions resolved (waiting on @edwintorok)

---

generator/lib/control.ml, line 289 at r7 (raw file):

Previously, gaborigloi wrote…

I used int here because users are not supposed to list changed blocks for such a large extent length that would require an int64 - that would result in significant memory usage. (https://issues.citrite.net/browse/CA-290243)

OK. In practice you only loose 1 bit on 64-bit architectures, so int should be fine, we do not run on 32-bit architectures anyway.

---

Comments from Reviewable

[edwintorok]

Reviewed 2 of 44 files at r1, 1 of 6 files at r7, 3 of 3 files at r8.
Review status: all files reviewed, 23 unresolved discussions (waiting on @gaborigloi)

---

generator/lib/control.ml, line 27 at r8 (raw file):

[@@deriving rpcty]

type sr = string [@@deriving rpcty]

Could we use type sr = private string (assuming ocaml-rpc copes with that). That should ensure that XAPI won't accidentally change or use this in any way as a string. Its only the compiler that will know that this is a string, but otherwise its an abstract type at ocaml level too.

---

generator/lib/control.ml, line 117 at r8 (raw file):

  (** Amount of space currently used on the backing storage (in bytes) *)

  uri : string list;

Didn't we drop URIs in favour of device-config? I'm a bit confused why its still here.

---

generator/lib/control.ml, line 132 at r8 (raw file):

type changed_blocks = {
  granularity: int;

What units does granularity use, is it bytes?

---

generator/lib/control.ml, line 133 at r8 (raw file):

type changed_blocks = {
  granularity: int;
  bitmap: string;

Could we define a base64 abstract Rpc type and conversion functions to/from string?
i.e. something like this but for base64:

let typ_of_ipaddr =                                                                                                                                                                     
  Rpc.Types.Abstract                                                                                                                                                                    
    { aname= "ipaddr"                                                                                                                                                                   
    ; test_data= [Ipaddr.of_string_exn "127.0.0.1"]                                                                                                                                     
    ; rpc_of= (fun t -> Rpc.String (Ipaddr.to_string t))                                                                                                                                
    ; of_rpc=                                                                                                                                                                           
        (function                                                                                                                                                                       
          | Rpc.String s -> (                                                                                                                                                           
            try Ok (Ipaddr.of_string_exn s)                                                                                                                                             
            with Ipaddr.Parse_error (msg, arg) -> R.error_msgf "%s: %s" msg arg                                                                                                         
            )                                                                                                                                                                           
          | r ->                                                                                                                                                                        
              R.error_msgf "typ_of_ipaddr: expected rpc string got %s"                                                                                                                  
                (Rpc.to_string r)) }  

---

generator/lib/control.ml, line 133 at r8 (raw file):

type changed_blocks = {
  granularity: int;
  bitmap: string;

What is the meaning of the bitmap? bit is set if block has changed?

---

generator/lib/control.ml, line 134 at r8 (raw file):

changed_blocks

Will this be a single struct, or a list of extents (list of changed_blocks chunks) like we have in sparse_dd?
Even for a ~2TiB disk this would be ~90MiB considering 4KiB block sizes. If we want to support >2TiB (e.g. 1PiB) we would need several hundreds of gigabytes of memory, so would be better if this was streamed.

---

generator/lib/control.ml, line 283 at r8 (raw file):

changed_blocks

It is slightly confusing to have both parameter types and methods declared at topmost level.
Would it make sense to nest the declaration of the offset type in the method that uses it, assuming its just one place?

---

generator/lib/control.ml, line 302 at r8 (raw file):

       description=["Operations which operate on volumes (also known as ";
                    "Virtual Disk Images)"];
       version=(1,0,0)}

We've already released API version 4, so this should be version 5. We've got plenty of integers, lets not worry about keeping them low :)

---

generator/lib/control.ml, line 420 at r8 (raw file):

       namespace = Some "SR";
       description=["Operations which act on Storage Repositories"];
       version=(1,0,0)}

Version 5 please, we already released version 4.

---

generator/lib/data.ml, line 179 at r8 (raw file):

   simultaneously.
      |}];
      version=(1,0,0);

Already released version 4, lets start from 5 here.

---

generator/lib/data.ml, line 217 at r8 (raw file):

blocklist

Where is blocklist defined?

---

generator/lib/data.ml, line 227 at r8 (raw file):

      "`nbd://root:pass@foo.com/path/to/disk` that must contain all necessary";
      "authentication tokens"]
      (dbg @-> uri_p @-> domain @-> remote @-> blocklist @-> returning operation error)

Will this use NBD over TLS? Would be good if we can ensure that this path is always encrypted.

---

generator/lib/data.ml, line 255 at r8 (raw file):

  let ls = declare "ls"
      ["[ls] returns a list of all current operations"]
      (dbg @-> returning operations error)

I would've expected the Task interface to be used here, any reason why it isn't?

---

generator/lib/data.ml, line 283 at r8 (raw file):

   `Data.mirror` API call.

4. Find the list of blocks to copy via the CBT API call. Note that if

Does this mean that all SMAPIv3 SRs must support CBT if they support SxM?

---

generator/lib/data.ml, line 297 at r8 (raw file):

7. Once the copy operation has succesfully completed the destination
   disk will be a perfect mirror of the source.

Would be good if there was another optional operation available here that took a snapshot and calculated a SHA256 (or some other) hash of the VDI, and then did the same on the destination after all (delta) copies have finished.
We could use this during a quicktest, or just in general in some "debug" mode and run a stress test.

---

generator/lib/plugin.ml, line 4 at r8 (raw file):

open Idl
open Common

What happend to the Unimplemented exception here? Or do we simply require all plugin methods to be implemented?

---

generator/lib/plugin.ml, line 28 at r8 (raw file):

      ["minimum required API version"]];

  features : string list [@doc

Could we use an enum here, or the "wire" format would not be compatible?

---

generator/lib/plugin.ml, line 34 at r8 (raw file):

      ["key/description pairs describing required device_config parameters"]];

  required_cluster_stack : string list [@doc

Same question here, would using an enum break the 'wire API'? Unfortunately Plugin is the one API we cannot break, because it is the one used to determine the version number of the plugin.

---

generator/lib/plugin.ml, line 80 at r8 (raw file):

          "must support the query interface or it will not be recognised as";
          "a storage plugin by xapi."];
       version=(1,0,0)}

1.0 is correct here, we can't ever change this API, see above.

---

generator/lib/plugin.ml, line 87 at r8 (raw file):

let interfaces = Codegen.Interfaces.create
      ~name:"plugin"
      ~title:"The Datapath plugin interface"

This seems to have been taken from the wrong place. This is Plugin.ml, so it should say something like the old one did

 Interfaces.name = "plugin";                                                                                                                                                       
      title = "The Plugin interface";                                                                                                                                                   
      description =                                                                                                                                                                     
        String.concat " " [                                                                                                                                                             
          "The xapi toolstack expects all plugins to support a basic query";                                                                                                            
          "interface.";                                                                                                                                                                 
        ]; 

---

generator/lib/task.ml, line 1 at r8 (raw file):

(* Tasks API *)

Is this the equivalent of the Task API from https://github.com/xapi-project/xcp-idl/blob/b062924ccc99713e9836e5c9b33b30734febce85/storage/storage_interface.ml#L168-L192

---

generator/lib/task.ml, line 14 at r8 (raw file):

type async_result_t =
  | UnitResult of unit
  | Volume of Control.volume

There is also a Mirror.id in the xcp-idl enum, is that needed here?

---

generator/lib/task.ml, line 33 at r8 (raw file):

  debug_info : string;
  ctime : float;
  state : state;

subtasks, debug_info and backtrace are missing compared to xcp-idl.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 27 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

Could we use type sr = private string (assuming ocaml-rpc copes with that). That should ensure that XAPI won't accidentally change or use this in any way as a string. Its only the compiler that will know that this is a string, but otherwise its an abstract type at ocaml level too.

No, sadly that didn't work, it confused the ppx somehow :/

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 117 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

Didn't we drop URIs in favour of device-config? I'm a bit confused why its still here.

These URIs are different, they are for accessing the data of the Volume via a datapath plugin, not for identifying or locating it. We use them in xapi-storage-script to select a datapath based on the URI scheme, in choose_datapath. But maybe the documentation could be improved to clarify that, I'm not sure how

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 132 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

What units does granularity use, is it bytes?

Yes, I think that makes the most sense, I'll document that

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 133 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

Could we define a base64 abstract Rpc type and conversion functions to/from string?
i.e. something like this but for base64:

let typ_of_ipaddr =                                                                                                                                                                     
  Rpc.Types.Abstract                                                                                                                                                                    
    { aname= "ipaddr"                                                                                                                                                                   
    ; test_data= [Ipaddr.of_string_exn "127.0.0.1"]                                                                                                                                     
    ; rpc_of= (fun t -> Rpc.String (Ipaddr.to_string t))                                                                                                                                
    ; of_rpc=                                                                                                                                                                           
        (function                                                                                                                                                                       
          | Rpc.String s -> (                                                                                                                                                           
            try Ok (Ipaddr.of_string_exn s)                                                                                                                                             
            with Ipaddr.Parse_error (msg, arg) -> R.error_msgf "%s: %s" msg arg                                                                                                         
            )                                                                                                                                                                           
          | r ->                                                                                                                                                                        
              R.error_msgf "typ_of_ipaddr: expected rpc string got %s"                                                                                                                  
                (Rpc.to_string r)) }  

I think we should leave it in base64 for now, because that is the format we return from the relevant XenAPI call, and in the current CBT implementation we pass it up from SM without conversion to/from base64.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 134 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

changed_blocks

Will this be a single struct, or a list of extents (list of changed_blocks chunks) like we have in sparse_dd?
Even for a ~2TiB disk this would be ~90MiB considering 4KiB block sizes. If we want to support >2TiB (e.g. 1PiB) we would need several hundreds of gigabytes of memory, so would be better if this was streamed.

The API is now chunked, we have to request changed blocks for a sub-area of the disk, and pass in the offset, and the length. This is to fix the issue you mention about excessive memory consumption etc.

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 133 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

What is the meaning of the bitmap? bit is set if block has changed?

Yes, I've updated the docs

---

Comments from Reviewable

[edwintorok]

Reviewed 1 of 1 files at r9.
Review status: all files reviewed, 21 unresolved discussions (waiting on @gaborigloi and @edwintorok)

---

generator/lib/control.ml, line 117 at r8 (raw file):

Previously, gaborigloi wrote…

These URIs are different, they are for accessing the data of the Volume via a datapath plugin, not for identifying or locating it. We use them in xapi-storage-script to select a datapath based on the URI scheme, in choose_datapath. But maybe the documentation could be improved to clarify that, I'm not sure how

Perhaps say that it can be used to 'by a datapath plugin for I/O' instead of just I/O

---

generator/lib/control.ml, line 121 at r8 (raw file):

      reference a local block device, a remote NFS share, iSCSI LUN or RBD
      volume. In cases where the data may be accessed over several
      protocols, he list should be sorted into descending order of

typo: /he list/the list/

---

generator/lib/control.ml, line 133 at r8 (raw file):

Previously, gaborigloi wrote…

I think we should leave it in base64 for now, because that is the format we return from the relevant XenAPI call, and in the current CBT implementation we pass it up from SM without conversion to/from base64.

can you define a type base64 = string [@@deriving ..., won't prevent mistakenly putting another string there but would make it clearer when looking at types that this is not a regular string.

---

generator/lib/control.ml, line 134 at r8 (raw file):

Previously, gaborigloi wrote…

The API is now chunked, we have to request changed blocks for a sub-area of the disk, and pass in the offset, and the length. This is to fix the issue you mention about excessive memory consumption etc.

Thanks, would it help to include in the reply the offset and length, so the struct is self-contained and unambiguously describes a changed block chunk?

---

Comments from Reviewable

[gaborigloi]

generator/lib/control.ml, line 283 at r8 (raw file):

Previously, edwintorok (Török Edwin) wrote…

changed_blocks

It is slightly confusing to have both parameter types and methods declared at topmost level.
Would it make sense to nest the declaration of the offset type in the method that uses it, assuming its just one place?

Good idea, I've done this for all affected params, I think for now they are not needed elsewhere.

---

Comments from Reviewable

[edwintorok]

It doesn't show up in reviewable or github. Did you push the branch, or is github lagging behind?

---

Review status: complete! all files reviewed, all discussions resolved

---

Comments from Reviewable

[gaborigloi]

I did push the branch 🤔

---

Comments from Reviewable

[gaborigloi]

I've cleaned up the commit history, now it's ready for merging.

---

Comments from Reviewable

[edwintorok]

---

Review status: complete! all files reviewed, all discussions resolved

---

Comments from Reviewable