Multi-Version Drivers #6163

lindig · 2024-12-06T14:36:30Z

MVD CP-52334 multi-version driver API/CLI

Provide an API/CLI for multi-version drivers. Two new classes represent
them: Host_driver and Driver_variant. See

  * doc/content/toolstack/features/MVD/index.md

for technical background and feature overview.

The current implementation mocks the missing drivertool that manipulates
the file system and observes driver state. We expect to integrate this
at a later point. Consequently, the current implementation is incomplete
and exists to faciliate development of API and XE clients.

On Xapi start and restart driver information is purged from the
database and re-built from scrach. This could be improved in the future
to be incremental.

quality-gate.sh

Vincent-lau · 2024-12-10T12:18:39Z

I understand this is a draft version. But would be easier to split it into smaller commits for review, or is that not very feasible?

lindig · 2024-12-10T13:23:31Z

I worked from the original work by @last-genius but ended up having so many changes that the commit history was not meaningful. In addition, re-basing made a mess of them. In the end, this is difficult to split up - it's almost impossible to split this up such that each commit still compiles because it references each other. I could split it by module or something like it - but that is not entirely logical either.

lindig · 2024-12-12T09:21:59Z

I have additional code to make the database update incremental on scan() but would like to see the basics reviewed first.

Vincent-lau · 2024-12-12T16:27:40Z

doc/content/toolstack/features/MVD/index.md

+
+## Variant vs. Version
+
+A driver comes in several variants, each of which has a version. A


When is a driver variant useful?

It can be useful when you want to update (backport) a driver (e.g for newer hardware or some risky fixes) without the risk of introducing regressions.

You mean updating a driver to a variant, but that update itself is safe? How does it achieve that safety?

ocaml/idl/datamodel_driver_variant.ml

Vincent-lau · 2024-12-12T17:06:17Z

ocaml/idl/datamodel_host_driver.ml

+  call ~name:"deselect" ~in_oss_since:None ~lifecycle:[]
+    ~doc:
+      "UNSUPPORTED. Deselect the currently active variant of this driver after \
+       reboot. No action will be taken if no variant is currently active."


So selected and active are independent operations, as mentioned in the design. What happens when there is no active driver but there is a selected driver? Will it be deselected?

A selected driver will become active on next reboot.

I guess my confusion comes from this statement "Deselect the currently active variant of this driver", which says if the current driver is active (i.e. in use, but will not be loaded on next boot), calling deselect on it will make it deselected? I was expecting something like "deselect the currently selected variant of this driver", so that it will not be loaded on next boot.

Maybe the wording could be improved. What happens is that the currently active driver/variant combination will not become active again on reboot. Technically a symlink is removed such that this variant is no longer loaded by the kernel.

Vincent-lau · 2024-12-12T17:08:53Z

ocaml/xapi/db_gc_util.ml

@@ -581,17 +602,6 @@ let gc_PVS_cache_storage ~__context =
    (fun x -> valid_ref __context x.pVS_cache_storage_host)
    Db.PVS_cache_storage.destroy

-(*


Remove these?

ocaml/xapi/xapi_host_driver.ml

ocaml/xapi-cli-server/cli_operations.ml

Vincent-lau · 2024-12-12T17:18:32Z

ocaml/xapi/xapi_host_driver.ml

+
+(** destroy driver and recursively its variants, too *)
+let destroy ~__context ~self =
+  D.debug "Destroying driver %s" (Ref.string_of self) ;


The %s __FUNCTION__ can be added here as well.

robhoes

This is a very long single commit... I would normally split out at least the datamodel definition, internal logic, tests and mock tool into separate commits.

doc/content/toolstack/features/MVD/index.md

ocaml/idl/datamodel_driver_variant.ml

robhoes · 2024-12-17T10:43:43Z

ocaml/idl/datamodel_driver_variant.ml

+          "Unique versions of this driver variant"
+      ; field ~lifecycle:[] ~qualifier:DynamicRO ~ty:Bool "hardware_present"
+          "True if the hardware for this variant is present on the host"
+      ; field ~lifecycle:[] ~qualifier:DynamicRO ~ty:Float "priority"


This is not mentioned in the markdown doc.

True; the role of priority is not clear at the moment; we are not actively using it except to order presentation. The value is reported by the lower level.

ocaml/idl/datamodel_driver_variant.ml

ocaml/xapi/message_forwarding.ml

ocaml/xapi/xapi_host_driver.ml

robhoes · 2024-12-17T18:08:40Z

ocaml/xapi/xapi_host_driver.ml

+      D.debug "%s selecting driver %s variant %s" __FUNCTION__ drv var ;
+      let d = Db.Host_driver.get_record ~__context ~self in
+      let v = Db.Driver_variant.get_record ~__context ~self:variant in
+      let stdout =
+        drivertool ["select"; d.API.host_driver_name; v.API.driver_variant_name]
+      in
+      info "%s: %s" __FUNCTION__ stdout ;
+      Db.Host_driver.set_selected_variant ~__context ~self ~value:variant


Looks like a copy of Variant.select, except you have the driver ref already. But I'm not sure we actually need Driver_variant.select in the API at all.

One uses the name of the variant, the other the ref. Both may be the most convenient for an API client; I would make the decision to remove this later as the feature progresses.

So both for select and rescan have redundant versions in the datamodel now. I prefer to keep the API simple and have just one way of doing something, and keep just one of each kind. But fine if you want to decide later which one is best.

ocaml/xapi/xapi_host_driver.ml

stormi · 2024-12-18T17:51:53Z

doc/content/toolstack/features/MVD/index.md

+
+The format of the description is vendor specific and is used for
+a null-terminated string holding the version. The name is fixed to
+"XenServer". The exact format is described in [ELF notes].


Is this configurable? The spec says the identifier is vendor-specific, so can another vendor provide .note.$VENDOR instead and tell XAPI that it's where it should look for the extra driver information?

Alternatively, if not configurable, would Xapi be a better fit as an identifier for such a feature of the XAPI Project?

All of this is quite far out and we are trying to get this off the ground. It's too early to encode this kind of flexibility where we are far away from any end-to-end testing.

I understand regarding the configurability. However I tried to suggest changing the hardcoded names to Xapi (paths, ELF notes...), which seemed a good and neutral identifier for downstreams, in the previous draft iteration of this PR, but without an answer at the time, that's why I'm asking again now. This would likely avert the need for configurable paths and identifiers.

I'll bring it up in a discussion. I understand the desire to keep XenServer out of this.

robhoes · 2024-12-19T15:49:24Z

The diff for ocaml/idl/datamodel_lifecycle.ml should be removed and regenerated as versions have moved on.
Some of my comments have not yet been addressed (all simple fixes).

Provide an API/CLI for multi-version drivers. Two new classes represent them: Host_driver and Driver_variant. See * doc/content/toolstack/features/MVD/index.md for technical background and feature overview. The current implementation mocks the missing drivertool that manipulates the file system and observes driver state. We expect to integrate this at a later point. Consequently, the current implementation is incomplete and exists to faciliate development of API and XE clients. On Xapi start and restart driver information is purged from the database and re-built from scrach. This could be improved in the future to be incremental. Signed-off-by: Christian Lindig <[email protected]>

When scanning a host for drivers, don't remove and re-create them but instead update an entry if it already exists. Signed-off-by: Christian Lindig <[email protected]> Signed-off-by: Christian Lindig <[email protected]>

Address review comments Signed-off-by: Christian Lindig <[email protected]>

Signed-off-by: Christian Lindig <[email protected]>

github-advanced-security bot found potential problems Dec 6, 2024

View reviewed changes

quality-gate.sh Fixed Show fixed Hide fixed

lindig marked this pull request as ready for review December 9, 2024 13:18

lindig requested review from edwintorok, last-genius and robhoes December 9, 2024 13:19

lindig force-pushed the private/christianlin/multi-driver-v2 branch 2 times, most recently from 6d992d8 to f0db7d8 Compare December 9, 2024 16:16

Vincent-lau reviewed Dec 12, 2024

View reviewed changes

robhoes reviewed Dec 17, 2024

View reviewed changes

lindig force-pushed the private/christianlin/multi-driver-v2 branch from f0db7d8 to 2a1a1d1 Compare December 18, 2024 13:17

stormi reviewed Dec 18, 2024

View reviewed changes

lindig force-pushed the private/christianlin/multi-driver-v2 branch from 78b7ff3 to fa3b4ae Compare December 20, 2024 10:44

Christian Lindig added 5 commits December 20, 2024 10:47

MVD CP-52334 multi-version driver API/CLI - incremental update

a81f1b0

When scanning a host for drivers, don't remove and re-create them but instead update an entry if it already exists. Signed-off-by: Christian Lindig <[email protected]> Signed-off-by: Christian Lindig <[email protected]>

MVD CP-52334 multi-version driver API/CLI - small changes

ad08598

Address review comments Signed-off-by: Christian Lindig <[email protected]>

MVD CP-52334 Update schema hash

40939e0

Signed-off-by: Christian Lindig <[email protected]>

MVD CP-52334 multi-version driver API/CLI - incremental update

f1d6fe8

Signed-off-by: Christian Lindig <[email protected]>

MVD CP-52334 Update datamodel lifecycle

0e02275

Signed-off-by: Christian Lindig <[email protected]>

lindig force-pushed the private/christianlin/multi-driver-v2 branch from fa3b4ae to 0e02275 Compare December 20, 2024 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-Version Drivers #6163

Multi-Version Drivers #6163

lindig commented Dec 6, 2024

Vincent-lau commented Dec 10, 2024

lindig commented Dec 10, 2024

lindig commented Dec 12, 2024

Vincent-lau Dec 12, 2024

TSnake41 Dec 18, 2024

Vincent-lau Dec 19, 2024

Vincent-lau Dec 12, 2024

lindig Dec 18, 2024

Vincent-lau Dec 19, 2024 •

edited

Loading

lindig Dec 19, 2024

Vincent-lau Dec 12, 2024

Vincent-lau Dec 12, 2024

robhoes left a comment

robhoes Dec 17, 2024

lindig Dec 18, 2024

robhoes Dec 17, 2024

lindig Dec 18, 2024

robhoes Dec 19, 2024

stormi Dec 18, 2024

lindig Dec 19, 2024

stormi Dec 19, 2024

lindig Dec 19, 2024

robhoes commented Dec 19, 2024


		## Variant vs. Version

		A driver comes in several variants, each of which has a version. A

Multi-Version Drivers #6163

Are you sure you want to change the base?

Multi-Version Drivers #6163

Conversation

lindig commented Dec 6, 2024

Vincent-lau commented Dec 10, 2024

lindig commented Dec 10, 2024

lindig commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincent-lau Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhoes commented Dec 19, 2024

Vincent-lau Dec 19, 2024 •

edited

Loading