Support livepatch - part 1 #4678

minglumlu · 2022-04-11T08:53:14Z

This series contains the first part of changes to support live patch:

some existing codes are split into new files;
new functions to parse metadata of live patches from updateinfo;
get the status of running live patch (Xen and Kernel) on host;
get all applicable live patches from updateinfo;
calculate and consolidate guidance.

lindig · 2022-04-11T09:05:34Z

ocaml/xapi/livepatch.ml

+type component = Xen | Kernel
+
+let component_of_string = function
+  | "Xen" | "xen" ->


It would be better to use String.lowercase_ascii before the match is made.

ocaml/xapi/livepatch.ml

lindig · 2022-04-11T09:09:24Z

ocaml/xapi/livepatch.ml

+       * livepatch_4_19_19__8_0_20__4_19_19__8_0_22
+       *)
+      Re.Posix.compile_pat
+        {|^[ ]*livepatch_([^ \[]+)__([^ \[]+)__([^ \[]+)__([^ \[]+).*$|}


Can we make this regex stronger? ([0-9_]+) seems to capture the expected version strings.

The version and release strings may contain other characters. Actually we don't have any expectations on the strings. Here we only need to separate them.

Another option might be to chop off the livepatch_ prefix and then split the string on __. I suppose it doesn't really matter.

If we can describe the operation with a regular expression (because the number of fields is known in advance) I would prefer that over a procedural approach of chopping an splitting - let the regex engine do the work. A regex doesn't work if this becomes context sensitive - earlier information informs later decisions. In that case I would again consider using Angstrom because it is still quite declarative.

lindig · 2022-04-11T09:10:04Z

ocaml/xapi/livepatch.ml

+           | true ->
+               (acc, true)
+           | false ->
+               if


This could be expressed as when clause on the match case.

lindig · 2022-04-11T09:12:09Z

ocaml/xapi/livepatch.ml

+       )
+    |> get_latest_livepatch
+
+  let get_base_build_id () =


What is this code doing? It seems quite repetitive and using low-level operations. Parsing binary data could be easier using Angstrom which has operations to read bytes and Big Endian, Small Endian numbers.

It is going to read ELF notes from file /sys/kernel/notes to find out the running kernel's build id.
The file content is organized as some consecutive sections. For each section, at beginning of it, it's a header structure described by headers_struct. And the length of a section is determined by its headers. This function needs to find a specific section and read build id in it.

If Angstrom works for this in better way, I will look into it. Thanks :)

https://github.com/lindig/fit/blob/main/lib/fit.ml#L213-L222

For example, this is code I wrote to read the (binary) header of a FIT file. There is more complex code in that file but it gives you a flavor how to read bytes, multi-byte numbers, and strings.

There's another example of using angstrom for decoding the output of a command, it might prove useful as well: https://github.com/xapi-project/xen-api/blob/47fae74032aa6ade0fc12e867c530eaf2a96bf75/ocaml/xapi/bios_strings.ml (and tests: https://github.com/xapi-project/xen-api/blob/47fae74032aa6ade0fc12e867c530eaf2a96bf75/ocaml/tests/test_bios_strings.ml)

Looking at the documentation of angstrom was useful when I developed the module: https://docs.ocaml.pro/docs/LIBRARY.angstrom@angstrom.0.15.0/Angstrom/index.html

Hi @lindig @psafont
Thanks for your pointers. I've reworked this with Angstrom which looks very great.

ocaml/xapi/livepatch.mli

lindig · 2022-04-11T09:21:56Z

ocaml/xapi/rpm.mli

+ * GNU Lesser General Public License for more details.
+ *)
+
+module Epoch : sig


This needs doucumentation. What are these modules describing?

ocaml/xapi/livepatch.ml

minglumlu · 2022-04-14T11:15:28Z

Hi Reviewers,
I reworked based on existing comments. May I please have your review more?
I squashed the fixup as I think the changes in rework are too dramatic. Sorry for this.

lindig

The code to extract the build information is much better now.

lindig · 2022-04-20T09:00:50Z

ocaml/xapi/livepatch.ml

+type t = {
+    component: component
+  ; base_build_id: string
+  ; to_version: string option


I am bit surprised about to_ - it is somewhat unusual. Does this indicate the version and release reached after applying the patch? Maybe a comment could clarify this.

Yes. It indicates that the version and release reached after applying the live patch.

I see that there are comments that explain this in the mli file.

lindig · 2022-04-20T09:17:05Z

ocaml/xapi/livepatch.ml

+module BuildId = struct
+  let one_byte =
+    let open Angstrom in
+    any_char >>= fun c ->


I'd use any_uint8 or any_int8 - which will result in an int.

lindig · 2022-04-20T09:25:19Z

ocaml/xapi/livepatch.ml

+            section' ()
+      )
+
+  let section = section' ()


Could we rename section' to section and get rid of this?

lindig · 2022-04-20T09:28:54Z

ocaml/xapi/livepatch.ml

+  let align x =
+    (* The actual size is aligned with 4. *)
+    let open Int32 in
+    (if rem x 4l = 0l then x else add x (sub 4l (rem x 4l))) |> to_int


The code is using 41 but the comment says something is 4-byte aligned. What is the relevance of 41? What is the result of this function for a given x?

Sorry, I misread this. The 1 is actually an l - a size indicator. A comment what align 6 would do would be still helpful. Are we rounding up or down to a multiple of 4? I assume align x rounds x up to the next multiple of 4. I believe it could be simplified (here for integers):

let align4 n = ((n + 3) / 4) * 4 utop # align4 12;; - : int = 12 utop # align4 10;; - : int = 12 utop # align4 13;; - : int = 16

Yeah, this is great. Thanks.

lindig · 2022-04-20T09:37:40Z

ocaml/xapi/livepatch.ml

+let get_installed_livepatch_file ~component ~base_build_id =
+  let lp_dir = get_livepatch_dir ~component ~base_build_id in
+  let installed_symlink = Filename.concat lp_dir "livepatch.livepatch" in
+  if not (Sys.file_exists installed_symlink) then


This could be done with a single match:

match Unix.lstat installed_symlink with | Unix.{st_kind = S_LNK;_} -> ... Option .some | exception _ -> None

lindig · 2022-04-20T09:52:29Z

ocaml/xapi/rpm.ml

+
+  let error_msg = Printf.sprintf "Failed to parse '%s'"
+
+  let parse_epoch_version_release epoch_ver_rel =


This module looks too complicated to me. I would expect that a version is represented as a list of numbers and that we are comparing two strings to establish their order. I think it needs to be spelled out how strings and numbers are treated: is every string that can be read as a number considered a number, and or still a string? This makes a difference because "01" and "1" are different strings but the same number. Likewise, how is any string that cannot be read as a number treated? I am not sure but I would have started with:

type comp = Number of int | String of string type version = comp list (* assuming a version is a list of components, each either a string or a number *) val parse: string -> string * version (* split into package and version *) val compare: version -> version -> int (* -1, 0, 1 *)

These bits are some existing lines which are moved into a new file only in this PR.

type comp = Number of int | String of string
This is defined as similarly as:
https://github.com/xapi-project/xen-api/pull/4678/files#diff-3c831fd11744c8735ff48fa1f9e436f357cb4316e4394855770ac300ed41dc69R55

val parse: string -> string * version (* split into package and version *)
is implemented like:
https://github.com/xapi-project/xen-api/pull/4678/files#diff-3c831fd11744c8735ff48fa1f9e436f357cb4316e4394855770ac300ed41dc69R183

and val compare: version -> version -> int (* -1, 0, 1 *) is implemented as:
https://github.com/xapi-project/xen-api/pull/4678/files#diff-3c831fd11744c8735ff48fa1f9e436f357cb4316e4394855770ac300ed41dc69R160

There are some examples to explain the rules of comparison:
https://github.com/xapi-project/xen-api/pull/4678/files#diff-3c831fd11744c8735ff48fa1f9e436f357cb4316e4394855770ac300ed41dc69R166

lindig · 2022-04-21T07:56:17Z

ocaml/xapi/livepatch.ml

+type component = Xen | Kernel
+
+let component_of_string x =
+  match Astring.String.Ascii.lowercase x with


This is deprecated; use lowercase_ascii instead

lindig · 2022-04-21T08:09:13Z

ocaml/xapi/repository.ml

@@ -304,6 +334,17 @@ let set_available_updates ~__context =
        Helpers.run_in_parallel funs capacity_in_parallel
    )
  in
+  let applied_livepatches_of_hosts =


How many lines is this function long? I would encourage to split out more functions that are defined in this function. If this is done just to keep the name space clean, I would use a local module rather than making this function larger and larger.

Thanks. I split some functions out. Indeed, the set_available_updates function was too long.

robhoes

It's not easy to get a good overview of the logic and control flow because there is a lot going on in the >1000 changed lines. I think it comes down to testing well. It mostly looks fine.

robhoes · 2022-04-21T13:39:32Z

Please do not merge yet: we are sorting out some build issues first.

minglumlu · 2022-04-22T00:44:46Z

It's not easy to get a good overview of the logic and control flow because there is a lot going on in the >1000 changed lines. I think it comes down to testing well. It mostly looks fine.

Sure. I will re-do tests before merging this one. And the following PRs will be unit tests and applying live patches.

minglumlu · 2022-04-22T02:43:51Z

Hi @lindig @robhoes
I appended a new commit for a minor fixup. 170e6c9
Would you please have a review again on it?

robhoes · 2022-04-22T13:43:19Z

Looks like there are now a few conflicts due to the warning-removal PR...

Signed-off-by: Ming Lu <ming.lu@citrix.com>

This commit doesn't change any code. It only move some codes to a few new files so that they could be used more easily later. Signed-off-by: Ming Lu <ming.lu@citrix.com>

Signed-off-by: Ming Lu <ming.lu@citrix.com>

…atches Signed-off-by: Ming Lu <ming.lu@citrix.com>

Signed-off-by: Ming Lu <ming.lu@citrix.com>

This commit fixes the bug introduced in c081442: prior to c081442, the bool value is for [up_to_date], while after the commit, it is for [pkg_updates_available]. If there is(are) RPM package updates, it means a host is NOT up to date. This commit is just for this bug. Signed-off-by: Ming Lu <ming.lu@citrix.com>

Signed-off-by: Ming Lu <ming.lu@citrix.com>

minglumlu · 2022-04-24T09:41:11Z

Tests have been done. And:
fe0634c is a result of rebasing and squashing.
8017dcf is a fixup for an issue found in test.
de7c897 is a fixup for "unused" warnings

minglumlu requested review from robhoes, lindig, edwintorok and psafont and removed request for robhoes April 11, 2022 08:58

lindig reviewed Apr 11, 2022

View reviewed changes

edwintorok reviewed Apr 11, 2022

View reviewed changes

ocaml/xapi/livepatch.ml Show resolved Hide resolved

minglumlu force-pushed the private/mingl/CP-32574 branch from 11498b6 to 9b0e275 Compare April 14, 2022 11:09

lindig reviewed Apr 20, 2022

View reviewed changes

minglumlu force-pushed the private/mingl/CP-32574 branch 2 times, most recently from 91a5854 to 0dd784a Compare April 21, 2022 03:04

lindig approved these changes Apr 21, 2022

View reviewed changes

robhoes approved these changes Apr 21, 2022

View reviewed changes

minglumlu added 9 commits April 24, 2022 08:42

CP-32574: Refine code to separate merge_updates function out

a0ca80d

Signed-off-by: Ming Lu <ming.lu@citrix.com>

CP-32574: Split code into more files

d9b4a15

This commit doesn't change any code. It only move some codes to a few new files so that they could be used more easily later. Signed-off-by: Ming Lu <ming.lu@citrix.com>

CP-32574: Add new modules to support livepatch

e49438a

Signed-off-by: Ming Lu <ming.lu@citrix.com>

CP-32574: Return applied_livepatches from getting host updates

a878676

Signed-off-by: Ming Lu <ming.lu@citrix.com>

CP-32574: Calculate up-to-date flag with considering un-applied livep…

c081442

…atches Signed-off-by: Ming Lu <ming.lu@citrix.com>

CP-32574: Consolidate with considertation on livepatches

bd39dd0

Signed-off-by: Ming Lu <ming.lu@citrix.com>

Split functions out of set_available_updates

301247e

Signed-off-by: Ming Lu <ming.lu@citrix.com>

Refine names of variables related with update ID

fe0634c

Signed-off-by: Ming Lu <ming.lu@citrix.com>

minglumlu force-pushed the private/mingl/CP-32574 branch from 170e6c9 to 8017dcf Compare April 24, 2022 08:21

minglumlu force-pushed the private/mingl/CP-32574 branch 2 times, most recently from a9189b4 to 4c0cbc1 Compare April 24, 2022 09:12

Fixup: Unused open, variables and functions

de7c897

Signed-off-by: Ming Lu <ming.lu@citrix.com>

minglumlu force-pushed the private/mingl/CP-32574 branch from 4c0cbc1 to de7c897 Compare April 24, 2022 09:24

minglumlu merged commit e0d286f into xapi-project:master Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support livepatch - part 1 #4678

Support livepatch - part 1 #4678

minglumlu commented Apr 11, 2022

lindig Apr 11, 2022

lindig Apr 11, 2022

minglumlu Apr 14, 2022 •

edited

Loading

robhoes Apr 21, 2022

lindig Apr 21, 2022

lindig Apr 11, 2022

lindig Apr 11, 2022

minglumlu Apr 11, 2022

lindig Apr 11, 2022

psafont Apr 11, 2022 •

edited

Loading

minglumlu Apr 14, 2022

lindig Apr 11, 2022

minglumlu commented Apr 14, 2022

lindig left a comment

lindig Apr 20, 2022

minglumlu Apr 21, 2022

robhoes Apr 21, 2022

lindig Apr 20, 2022

lindig Apr 20, 2022

lindig Apr 20, 2022

lindig Apr 20, 2022 •

edited

Loading

minglumlu Apr 21, 2022

lindig Apr 20, 2022

lindig Apr 20, 2022

minglumlu Apr 21, 2022

lindig Apr 21, 2022

lindig Apr 21, 2022

minglumlu Apr 21, 2022

robhoes left a comment

robhoes commented Apr 21, 2022

minglumlu commented Apr 22, 2022

minglumlu commented Apr 22, 2022

robhoes commented Apr 22, 2022

minglumlu commented Apr 24, 2022 •

edited

Loading


		let error_msg = Printf.sprintf "Failed to parse '%s'"

		let parse_epoch_version_release epoch_ver_rel =

Support livepatch - part 1 #4678

Support livepatch - part 1 #4678

Conversation

minglumlu commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minglumlu Apr 14, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psafont Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minglumlu commented Apr 14, 2022

lindig 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

lindig Apr 20, 2022 • 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

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

robhoes commented Apr 21, 2022

minglumlu commented Apr 22, 2022

minglumlu commented Apr 22, 2022

robhoes commented Apr 22, 2022

minglumlu commented Apr 24, 2022 • edited Loading

minglumlu Apr 14, 2022 •

edited

Loading

psafont Apr 11, 2022 •

edited

Loading

lindig Apr 20, 2022 •

edited

Loading

minglumlu commented Apr 24, 2022 •

edited

Loading