CP-53314: Read and watch <domain>/data/service in xenstore to DB

[changlei-li]

The field VM_guest_metrics.services added previously (#6309) is used to store VM service information reported by guest tools.
Xapi does not use this data but just holds it for outside consumers.
This PR reads the xenstore path data/service and converts to [("service_name/key", "value")] pair list and then store to VM_guest_metrics.services.
The path to read is like:
/local/domain/<id>/data/service/<service_name>/<key> = <value>

[psafont]

.

[psafont]

There's a new module that seems security-related, but its usage is not obvious to me, it lacks dopcumentation, and is missing unit-tests. Please at least add documentation and intended usage so we can change the module to make usre there aren't any footguns lurking around. After that, unit-test will better document how touse it and highlight edge-cases.

[lindig]

This needs documentation. I'm approving this but expect this to be updated.

[lindig]

What is the list function doing?

[changlei-li]

list is defined in xapi_xenops.ml update_vm. It lists all subdirs and keys in one depth under the given dir. In its definition, the data source is a big string string map like [("a", ""); ("a/b", ""); ("a/b/k1", "v1"); ("a/b/k2", "v2"); ("a/c/k1", "v1"); ("a/c/k2", "v2")]. Then function list "a" will give all subdirs ["b", "c"]. It needs to be noticed that the data source is not exposed to the caller in xapi_guest_agent.ml

[lindig]

utop # list_subkeys ["aaa/bbb/c", "x"] "aaa/bb";;
- : string list = ["b"]

Is this correct?

[changlei-li]

It's incorrect. I believe the real implementation of list in xapi_xenops.ml update_vm has this problem, too. Normally, it's a internal function and the caller only pass the parameter as a intact dir, so the problem is not exposed. It deserves to rasie a ticket to fix it.

[lindig]

  let ls assoc path =
    let split = String.split_on_char '/' in
    let prefix = split path in
    let paths = List.map (fun (path, _) -> split path) assoc in
    let rec entry prefix path =
      match prefix, path with
      | [], x::_ -> Some x
      | x::xs, y::ys when x = y -> entry xs ys
      | _ -> None
    in
      List.filter_map (entry prefix) paths

[andyhhp]

xenstore is already a bottlekneck. Xapi watching these keys really means xenopsd watching it and more traffic over message switch, which has proved to be an amplification vector in the past.

How many new watches does this create?

[lindig]

I would not expect these values to be updated frequently over the lifetime of a VM - so would not expect an impact from watches. @last-genius

[changlei-li]

Yes. Only single digit number of service-install data are written in data/service by guest agent. So, the path will be only written once after VM starting and expected not changed later.

[lindig]

I am a bit puzzled by introducing a Limit module but only using it for part of the keys that a VM is using. If there is no unified mechanism this is easy to circumvent by using different keys that don't maintain a quota.

[changlei-li]

Xenopsd already has a quota limit mechanism for guest_agent when reading xenstore. Remove the amout, key_len, value_len limit in this PR. It's better to use the general quota limit mechanism to keep it simple.