src/update_handler: ignore mount error before running slot hook #1591
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
- Coverage 84.42% 84.39% -0.03%
==========================================
Files 69 69
Lines 21666 21663 -3
==========================================
- Hits 18291 18282 -9
- Misses 3375 3381 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The handler mounts the other-slot filesystem, and it returns in error if it fails to mount it. The filesystem may be in bad health, or may be not yet made, and thus, it causes the handler to fail. As a consequence, it results in a slot that is unupdatable until the filesystem is remade manually (i.e. externally). This ignores the mount error before the slot hook is run, and leaves the hook to decide whether or not to fail. Signed-off-by: Gaël PORTAY <gael.portay@rtone.fr>
gportay
force-pushed
the
ignore-mount-error-before-running-slot-hook
branch
from
5fdf833 to
642cc35
Compare
|
Fixes: #276 @gportay Thank you for addressing this! The discussion on the original issue halted back in 2018, but re-reading my comment I would stick with what I said back then (and your solution is one of those proposed there). I would also clarify in the docs that a pre-install hook (on a mountable fs slot type) must not assume the slot is mounted/readable but must explicitly check and handle this itself. |
I am thinking about adding a flag in parameter to the function mount_and_run_slot_hook(), so it returns in error or not if the filesystem is mounted or not. This way, we can behave differently if running a pre-install or post-install slot hook. But, my opinion is to defer the error handling to the script for now and not adding any flags... this introduces a "behavior" change for bundles installing a blob that is not a filesystem (let's say dm-verity hashes) using a post-install slot hook (RAUC would no fail anymore). I wonder if it makes sense to control such things from the |
You could just check for the
When using a file system type, we must assume that the installed blob is a file system. Otherwise, a different slot type, like |
| g_message("Mounting slot %s", slot->device); | ||
| res = r_mount_slot(slot, &ierror); | ||
| if (!res) { | ||
| g_propagate_error(error, ierror); | ||
| goto out; | ||
| g_warning("Ignoring mount error before slot hook error: %s", ierror->message); |
There was a problem hiding this comment.
This message doesn't make sense here. I guess it is copied and modified from below?
Please also ensure that this applies to pre-install handlers only as mentioned in the other comment.
|
In general, we need to allow mount failures before the installation, as the slot can be un-mountable for valid reasons. The way this PR implements it would lead to cases where I'm not sure how widespread this issue is or what the best way to handle it would be. We could:
@gportay @ejoerns Do you see other options? Do you have preferences? |
Oh that is a good catch!
Well, I have no preference for now. EDIT; I would hesitate between 2 and 4. |
The documentation for
Thus one could also argue that this variable always needs to be checked and that the change now finally reflects the documented behavior (because "attempt to automatically mount" does not mean it will ensure it is mounted). The hook documentation itself is a bit more specific here:
But it does not say something about the variable, though. I also would not know of good use cases where a pre-install hook needs access to the target partition. The way more common use case would be to use this in a post-install hook, of course. Anyway, if we can prevent potential issues with this, we should probably do so.
I would like to introduce a new flag only if necessary. Based on the documentation, we could maybe implement the 'immutable file' approach and update the documentation or use a distinct hook name. Both add complexity, though.. |
A pre-install hook could be used to copy some data that needs to be restored later. I wouldn't count that as a good use case, but it's certainly currently possible, so there's probably someone doing that. |
|
@jluebbe, @ejoerns, do you think this can be merged? Or should we implement a flag somewhere? I do not mind implementing a flag, but I would prefer not to. For now, if I am correct, the update fails because the filesystem is not mountable. With this patch, the update would succeed but, the I tend to think the maintainer of the However, as I said, I do not mind implementing a flag to avoid people doing unexpected and unrecoverable things such as |
The handler mounts the other-slot filesystem, and it returns in error if it fails to mount it.
The filesystem may be in bad health, or may be not yet made, and thus, it causes the handler to fail.
As a consequence, it results in a slot that is unupdatable until the filesystem is remade manually (i.e. externally).
This ignores the mount error before the slot hook is run, and leaves the hook to decide whether or not to fail.
Hello,
This PR intends to fix an issue if installing a bundle with a pre-install slot hook, on the device slot A system, with the slot B partitions created, but without the filesystems made yet.
The handler function img_to_raw_handler() runs the pre_install hook and fails because the filesystem is not made yet. I will be
madecopy right after by the function write_image_to_dev().(Well, I supposed it comes from that handler, I have not checked it).
With this PR, I am suggesting to ignore mount failure in function mount_and_run_slot_hook(). This fixes the issue I am facing; however, I guess it may have some side-effects and I am not convinced yet.
What are you opinions?
The issue can be reproduced easily:
dd if=/dev/zero of=/dev/other-slot), andrauc install bundle.raucb).Regards,
Gaël
Here are the failing logs on 1.12:
Here are the logs on 1.12 + PR:
Note: I have tested it with that custom bootloader backend, and that pre-install slot hook. The bundle is generated by that genimage config file.