backintime icon indicating copy to clipboard operation
backintime copied to clipboard

Feature: Add postUnmount plugin callback

Open tatokis opened this issue 3 years ago • 6 comments

This is useful for unmounting the remote drive over ssh after sshfs is unmounted.

If the remote drive is unmounted before sshfs with the existing callback, then sshfs can not be unmounted and the mountpoint shows up as

d????????? ? ?         ?            ?            ? mountpoint

until the remote drive is remounted.

The existing unmount() function was not renamed to preUnmount() in order to not break any existing plugins.

tatokis avatar Aug 09 '22 02:08 tatokis

Thank's a lot for your contribution.

Please be aware of the discussion in #1232 especially 1232#issuecomment-1146882621 .

In short: The project is not dead but it will take a while until we can take your PR into account.

buhtz avatar Aug 22 '22 13:08 buhtz

@aryoda The key is that it's called after backend.umount(). That's when backintime unmounts the sshfs. The end user doesn't have control of the sshfs mount as it's handled by backintime itself.

The issue is that the remote drive that the sshfs points to would be unmounted before the sshfs itself if done in the unmount callback, rendering the sshfs non-unmountable afterwards by BIT.

The only way other way would be to try to detect which sshfs mountpoint BIT is using in the unmount callback, and manually unmount it before BIT tries to, but this is difficult and failure prone at best.

tatokis avatar Oct 04 '22 10:10 tatokis

THX! So the problem I think is that

https://github.com/bit-team/backintime/blob/76a4b1bc1e0df8344069c15f5ec9b13cad688243/common/mount.py#L178

is called by BiT after self.config.PLUGIN_MANAGER.unmount(self.profile_id) (=> user-callback) which is strange I think because it should be called after above unmount so that we don't need a postUnmount at all.

Edit: Or deprecate "unmount" and introduce "beforeUnmount" and "afterUnmount" (like "processBegin" and "processEnd")...

I am not sure if there is already a good unit test to cover these lines of code with sshfs so I don't dare to change this at the moment.

I will come back to look deeper into this PR once I have got more time left (and after stabilizing BiT by fixing the most urgent bugs).

aryoda avatar Oct 04 '22 11:10 aryoda

I did some research about the basic functionality and behavior of user-callback. As a side-product I created some unit-tests (#1639) but they won't help solving this actual PR.

To my knowledge user-callback behavior is not covered by any of the current tests. Of course the code is executed in some tests (and covered in a technical way) but not tested.

IMHO the behavior to test would be if the callback reasons/steps are called on the right position on the timeline after/before some other specific steps of the taking a snapshot process.

My assumption is that this is nearly impossible in the current state of the productive code. The code is not isolated enough. And especially in testing user-callback behavior to many elements of code are involved. I see no way doing this with "unit tests" or "integration tests" in the near time.

A "dirty" approach could be to do a full snapshot run in a test and then parsing the log output for the expected messages (or their patterns) and if they appear in the correct order. We still have tests doing full backups. They do use some helper code from test/generic.py.

buhtz avatar Feb 08 '24 11:02 buhtz

is called by BiT after self.config.PLUGIN_MANAGER.unmount(self.profile_id) (=> user-callback) which is strange I think because it should be called after above unmount so that we don't need a postUnmount at all.

Edit: Or deprecate "unmount" and introduce "beforeUnmount" and "afterUnmount" (like "processBegin" and "processEnd")...

There is an old Issue (#648) from 2016 requesting also a "level 9" in the callback scripts to signaling that everything is done (after unmount).

buhtz avatar Feb 20 '24 08:02 buhtz