Support livepatch - part 2 - unit tests #4692
Conversation
minglumlu
force-pushed
the
private/mingl/CP-39487
branch
from
f79011d
to
f856ae5
Compare
| let transform (updates_info, update) = | ||
| eval_guidance_for_one_update ~updates_info ~update ~kind:Guidance.Absolute | ||
| ~upd_ids_of_livepatches:UpdateIdSet.empty | ||
| let transform ((updates_info, update), upd_ids_of_livepatches) = | ||
| eval_guidance_for_one_update ~updates_info ~update | ||
| ~kind:Guidance.Recommended | ||
| ~upd_ids_of_livepatches:(UpdateIdSet.of_list upd_ids_of_livepatches) |
There was a problem hiding this comment.
Could the changes here be explained in the commit message? These seem important and introduce quite a lot of churn in the test data, it would be good to know why
There was a problem hiding this comment.
Sure. I will add a commit message. The message would be:
Prior to support live patch, the guidance of an update is determined by "recommendedGuidance". With supporting to live patch, a new metadata "livepatchGuidance" is introduced. In an update, if a live patch in it is determined as applicable, its "livepatchGuidance" will overwrite its "recommendedGuidance".
There was a problem hiding this comment.
Looks like this hasn't yet been added to the commit message.
|
|
||
| exception Can_not_parse of string | ||
|
|
||
| let transform input = |
There was a problem hiding this comment.
transform is not not a good name; transform to what? It looks like this code is reading some file. So at least call it read.
There was a problem hiding this comment.
This is part of the functor interface. It is meant to transform something of type input_t to output_t, where tests below lists expected input/output combinations.
| @@ -88,7 +88,7 @@ module BuildId = struct | |||
| end | |||
|
|
|||
| module KernelLivePatch = struct | |||
| let get_running_livepatch () = | |||
| let get_running_livepatch' s = | |||
There was a problem hiding this comment.
A better name would be extract_livepatch_version. In my understanding this is extracting the version from the string argument s (would be better to use str).
There was a problem hiding this comment.
The original function (now below) called a script and then processed the output, so did more "getting". The "running" bit seems important though, because it looks for already loaded/applied/running livepatches.
There was a problem hiding this comment.
This function does nothing about running - it is extracting a version from a string. The string might come from a running system, though. So it would be at the call site where "running" should appear.
There was a problem hiding this comment.
It does if you look at the tests: it only takes things from the string (which is the output from a command) that indicates applied patches and ignores the not-yet-applied ones.
There was a problem hiding this comment.
Fair; my argument was, that this is a string function and not a function that obtains something from the running system. But you can argue that "running" is still a property of the version that this function is extracting. We can keep this.
|
It looks that 19a60eb (the second patch) is only moving test code from one file into two new ones. |
|
Note to other reviewers: Hiding whitespace to review the last patch is also helpful to see the actual changes. Definitely review this patch-by-patch. |
| component= Livepatch.Xen | ||
| ; base_build_id= | ||
| "9346194f2e98a228f5a595b13ecabd43a99fada0" | ||
| ; base_version= "4.19.19" |
There was a problem hiding this comment.
Is the point here that these versions don't match the ones in the Update below?
There was a problem hiding this comment.
Or that the update ids don't match? I don't understand "livepatch does not come from myself" in the comment above.
There was a problem hiding this comment.
I updated the comment to make it clearer.
Here the myself refers to the one update in eval_guidance_for_one_update. This function evaluate guidance against this RPM update. But this evaluation needs to consider livepatch as well:
- If this RPM update has a livepatch (its update info is contained in the ones introduced by applicable livepatches), this is for the original comment "livepatch comes from myself"
- Otherwise, "livepatch doesn't come from myself".
But anyway, I refined the comments and test data to make it clearer.
Signed-off-by: Ming Lu <ming.lu@citrix.com>
Signed-off-by: Ming Lu <ming.lu@citrix.com>
Signed-off-by: Ming Lu <ming.lu@citrix.com>
Prior to support live patch, the guidance of an update is determined by "recommendedGuidance". With supporting to live patch, a new metadata "livepatchGuidance" is introduced. In an update, if a live patch in it is determined as applicable, its "livepatchGuidance" will overwrite its "recommendedGuidance". Signed-off-by: Ming Lu <ming.lu@citrix.com>
minglumlu
force-pushed
the
private/mingl/CP-39487
branch
from
0185698
to
93ab198
Compare
|
After a fixup commit 0185698, this series have been squashed and re-based (against current master). |
This series contains unit tests against: