CP-40357: Parse all lifecycled in the datamodel, avoid loading removed fields into the database #4853
Conversation
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
4 times, most recently
from
3b62b81
to
03765e6
Compare
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
4 times, most recently
from
3dc9ace
to
e420f6c
Compare
|
Now the database ignores removed fields when loading, the second time there are no messages about ignored fields. Now testing the json generation code |
|
With the docs fixed, this is ready to be reviewed. I recommend doing it commit by commit, and special attention should be given to |
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
3 times, most recently
from
1b2767d
to
84ea790
Compare
ocaml/idl/datamodel.ml
Outdated
|
|
||
| let all_lifecycles = | ||
| let parse_with ~name lifecycle = | ||
| try Lifecycle.from lifecycle |
There was a problem hiding this comment.
Should this use a result type instead, or do you want to preserve the location that raised the invalid message in stacktraces (if it is useful then it should be preserved and using exceptions here is better)
There was a problem hiding this comment.
This code is replaced when the lifecycle state is integrated into the datamodel, I don't think it's important enough to change it for the few commits it's used
| @@ -381,6 +381,32 @@ end = struct | |||
| else | |||
| cmp | |||
|
|
|||
| let rec list_dedup cmp = | |||
There was a problem hiding this comment.
I think we have an implementation of this elsewhere already (somewhere in stdext)?
Although there is List.sort_uniq would that help, or you need more control over which one to keep?
There was a problem hiding this comment.
I want to sort using a criteria then remove duplicates according to another, specific criteria. This means i need sorting and deduplication to be separate functions, which discards sort_uniq. Stdext has setify with is the same as dedup, but it hardcodes the comparison function to = which is not enough here.
There was a problem hiding this comment.
This mostly looks really good, and is a much needed update to control the evolution of the API. Before merging, we'll need to ensure that we test the upgrade case very well. We'll also need to assess the impact on the API bindings (SDK), as I suspect that the last commit goes a step too far in removing fields, where clients may need them to connect to older server versions.
| @@ -1380,8 +1396,7 @@ let copy_bios_strings = | |||
| ~allowed_roles:_R_VM_ADMIN () | |||
|
|
|||
| let set_protection_policy = | |||
| call ~name:"set_protection_policy" ~in_oss_since:None | |||
| ~in_product_since:rel_orlando | |||
| call ~name:"set_protection_policy" ~in_oss_since:None ~lifecycle:vmpp_removed | |||
There was a problem hiding this comment.
It said ~in_product_since:rel_orlando, but that wasn't actually true...
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
2 times, most recently
from
d851d42
to
9662286
Compare
| significative one and drop the rest *) | ||
| changes | ||
| |> List.sort (fun ((_, nam_a, _, _) as a) ((_, nam_b, _, _) as b) -> | ||
| let cmp = String.compare nam_a nam_b in |
There was a problem hiding this comment.
I am not sure I understand this logic: when the names are identical, call compare_changes, which again uses the complete structure. Is the purpose here to compare the two structures by element and controlling the order? I've used the code below in such a situation where I define an order by comparing elements in a particular order.
let (<?>) a b =
if a = 0 then b else a
let compare a b =
compare a.hour b.hour
<?> compare a.minutes b.minutes
<?> compare a.seconds b.seconds
<?> 0
There was a problem hiding this comment.
It orders the list of entity changes in a release by the name of the entity and the type of change (removed last, prototyped first). This means all changes regarding a field are all group together and ordered in reverse order, from last to first.
Once that is done it drops items with a name that's the same as the preceding one. This way only the last change per entity is kept. For example if in a release a field got deprecated and removed, only the removed change is kept.
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
from
9662286
to
391baa9
Compare
This allows to enforce lifecycles to all fields and messages and xapi to know it, especially when loading the database from a file. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
These we all removed without being deprecated. These would be all rejected by the lifecycle automaton. Only with the SSL legacy it's clear at which released the message got deprecated (as soon as it was introduced) For the rest I used an easily-recognisable message that happens before and at the same release as the removal. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This will block any unsafe transitions from happening in the API Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This allows the database library to be able to depend on the datamodel one, while the latter now depends only the schema library The database library needs to depend on the datamodel one to be able to know when a field is a prototype one Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
No functional changes, the changes try to reduce the complexity of reading the code and reuse code for brevity Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
The comment is misleading since it refers to behaviour that has long been forgotten Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This allows dropping values that were only prototyped, or deprecated from the database Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This is the default value and serves no purpose Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This enables build the state at compile time without ad-hoc processing as this is done in the datamodel itself. For example to block removed fields from database constructors when autogenerating their code. This information is encapsulated in Lifecycle.t which is a private record. This makes it easy to deconstruct while forces its construction using the function Lifecycle.from. This prevents inconsistent Lifecycles from being created. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Now the database schema and most of the code generation are filtering these. In the case where the documentation about removal is still needed, the old method is used, albeit with a new name for the avoidance of doubt. This change means that it's impossible to create objects with removed fields, introducing quite a bit of churn. The most salient issues here are: the host metrics drop the free memory, which was still being maintained and used for estimating evacuation time. This is now done using total memory consumed by VMs instead; the removal of metrics in VIFs and VBDs, which makes it impossible to delete these objects from the database; and the removal of vmpp-related fields. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This change affects the public documentation only. When a field is published and deprecated on the same release, only the the deprecation notice is shown. Similarly when a message is deprecated and removed in the same release, only the removal gets shown. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
psafont
force-pushed
the
private/paus/tears-in-the-rain
branch
from
391baa9
to
ad1e634
Compare
Now all the lifecycles in the datamodel are check by an automaton found in datamodel_types
This allows to enforce lifecycles to all fields and messages and xapi to
know it, especially when loading the database from a file, which will now avoid loading removed fields onto the in-memory database
Marking as draft because I want feedback on the lifecycle automation and the datamodel changes but I haven't tested the database changes