Remove Legacy API module #3490

mseri · 2018-03-06T16:57:32Z

I had an old experiment lying around, I've decided to resurrect it and polish it after a Slack discussion today. I've rewritten the commit history to reflect more it's evolution and avoid intermediate debugging commits.

I am going to test it on a private branch, this should not be merged until testing is done and we agree on what to do with the Bigbuffer story (look for Bigbuffer.to_string, it appears in a few places in the new code).

mseri · 2018-03-06T17:08:39Z

I think originally the issue with Bigbuffer was that the maximum size of 32bits ocaml strings was 16Mb. The size of strings on 64 bit systems is on the order of 144 Petabytes (output of Sys.max_string_length, thanks @lindig)

coveralls · 2018-03-07T12:00:28Z

Coverage increased (+0.3%) to 19.111% when pulling f8d1cd9 on mseri:no-legacy into 05def45 on xapi-project:master.

gaborigloi · 2018-03-08T15:24:54Z

Hope this will be merged soon as it will make my work easier ^^

mseri · 2018-03-08T15:27:45Z

I'm running BVT and BST right now, if there are no further failures, and if somebody has the patience to review it in the meanwhile, we could proceed

gaborigloi · 2018-03-08T15:47:37Z

Actually I only need the commits that remove the Legacy module and genOCaml.ml

gaborigloi · 2018-03-08T15:52:25Z

Figured out how to solve my problem, so it's not that urgent now :)

mseri · 2018-03-08T16:53:35Z

The others are necessary to remove the Legacy module :P

mseri · 2018-03-08T17:18:12Z

For reference:

~/src/xen-api master $ cat _build/default/ocaml/xapi-types/aPI.ml | wc -l
6862

~/src/xen-api no-legacy $ cat _build/default/ocaml/xapi-types/aPI.ml | wc -l
3144

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

This greatly simplifies the changes needed in import.ml Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Here we are performing an itermediate re-serialization to string. It will be possible to simplify that further by implementing an Xmlrpc.of_xml method in ocaml-rpc. Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

This removes the necessity to pass trough an intermediate re-stringification of the various datastructures. There is still an open question about the need of Bigbuffer. If we can cope with strings as done now, we could actually get rif of Bigbuffer in all the rpc interaction, and maybe in xapi itself. If it is needed it should not be hard to provide the necessary infrastructure in ocaml-rpc to deal with it. We could also probably rely on Xmlrpc.from_a although I am a bit wary of that call... Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Now (64bit) that strings can hold 144 Petabytes of data, this module is no longer necessary. Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

…stead Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

…tive Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

… code that break the unmarshaller Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

… API generator Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

gaborigloi · 2018-03-09T10:02:23Z

ocaml/idl/ocaml_backend/gen_api.ml

-        sprintf "%s = %s_of_rpc (List.assoc \"%s\" x)" (field fld) (OU.alias_of_ty fld.DT.ty)
-          (String.concat "_" fld.DT.full_name)
+        sprintf {|%s = %s_of_rpc (assocer "%s" x %s)|} (field fld) (OU.alias_of_ty fld.DT.ty)
+          (String.concat "_" fld.DT.full_name) (get_default fld)


It is not necessary to generate these X_of_rpc, and rpc_of_X things, rpclib can do it for us. I have a set of working patches, one of which removes these custom functions and moves to ones generated by rpclib:
gaborigloi@2c4fc73

Not in this case, we need to customise them and replace the autogenerated ones.

Maybe we can use the new [@default ...] key to resolve the main issues, but it needs more work to serialize it correctly and it needs further testing. It is an improvement that can be added later and we should keep in mind to try it

gaborigloi · 2018-03-09T10:45:06Z

ocaml/idl/ocaml_backend/gen_api.ml

+        in
+        match default_value with
+          None -> ""
+        | Some default -> sprintf "[@default (%s)]" (Datamodel_values.to_ocaml_string ~v2:true default)


Jon said this is okay

Unfortunately it doesn't but we have a plan to fix ocaml-rpc and make it possible

mseri · 2018-03-09T13:42:51Z

As it is now, this code passed both the BVT and the BST (jobs 80812 and 80813) 🎉 😄

mseri · 2018-03-09T13:44:09Z

If it is fine for you, I'd suggest that we review and proceed with the current changes, and we keep working to improve them to use only ocaml-rpc instead of generating the modules by hand. In the meanwhile ths would be further tested

gaborigloi

Nice! Just some minor comments - it's not important to change the code, good to merge IMO

gaborigloi · 2018-03-09T16:26:22Z

ocaml/database/db_interface.ml

-type response =
-  | String of string
-  | Bigbuf of Xapi_stdext_bigbuffer.Bigbuffer.t
+type response = String of string


Would it be worth to just say type response = string?

Probably it is fine. I wanted to minimize the changes. I’ll have a look

It's fine. Done

gaborigloi · 2018-03-09T17:29:31Z

ocaml/xapi/import.ml

-  let xml_string = Bigbuffer.make () in
-  really_read_bigbuffer fd xml_string hdr.Tar_unix.Header.file_size;
-  Xml.parse_bigbuffer xml_string
+  Unixext.really_read_string fd (Int64.to_int hdr.Tar_unix.Header.file_size)


This could be renamed to read_next_file, or we could add a Xmlrpc.of_string to the end.

There was a reason I did not add the Xmlrpc.to_string but I think that woth the latest changes that reason is no longer there. I’ll fix it or let you know the reason :)

Found it. The old Xva module uses a direct approach manipulating and marshalling/unmarshalling the xml. The read_xml function is used both by our new Xmlrpc and that old Xml.parse_String |> Xva.from_xml. Changing that is invasive and out of scope for this pr

gaborigloi · 2018-03-09T17:35:32Z

ocaml/xapi/importexport.ml

+    ; xapi_vsn_minor  = Rpc.int_of_rpc (find _xapi_minor)
+    ; export_vsn      = try Rpc.int_of_rpc (find _export_vsn) with _ -> 0
+    }
+  | rpc -> raise (Failure(Printf.sprintf "version_of_rpc: malformed RPC %s" (Jsonrpc.to_string rpc)))


This is a minor point, but I'd use Rpc.to_string here, because we're dealing with a generic Rpc structure here.

Faor enough. I uesd json only because I find it more readable but it is more natural to use Rpc.to_string. I’ll change it

gaborigloi · 2018-03-09T17:41:06Z

ocaml/xapi/importexport.ml

+                 |> Xmlrpc.of_string
+                 |> API.ref_VM_set_of_rpc
+               with
+                 parse_error -> raise Api_errors.(Server_error (field_type_error, [Printexc.to_string parse_error]))


This is a very minor point again, but the param of this error seems to the field:

91 error Api_errors.field_type_error ["field"] 92 ~doc:"The value specified is of the wrong type" ();

The error is bogus. Before we were not even adding the field, and passing an empty string instead.

Using the printexc I am printing the real reason we cannot unmarshal the type. It is still a field type error, but with a useful string in the metadata. I think that even if is not what announced the errors with that content are way more readable and understandable. I agree that it is anstretch, but I will keep them as they are

gaborigloi · 2018-03-09T17:41:20Z

ocaml/xapi/xapi_vm_clone.ml

+    |> Xmlrpc.of_string
+    |> API.ref_VDI_of_rpc
+  with
+    parse_error -> raise Api_errors.(Server_error (field_type_error, [Printexc.to_string parse_error]))


Same as above.

mseri · 2018-03-09T22:19:43Z

Thanks for the review. I think it’s worth making some of the changes before merging

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

This gets rid of a leftover of 0583a91 Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

jonludlam · 2018-03-13T07:44:22Z

This looks really good!

robhoes · 2018-03-13T09:57:52Z

Nice cleanup: +202 −477 👍

mseri requested a review from jonludlam March 6, 2018 16:57

Marcello Seri added 21 commits March 8, 2018 17:25

gen_api: remove Legacy module

526ee06

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

graph: remove unnecessary use of the API.Legacy module

124be4c

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

xapi_vm_clone: remove unnecessary Legacy module

52f8b79

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

xapi_vm_clone: improve error reporting

4ac4085

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

importexport: remove unused function

036837e

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

xapi_vm_clone: read the xml as an rpc and not as a response

f9221ba

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

importexport: replace use of Legacy module

2cf16b6

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

importexport: restore some of the old behaviour

32d1bdf

This greatly simplifies the changes needed in import.ml Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

import: don't use the Legacy module

ae211ce

Here we are performing an itermediate re-serialization to string. It will be possible to simplify that further by implementing an Xmlrpc.of_xml method in ocaml-rpc. Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

cli_operations: remove unnecessary use of Legacy module

14fd690

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

export: get rid of Legacy module

995d73c

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

import: further reduce and localise the use of Bigbuffer.to_string

f91a014

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

xapi, xapi-database: remove use of Xapi_stdext_bigbuffer

e5b9bb4

Now (64bit) that strings can hold 144 Petabytes of data, this module is no longer necessary. Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

datamodel_values: to_rpc, function parity to to_xml

147b67d

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

gen_api: add default values to rpc unmarshal

e3d6bf6

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

genOCaml: delete unused module

4d849fd

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

datamodel_values: remove unused to_xml

2abe925

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

idl: revert datamodel_values to_rpc change and use to_ocaml_string in…

c65c1c6

…stead Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

datamodel_values: correctly stringify numbers also when they are nega…

06c75bc

…tive Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

gen_api: prevent default values for VCustom fields, these may contain…

a106d54

… code that break the unmarshaller Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Marcello Seri added 2 commits March 8, 2018 17:25

gen_api, datamodel_values: get defaults for VCustoms suitable for the…

7650afe

… API generator Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

Further unused files cleanup

2fcf58c

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

gaborigloi reviewed Mar 9, 2018

View reviewed changes

gaborigloi approved these changes Mar 9, 2018

View reviewed changes

Marcello Seri added 2 commits March 12, 2018 10:04

database: simplify types after removing bigbuffer

24938e3

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

importexport: use Rpc.to_string instead of Jsonrpc.to_string

f27a01d

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

gaborigloi approved these changes Mar 12, 2018

View reviewed changes

mseri mentioned this pull request Mar 12, 2018

Implement SR.probe_ext #3502

Merged

Marcello Seri added 2 commits March 12, 2018 16:47

database: resurrect db_cache_test

9ec2685

Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

db_rpc_common_v2: completely remove Read_set_ref

f8d1cd9

This gets rid of a leftover of 0583a91 Signed-off-by: Marcello Seri <marcello.seri@citrix.com>

jonludlam merged commit e14d893 into xapi-project:master Mar 13, 2018

mseri deleted the no-legacy branch March 13, 2018 09:30

Remove Legacy API module #3490

Remove Legacy API module #3490

Uh oh!

Conversation

mseri commented Mar 6, 2018

Uh oh!

mseri commented Mar 6, 2018

Uh oh!

coveralls commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborigloi commented Mar 8, 2018

Uh oh!

mseri commented Mar 8, 2018

Uh oh!

gaborigloi commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborigloi commented Mar 8, 2018

Uh oh!

mseri commented Mar 8, 2018

Uh oh!

mseri commented Mar 8, 2018

Uh oh!

gaborigloi Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mseri commented Mar 9, 2018

Uh oh!

gaborigloi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri commented Mar 9, 2018

Uh oh!

jonludlam commented Mar 13, 2018

Uh oh!

robhoes commented Mar 13, 2018

Uh oh!

coveralls commented Mar 7, 2018 •

edited

Loading

gaborigloi commented Mar 8, 2018 •

edited

Loading

gaborigloi Mar 9, 2018 •

edited

Loading

mseri Mar 9, 2018 •

edited

Loading

mseri commented Mar 9, 2018 •

edited

Loading

mseri Mar 9, 2018 •

edited

Loading

mseri Mar 9, 2018 •

edited

Loading