CA-400860: rrdp-netdev - drop xenctrl, use xenstore to get UUIDs from domids instead #6068
Conversation
| (Printf.sprintf "Failed to find uuid corresponding to domid: %d" domid) | ||
| in | ||
| uuid | ||
| let uuid_of_domid domid = |
There was a problem hiding this comment.
Are there any other plugins that use this pattern? They could share code in the plugin library that you added
There was a problem hiding this comment.
xenops_helpers has the same function
There was a problem hiding this comment.
yes, it's where i stole the function from. but xenops_helpers also converts strings to UUIDs, which I don't need.
| xs.Xenstore.Xs.read (vm_dir ^ "/uuid") | ||
| ) | ||
| with _ -> | ||
| failwith |
There was a problem hiding this comment.
let fail = Printf.ksprintf failwith (* somewhere at the top of the file *)
..
fail "Something %s" "a string"
Might want to log the exception to understand why this fails. I would use Filename.concat to construct the paths but it's a aesthetic choice.
There was a problem hiding this comment.
This should be Xenstore.Path.concat or similar, since they are not filenames
… domids instead Signed-off-by: Andrii Sultanov <[email protected]>
last-genius
force-pushed
the
private/asultanov/rrdd-xenctrl-fix
branch
from
537904f to
1bdb22a
Compare
| Xenstore.with_xs (fun xs -> | ||
| let vm = xs.Xenstore.Xs.getdomainpath domid ^ "/vm" in | ||
| let vm_dir = xs.Xenstore.Xs.read vm in | ||
| xs.Xenstore.Xs.read (vm_dir ^ "/uuid") |
There was a problem hiding this comment.
I know I'm late to the party here, but this is very expensive. You've got 3 xenstore accesses to do what was previously a single hypercall (would be faster than a single xenstore access), and xenstore is a known bottleneck.
First, getdomainpath $x is universally /local/domain/$x. Some original plans for xenstore involved having /remote/$host/domain/$domid as shared-virtual-memory across the pool. Luckily, this plan never even got started, and you can reasonably hard-code it.
Second, /vm/$uuid/uuid is always self-referential, so you could just split the answer from /local/domain/$domid/vm at / and take the final component.
Or, if you want something less dodgy, just have Xapi/Xenops/whatever fill in a new top-level dir with /uuid-by-domid/$domid = $uuid so logic like this can issue a single read for /uuid-by-domid/$domid and get the precise data you want.
Bake in assumptions that have been constant ever since xenstore was created: getdomainpath always returns "/local/domain/<domid>", /local/domain/domid/vm returns "/vm/<uuid>", so there's no need to look up that path to get the uuid again This reduces the number of xenstore accesses from 3 to 1 with no functional change. As suggested in: xapi-project#6068 (review) Signed-off-by: Andrii Sultanov <[email protected]>
Bake in assumptions that have been constant ever since xenstore was created: getdomainpath always returns "/local/domain/<domid>", /local/domain/domid/vm returns "/vm/<uuid>", so there's no need to look up that path to get the uuid again This reduces the number of xenstore accesses from 3 to 1 with no functional change. As suggested in: xapi-project#6068 (review) Signed-off-by: Andrii Sultanov <[email protected]>
Bake in assumptions that have been constant ever since xenstore was created: getdomainpath always returns "/local/domain/domid", /local/domain/domid/vm returns "/vm/uuid", so there's no need to look up that path to get the uuid again This reduces the number of xenstore accesses from 3 to 1 with no functional change. As suggested in: #6068 (review)
Follow-up to the fix we had to rush, dropping xenctrl entirely.
Tested with a Windows VM, the domain creation is picked up and network metrics are present for it, with correct UUIDs determined.