Split Update module into components and add tests

[yfyf]

This is in an unfinished state (esp. the test part), but pushing this out for early review/feedback.

This PR does the following:

• Refactors the core of Update module to use abstract interfaces for its external dependencies (RAUC, Curl/Connman, runtime configuration) rather than direct bindings / values.
• Splits the Update.run function into a separate single transition function (run_step) and the infinite recursive process (run).
• Exposes an alternative interface to the Update process as a functor over the external dependencies.
• Adds a prototype test "framework" and a sample unit test for the Update module.

Some of the code is shifted around/generalized as a side-effect of the changes, but this PR should (hopefully!) not introduce any logical / semantic changes, it only creates new ways for interacting with the code and the sample tests.

This is WIP since several things need to be worked out in more detail:

• The dependency interfaces are just straightforward wrappers around the functions used in the original code, but more thought could be put into this to figure out what exactly are we abstracting over.
• The code split into the update_deps module/file is rather ad-hoc
• The test "framework" is now quite basic in terms of what it can test, but my ocaml skills are too limited to figure out how to create mock interfaces neatly¹.
• Other server modules should probably share (at least) the RAUC abstract interfaces too?

I also left some TODOs as inline comments.

@knuton let me know of what you think about this "direction" and specific things that you think need to be done differently. If the general direction seems reasonable, I would attempt to flesh out the test framework and add more complex tests cases. Any ocaml-foo tips are also very welcome.

Apologies for the huge diff, the process was very unlinear, so did not produce structured git commits.

P.S. Is there a specific ocaml style used at Dividat? Maybe we could adopt one of the standard ocamlformat profiles?

Footnotes

1. I want to be able to record every mock call and update the mocked responses at run time. I have an idea how to do this with a simple PPX "decorator", but no idea how to do it otherwise. There's ocaml-mock, but it's very primitive / requires lots of boilreplate. ↩

[knuton]

Thanks!

Pushing to wrap up some feature work for an app release now, and I have only had the time for a first skim. Some high level comments for now.

• Refactors the core of Update module to use abstract interfaces for its external dependencies (RAUC, Curl/Connman, runtime configuration) rather than direct bindings / values.

Good

• Splits the Update.run function into a separate single transition function (run_step) and the infinite recursive process (run).

Good

• Exposes an alternative interface to the Update process as a functor over the external dependencies.

Good, I think

• Adds a prototype test "framework" and a sample unit test for the Update module.

Good, I think

• The dependency interfaces are just straightforward wrappers around the functions used in the original code, but more thought could be put into this to figure out what exactly are we abstracting over.
• The code split into the update_deps module/file is rather ad-hoc
• Other server modules should probably share (at least) the RAUC abstract interfaces too?

I think these are related, and also what I was struggling with from a first look. For just this module, the abstractions seem a bit heavy-handed at the moment. My hunch is that this would benefit from thinking about it in a slightly wider scope, as in how could other modules consume this? Even if then possibly only implementing in the context of this one module at first.

[yfyf]

I think these are related, and also what I was struggling with from a first look. For just this module, the abstractions seem a bit heavy-handed at the moment. My hunch is that this would benefit from thinking about it in a slightly wider scope, as in how could other modules consume this? Even if then possibly only implementing in the context of this one module at first.

Agree, I did not really put much thought into this and it has to be refined w.r.t. the other modules/components. I will attempt this later this week, but I definitely want to avoid this becoming a full-blown refactoring of service composition within the codebase. However, I pushed this out for early review because I am interested in your thoughts on the overall mechanics (i.e. interfaces for dependencies + functors + first-class modules) of this approach. It feels a bit un-ocaml-ish to me, but then again - I cannot think of a much better alternative? Been reading discussions <https://discuss.ocaml.org/t/the-shape-design-problem/7810> (also <https://discuss.ocaml.org/t/functors-for-dependency-injection/736>, also <https://mcclurmc.wordpress.com/2012/12/18/ocaml-pattern-easy-functor/>) on different approaches, but there does not seem to be a consensus on the "best" way.

[yfyf]

@knuton pushed updates that attempt to tidy-up / generalize things:

• RAUC gets its own "service" interface, added the mark_good function to it which would make it immediately re-usable in other modules where it is used.
• Could not think of a useful abstraction that would apply to the Curl wrapper, so essentially left it as is, only moved it to a separate file. It does not seem to be useful outside of the update module, as the only other place where proxy resolution is needed is gui, but there it is dealing with proxy settings at a much lower level. An alternative could be to create an UpdateHTTPClient interface/module, but I would prefer to not introduce new abstractions at this point.
• Converted the ConfigInterface into a plain record, because it's supposed to be just data anyway.
• Added init _ methods to the interfaces that provide a canonical way to initialize these services / wrappers, to be used in the top-level entrypoint (i.e. server.ml)

Also, answering my own question the mechanics, the Ocaml manual states that "a typical use of first-class modules is to select at run-time among several implementations of a signature", so I feel it kinda validates this approach.

P.S. I think I also figured out how to elegantly wrap the test implementations into a mock'ed interface, so will update the test prototype soon too.

[yfyf]

Went all in with splitting the code into modular components and testing all of them independently. Also abandoned the "mock" approach, instead went for stub-implementations of the dependencies.

The changes introduce 3 new abstractions/modules:

• RaucServiceIntf - methods for interacting with RAUC, implementation is just a functor that wraps the OBus peer reference, stub for tests is Mock_rauc
• UpateClientIntf - interface for obtaining version metadata and downloading bundles, implementation is obtained by moving out related functions from the Update module. Also a functor, parameterized over the config parameters. Stub for tests is Mock_update_client. This replaces the kind of useless CurlProxyInterface I had earier.
• UpdateService which uses the above two, the interface is essentially the same as in the last changeset.

I also moved all of the runtime / nix-provided system configuration into a single config library as a starting point for a more structured approach towards config management. This is necessary to enable runtime test setup, but also sed-template-replacements-as-configuration is kind of a massive hack, so this at least consolidates it into once location and we can think where to go from here.

To run the tests, do

CURL_EXECUTABLE_PATH="" dune test -f

(The env var / path should later be specified in either the dune config for tests, or, more generally as part of nix-shell setup? Something to be discussed.)

There are separate tests for update_client and for the whole of update / update_service. Many more should be added for update_service, but I actually need specification on what is supposed to happen in various scenarios, it is a bit unclear.

I also littered the code with various TODOs and NOTEs for potential future improvements / fixes, but I tried to avoid changing any of the semantics, as before.

P.S. Since curl is invoked via a sub-process, could we not replace all of the manual-passing-around-of-proxy in the code by simply setting the HTTPS_PROXY env variable in the nix-equivalent of /etc/profile when it is setup in gui.ml? Unless you maintain separate proxy configs per connman service, but it does not seem so.

[knuton]

P.S. Since curl is invoked via a sub-process, could we not replace all of the manual-passing-around-of-proxy in the code by simply setting the HTTPS_PROXY env variable in the nix-equivalent of /etc/profile when it is setup in gui.ml? Unless you maintain separate proxy configs per connman service, but it does not seem so.

Assuming you are suggesting to just read HTTPS_PROXY from an environment variable at service startup once: I think this wouldn't work, because (a) the proxy should be picked up immediately (without restart) when the configuration is changed, and (b) the active proxy might change depending on which network service is the current gateway (which might change without the controller's involvement).

[knuton]

A collection of initial and mostly stylistic comments as I work my way from bottom up.

[knuton]

Nice! I like the little spec language.

Going to send a lightweight spec list next.

[knuton]

Sat down to write a spec, but instead accidentally refactored the run function to separate effectful queries from the version info processing logic: https://github.com/knuton/playos/commits/testing-pure-fun

Even with the added abstractions for effects, I think this kind of reorganization should make testing considerably easier.

[yfyf]

refactored the run function to separate effectful queries from the version info processing logic

Incorporated these changes, but I went further and made evaluate_version_info totally pure, because it does not need to log the error case - it will be done anyway once the state machine enters the ErrorGettingVersionInfo state. So now it just returns the next state.

It could be potentially simplified even further by writing higher level helper funs for dealing with semver comparisons RAUC booted/primary slot semantics, but let's leave it for the future.

[yfyf]

Pushed some semi-final (hopefully!) changes:

• UpdateClient now looks up the current proxy dynamically from connman (as it was done originally)
• No more global test state - all mocks/stubs are now object instances that get initialized on each test case, tests are parameterized over a test_context. (I tried a module/functor based approach, but it just felt like re-inventing classes and objects/instances rather than using them directly, so I went ahead with objects. It still requires some boilerplate, but it's somewhat more straightforward in mu opinion).
• some minor improvements / clearing TODOs and dead code
• I noticed Config.System now partially duplicates the role of Info. I added an include Config.System in info.ml for now, not sure it's optimal.

At this point I see two main remaining things:

• More test scenarios for UpdateService
• I kinda want to rename mock everywhere into stub, because mock would imply doing some assertions on how the mock was used (was-method-called, etc), whereas here we just have a test implementation. What do you think?

[yfyf]

@knuton pushed updates to the earlier tests and a new set of tests that attempt to exhaustively check all the possible (162) combinations as per the spec you provided, which I distilled into this function.

Some cases (well, actually about a third of them) are failing now (see attachment), because the current implementation does not match the spec. Many are irrelevant and are due to the spec not distinguishing between "doing nothing" and producing an error/warning (and then doing nothing), which can be easily resolved. But some are really questionable, e.g. in test case 90 UpdateService would attempt to update the inactive slot, whereas nothing should be done (according to the spec, but also probably a good idea in practice).

I am using an intermediate representation of expected outcomes for now to provide more room for interpreting the results.

Will add the side-effect / IO / failure tests next.

P.S. Alcotest has automagic terminal width detection that leads to truncated output and unreadable test case descriptions. We are running an outdated version of alcotest that does not support the ALCOTEST_COLUMNS env setting. If you want to get the full output locally, you have to open a "wide" terminal window and run ALCOTEST_SHOW_ERRORS=1 dune test --no-buffer.

[knuton]

This is very interesting

• I kinda want to rename mock everywhere into stub, because mock would imply doing some assertions on how the mock was used (was-method-called, etc), whereas here we just have a test implementation. What do you think?

I think you are right, though I admit that I wouldn't have minded calling them "mocks".

Some cases (well, actually about a third of them) are failing now (see attachment), because the current implementation does not match the spec. Many are irrelevant and are due to the spec not distinguishing between "doing nothing" and producing an error/warning (and then doing nothing), which can be easily resolved. But some are really questionable, e.g. in test case 90 UpdateService would attempt to update the inactive slot, whereas nothing should be done (according to the spec, but also probably a good idea in practice).

Great. I was indeed struggling with this distinction when drafting a spec, and I think it was part of what led me to think about separating data from interpretation and action.

You did warm me to the goldfish memory design, so maybe it's enough to keep the action separate. What do you think about an interpreter ("free monad") approach to the effects? This may help check the expected action/effect often being "nothing", while still being able to expect a specific interpretation. This may bridge to your intermediate representations.

[yfyf]

Updates:

• Changed evaluate_version_info to use the agreed notion of up-to-date = greater than or equal to latest upstream for version comparison.
• Simplified the test predicate for version comparison, now all the cases pass.
• Added a mechanism for randomly injecting faults into the mocks
• Using the above, implemented qcheck prop tests that inject faults and check that UpdateService always recovers, reusing the different possible system_slot_spec combos to trigger different possible state machine transitions.
• Added a few extra test cases for UpdateService and UpdateClient.
• Plus a bunch of misc fixes, shuffling the code around and rebased on top of main.

I consider this complete now, unless there are specific things that you want me to change. Commits could be squashed too if you prefer that.

Thoughts for the future: as we discussed, the "absolute worst" scenario is if we release a version of PlayOS that boots and seems "good", but that somehow breaks the auto-update process (and therefore the system never self-updates). Therefore, it would be a good idea to be a bit more cautious before marking the newly installed system as good, e.g. add checks for whether RAUC is functional, the UpdateService loop completes successfully, etc. Now it only checks whether it is able to boot?

[guyonvarch]

Running the VM, with ./build vm and ./result/bin/run-in-vm, and openning the backdor with socat (see command given by the output of run-in-vm), I am getting at a high rate the following error message:

[  107.255679] playos-controller[774]: playos-controller: [ERROR] failed to get version information: E("The name de.pengutronix.rauc was not provided by any .service files")
[  107.255861] playos-controller[774]: playos-controller: [INFO] update mechanism: (ErrorGettingVersionInfo
[  107.255974] playos-controller[774]:  "E(\"The name de.pengutronix.rauc was not provided by any .service files\")")

I don’t get this message on the main branch.

[yfyf]

Running the VM, with ./build vm and ./result/bin/run-in-vm, and openning the backdor with socat (see command given by the output of run-in-vm), I am getting at a high rate the following error message:
<..>

RAUC is not available when running PlayOS via run-in-vm, so this error is kind of expected? It might have been logged differently (or not logged?), but the fact that it is failing to interact with RAUC should not have changed.

[yfyf]

I am converting this back to draft, because while implementing end-to-end tests (PR incoming today), I realized this code will break when deployed, because the nix template variables are being replaced only in specific modules and they are moved into config.ml in this PR. Will fix it ASAP.

[guyonvarch]

RAUC is not available when running PlayOS via run-in-vm, so this error is kind of expected? It might have been logged differently (or not logged?), but the fact that it is failing to interact with RAUC should not have changed.

Yes, but the problem is the super high rate (multiple times each second), making the backdoor when developping useless, and flooding the logs. In main, the error message shows up, but at a slow rate (every x seconds).

[yfyf]

Yes, but the problem is the super high rate (multiple times each second), making the backdoor when developping useless, and flooding the logs. In main, the error message shows up, but at a slow rate (every x seconds).

Ah, I see, strange - I will look into it!

[yfyf]

Yes, but the problem is the super high rate (multiple times each second), making the backdoor when developping useless, and flooding the logs. In main, the error message shows up, but at a slow rate (every x seconds).

Ah, I see, strange - I will look into it!

@guyonvarch should be fixed now, see the last commit. This was quite an ugly bug, thank you for pointing it out!

[guyonvarch]

First batch of comments on the non test part, I’ll have a look at the test afterward.

[guyonvarch]

I mostly read through the tests, but without necessarily getting everything, as there are non trivial parts.

Ocaml doesn’t help in this regard, requiring to put the most general function at the end of the file, you get the big picture in the end.

[guyonvarch]

Why would we have to match on every variant in the ADT, could you provide an example?

Also, I don’t understand at this point where the magic pattern is created, is it in Update.sexp_of_state?

[yfyf]

Why would we have to match on every variant in the ADT, could you provide an example?

Because we need to define equality for state, which would require defining equality for all the cases, including e.g. UpToDate of version_info.

[yfyf]

Also, I don’t understand at this point where the magic pattern is created, is it in Update.sexp_of_state?

Do you mean it's unclear how it's used? Here are a few places:

• playos/controller/tests/update_tests.ml

  Line 65 in 1831876

  StateReached (Installing (_MAGIC_PAT ^ expected_bundle_name upstream_version));

• playos/controller/tests/update_tests.ml

  Line 77 in 1831876

  StateReached (ErrorInstalling _MAGIC_PAT);

• playos/controller/tests/update_tests.ml

  Lines 12 to 14 in 1831876

  	let expected_bundle_name vsn =
  	Mock_update_client.test_bundle_name ^ _MAGIC_PAT ^ vsn ^ _MAGIC_PAT
  	in