CA-390277: Reduce record usage on CLI cross-pool migrations #5773
Conversation
|
I've verified manually that it works with and without remote-host, and with and without a remote-network, especially using the name label. For the network, the bridge parameter is strange since it needs the reference, not its name (e.g. xenbr0). |
| remote Client.PBD.get_all_where ~expr | ||
| |> List.map (fun pbd -> | ||
| let sr = remote Client.PBD.get_SR ~self:pbd in | ||
| (sr, remote Client.SR.get_record ~self:sr) |
There was a problem hiding this comment.
so it is safe to use the SR record, but not the host and network record?
There was a problem hiding this comment.
It's not safe, but not using them means risking a large amount of roundtrips between both servers to order the SRs.
The internal tests should now try to find this issue continually and catch this issue in time
There was a problem hiding this comment.
So using SR record is unsafe, but there hasn't been problems using them. And we can fix them once there are changes causing them to become real problems, is that right?
There was a problem hiding this comment.
And we can fix them once there are changes causing them to become real problems, is that right?
We have 3 options here:
- Roll back breaking changes when the are found
- Create queries for sorting (mimicking
SORT BY, similar to the current where) and use that. This seems like reinventing SQL queries. - Make the client robust to adding fields (ignore them), and removing them (allowing client to fail if they needed it)
|
Looks good to me, as long as the use of SR record is ok.... |
| @@ -1374,9 +1377,9 @@ let message_destroy (_ : printer) rpc session_id params = | |||
| | None, Some uuids -> | |||
| String.split_on_char ',' uuids | |||
| | Some _, Some _ -> | |||
| fail "Ambiguous arguments; need one of uuid, uuids" | |||
| failwith "Ambiguous arguments; need one of uuid, uuids" | |||
There was a problem hiding this comment.
It's confusing that failwith is overridden in this module...
Making all the cases explicit avoids wrong behaviour when adding a new message Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This is useful to avoid fetching whole records in vm migrations with different versions. This is because records can contain new variants, which can fail to be serialized. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This reduces repetition, makes calls easier to read, and shorter. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Using records in cross-pool migration code is dangerous, as the code interacts with potentially newer hosts. This means that fields in the record might be different from what's expected. In particular adding an enum field can break the deserialization, and removing a field as well. With this change, only SR records are used. This is done to minimize the number of calls done. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Using records in cross-pool migration code is dangerous, as the code interacts with potentially newer hosts. This means that fields in the record might be different from what's expected. In particular, adding an enum field can break the deserialization, and removing a field as well.
Instead, introduce and use a new call
get_all_where: this calls allows the host to return a filtered list of object references.With these changes, only SR records are used. This is done to minimize the number of calls done.