Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add postUnmount plugin callback #1269

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

tatokis
Copy link
Contributor

@tatokis tatokis commented Aug 9, 2022

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.

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.
@buhtz
Copy link
Member

buhtz commented Aug 22, 2022

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.

@emtiu emtiu added the Feature requests a new feature label Sep 9, 2022
Copy link
Contributor

@aryoda aryoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tatokis I am trying to understand how the implementation supports your described use case:

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

Your proposed implementation introduces a new postUnmount() method but it is always called together with the unmount() user-callback method:

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

So I am trying to understand why the extra postUnmount() is required at all.
Could it suffice for your use case to implement the unmount of sshfs and the remote drive together (sequentially) directly in the unmount() user-callback script only?

@tatokis
Copy link
Contributor Author

tatokis commented Oct 4, 2022

@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.

@aryoda
Copy link
Contributor

aryoda commented Oct 4, 2022

THX! So the problem I think is that

backend.umount()

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

@buhtz buhtz changed the base branch from main to dev January 16, 2023 09:06
@buhtz buhtz assigned buhtz and unassigned buhtz Jul 6, 2023
@buhtz buhtz added Low relevant, but not urgent HELP-WANTED Used by 24pullrequests.com to suggest issues labels Jan 5, 2024
@buhtz buhtz changed the title Add postUnmount plugin callback Feature: Add postUnmount plugin callback Feb 4, 2024
@buhtz
Copy link
Member

buhtz commented Feb 8, 2024

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 buhtz marked this pull request as draft February 20, 2024 08:29
@buhtz buhtz mentioned this pull request Feb 20, 2024
@buhtz
Copy link
Member

buhtz commented Feb 20, 2024

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 buhtz mentioned this pull request Feb 27, 2024
buhtz added a commit that referenced this pull request Mar 9, 2024
@buhtz buhtz added the Discussion decision or consensus needed label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion decision or consensus needed Feature requests a new feature HELP-WANTED Used by 24pullrequests.com to suggest issues Low relevant, but not urgent User-Callback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants