Implement a Last-Known-Good feature

[nephros]

Lets improve usability in case of failure some more: make it easier to recover.
Contributes-To: #277

Currently:

• saves the list of applied patches each time "autoapply" succeeds, i.e. at boot or Lipstick launch
• list is saved as new entry in the normal .ini file , /etc/patchmanager2.conf
• if PM goes into failure mode, adds the user menu option to load that list
• after this, 'resolve failure' should have a valid list of active patches

adaed74 adds parameters to patchmanager-tool, which is currently broken, so:
Depends-on: #436

[nephros]

TODOs

• check if setAppliedPatches is actually enough to cause the desired effect
• revertToLastGood() should check for an empty/invalid list, possibly even emit signals to be handled in the UI
• add command line equivalent of revertToLastGood() to patchmanager/patchmanager-tool
• find a better name for the menu entry - @Olf0, any ideas?

To test:

• good list saved successfully to config
• good loaded successfully from config
• revertToLastGood() working correctly
• command line saving works
• command reverting works

[b100dian]

Changes look sound, I have yet to test them though

[Olf0]

find a better name for the menu entry - @Olf0, any ideas?

I am pondering about this. I would like to get rid of the term "known good" in the user-facing dialogues and options, because users may misunderstand its meaning easily.

Do I understand correctly, that the new section in patchmanager.conf lists all enabled Patches which were successfully applied during the last auto-apply run? I.e., (corner case) an enabled Patch which failed to apply is not listed; or does the auto-apply run stop with an error if one of the enabled Patches fails to apply (sorry, I do not remember) and the new section containing this list is not updated then (so it only records fully successful auto-apply runs)?

My thoughts are currently revolving around "fall-back list" or so ... still pondering.

[b100dian]

@Olf0 is it intended that the build only provides i486 artifacts, or should I raise a bug?

[Olf0]

@Olf0 is it intended that the build only provides i486 artifacts, or should I raise a bug?

Yes, the CI run for each commit to a Pull-Request against the master branch always was only for a single architecture on the oldest supported SFOS release (currently 3.4.0): It is intended as a simple build check, but not to provide binaries to test.

I recently switched from armv7hl to i486 (upon a suggestion by @nephros), because it compiles faster, likely due to being the native architecture of the build host. This is also why I picked the oldest supported release: Coderus docker images have grown much from each SFOS release to the next release; this way we only pull 2 GB instead of 4 GB from Docker Hub for each commit of a PR to master.

P.S.: This is why I once upon long ago started to work on using GitHub's local cache. This initiative has stalled, partly due to a lack of time and priority, partly because nobody seems to be interested, because "it works", though half of the time of each CI run is spent with downloading Coderus' SFOS-build image from Docker Hub.

[nephros]

Do I understand correctly, that the new section in patchmanager.conf lists all enabled Patches which were successfully applied during the last auto-apply run? I.e., (corner case) an enabled Patch which failed to apply is not listed; or does the auto-apply run stop with an error if one of the enabled Patches fails to apply (sorry, I do not remember) and the new section containing this list is not updated then (so it only records fully successful auto-apply runs)?

Yes, the list contains only the set of successfully applied patches, and it is only stored if the whole process ends up successfully.

Failure to apply one patch does not stop the autoapply process, nor does it send PM into "failure mode".

So in case one or more patches fail, the last good set remains at its previous contents, whereas 'applied', the list of successfully activated patches stored in the .conf file, reflects the real world after autoapply finished.

See: doPrepareCacheRoot() in
https://github.com/sailfishos-patches/patchmanager/blob/master/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L547

[Olf0]

find a better name for the menu entry - @Olf0, any ideas?

I am pondering about this. I would like to get rid of the term "known good" in the user-facing dialogues and options, because users may misunderstand its meaning easily.

… misunderstandable in the sense that we as Patchmanager developers know that these Patches are "good" (however that may be interpreted). After a while I thought of "O.K. set of Patches" and after translating this to proper lingo ended up with "saved, fine set of Patches" (IOW, "stored, acceptable list of Patches"). I also considered "working" instead of "fine", but "working set" has its own meaning, specifically in IT.

Do you also consider this to be better? Should I try to alter the strings in this PR accordingly, when I find some time for that?

[nephros]

Ehm, not happy with the variants. Maybe we should just call it a "backup list of patches" or "backup configuration" or so, and in the failure mode offer a "restore from backup"?

Users should be familiar with these terms, and they are generic enough to change implementation details later.

Another term that came to mind is "failsafe" instead of known-good.

[Olf0]

Ehm, not happy with the variants.

This is fine: I wanted to do something towards finishing this task, and because I had no good ideas, took two thesauri and played with words. This is the best I could come up with yesterday, and it appeared artificial to me, both the process and the result.

Maybe we should just call it a "backup list of patches" or "backup configuration" or so, and in the failure mode offer a "restore from backup"?

Users should be familiar with these terms, and they are generic enough to change implementation details later.

Another term that came to mind is "failsafe" instead of known-good.

I fully acknowledge all three statements!

What about "Backup list of working Patches" and "Restore backup Patch list"?

[Olf0]

I was about to write:
I prepared a Patchmanager 3.2.10 release, if you, @nephros, plan to finalise this in the upcoming weeks, then I will gladly wait for this PR. Note that I already prepared a changelog line for this PR.

But looking at this I now remember, that I intended to contribute with the wording changes to "Backup list of working Patches" and "Restore backup Patch list", plus other terms derived from these two. I will try to find some time for this this or next weekend.

[Olf0]

Please take a look at my trivial PR #450, first.

[...] I intended to contribute with the wording changes to "Backup list of working Patches" and "Restore backup Patch list", plus other terms derived from these two.

Done (and a little more) by my PR nephros#6. It became non-trivial due to the amount of changes, though it only alters documentation and comments (if I have done it correctly).

After reviewing that PR, you may consider (no need to hurry at all):

[...] I prepared a Patchmanager 3.2.10 release, if you, @nephros, plan to finalise this in the upcoming weeks, then I will gladly wait for this PR. Note that I already prepared a changelog line for this PR.

"This PR" is the one right here, i.e., PR #437.

[nephros]

I feel this feature has not been tested enough to be comfortable packing it into a release just yet.
Considering the bumpy history of the "strict checking" feature I'd like this to simmer a bit more.

[Olf0]

I feel this feature has not been tested enough to be comfortable packing it into a release just yet.

O.K., I will do a "safe" release without it, then.
Edit: Did so (3.2.10), which failed greatly and became fixed, and lastly released Patchmanager 3.2.11.

But after that now I believe we should resolve the diverging feature branches first, by merging this PR and consolidating it and the more-docs branch. While we could do all that in separate development branches, I really would like to have it all merged into master for the sake of simplicity: I lost track of which branches in which repository (here and yours) contain new stuff to be merged some day, are experiments or developments which reached a dead end (and hence are stale, but not deleted), or are simple left-overs because they have been merged, but not deleted (the latter, third category presumably dwells only in your repo). While I also love to keep old experiments around (in case one wants to look at these ideas, again), merged branches ought to be deleted IMO and diverging branches (my bad here) consolidated into one (preferably master).

[b100dian]

Considering the bumpy history of the "strict checking" feature I'd like this to simmer a bit more.

Shouldn't we just admit that we outsource testing and get through with this? I personally am not in the position to test locally (if no arm64 build comes out) very often.

[Olf0]

Shouldn't we just admit that we outsource testing and get through with this?

This is a conclusion I arrived at in other projects, too. I just lack the time, so I rather try to improve things than to test thoroughly. If we want to be "extra careful" we can label a release as beta, but deploy it via the usual update channels (otherwise it will not be broadly tested).

[Olf0]

Shouldn't we just admit that we outsource testing and get through with this?

This is a conclusion I arrived at in other projects, too.

I.e. "yes".

@nephros, what is your perspective on this?

[Olf0]

@nephros, in August 2023 we had this conversation in the comments thread of this PR:

I feel this feature has not been tested enough to be comfortable packing it into a release just yet.

O.K., I […] released Patchmanager 3.2.11.

But after that now I believe we should resolve the diverging feature branches first, by merging this PR and consolidating it and the more-docs branch.

I just updated the draft for the Patchmanager 3.2.12 release and there is definitely sufficient material, new features and time passed (16 months) that we should finalise the 3.2.12 release soon, but not Jolla-soon™ 😉:

• Either by creating a release now and then merging this and creating a beta release in the first half of December (which may also include fixes for flaws in Patchmanager 3.2.12).
• Or by merging this now, and creating a release or a beta release out of it.

@b100dian and I preferred the second choice ("thorough testing always happens first when being deployed to users", which is also very Jolla-style, respectively an unspoken mantra in software industry in general), but I do not really mind as log as we agree on a plan how to proceed with this PR with a timeline ending mid-December latest.

Side note: @nephros, what about merging the more-docs branch into master? Do I miss to see a reason why this is a bad idea?

P.S.: I am sure willing to perform the release handling, as soon as we agreed on a plan how to proceed.

[Olf0]

@b100dian, WRT:

I personally am not in the position to test locally (if no arm64 build comes out) very often.

aarch64 builds are generated very easily by setting an arbitrary Git-tag (which should clearly denote that it is a test version in its name), which triggers the CI pipeline. If you prefer to do that in GitHub's web-frontend, create a release with such a tag and delete the release immediately after hitting the Publish button: The Git-tag will remain (unless explicitly deleted, which is also possible in an non-obvious way in GitHub's web-frontend) and the triggered CI run will complete, generating RPM files as artefacts (which can be downloaded in GitHub's web-frontend from a GitHub-project's Actions "tab" → <the specific CI run>).

But even easier is to simply hit the Trigger services button at my Patchmanager test-build OBS-repo and to download an appropriate build from there. You and @nephros are in the "maintainer" role there, but please do not alter my OBS-repo configuration: You may submit my Patchmanager test-build OBS-repo to your personal OBS-"home"-repo as a starting point if you want to alter the config of it (but please notify me, if you think something is configured nonsensical; note that the my patchmanager package-repo has and should have exactly the same Meta-configuration as the patchmanager package-repo at SailfishOS:Chum:Testing, all diverging configuration is set in the Meta-configuration of the patchmanager sub-repo of my OBS-"home"-repo).

Please note it was really nerve killing to wait for more than 10 Minutes after adding a new commit to a PR to master in order to see if it still compiles: Hence the ci-on-pull_request.yml configuration only test-builds for i486 on the smallest Sailfish-SDK docker-image feasible (i.e. the SDK-image for SFOS 3.4.0, which saves many gigabytes of downloads from docker-hub, and the time these downloads require which is minutes for more recent Sailfish-SDK docker-images).

P.S. / Edit: If creating a aarch64 test build is something regularly needed, I also could derive a CI workflow for you which must be manually triggered, but allows to choose the branch it compiles. Just denote what your requirements are.

P.P.S. / Edit2: See also #472 (comment).

[nephros]

Side note: @nephros, what about merging the more-docs branch into master? Do I miss to see a reason why this is a bad idea?

Let me have a look-over this week.
I don't see anything that should prevent this, and shall put forth a PR for it.

Although I have yet another variant of the documentation tree somewhere which switches from qDoc to Doxygen and tries to add fancy flow graphs via Graphviz. But that is stuff for a later day :D (in part because Graphviz is not yet packaged for Sailfish OS.)

[nephros]

Ok, lets do this ;)

Now, about ordering and more-docs, I'd say we get this one in first, I'll then update more-docs on latest master.

[Olf0]

@b100dian, I would appreciate much, if you provide some guidance on #437 (comment) when you have time for that.