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

adaed74a9be0 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
• [x] revertToLastGood() should check for an empty/invalid list, possibly even emit signals to be handled in the UI
• [X] add command line equivalent of revertToLastGood() to patchmanager/patchmanager-tool
• [x] 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 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 https://github.com/nephros/patchmanager/pull/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).

[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).