backup functionality

[eimrek]

Hi @giovannipizzi @sphuber, as discussed (also in #20), I moved the disk-objectstore backup functionality from aiidateam/aiida-core#6069 to this project.

It works currently, but I added everything currently in a separate file backup_utils.py, to keep it a bit decoupled for now. Also requires some polishing (e.g. the logging part).

However, there are some issues:

1. Code duplication w.r.t. verdi storage backup aiida-core#6069: i basically added the same utility functions to run commands on local and remote machines & to call rsync. Is this duplication fine or do you have some ideas how to change this design?
2. We say windows is supported for this repo (in the README.md), but I think relying on the unix commands breaks this. e.g. i'm using [ -e <path> ] to check if a folder exists. I used this so that I could use the same method for both local and remote paths. However, another option is to use python-specific checks for local paths (e.g. pathlib.Path.exists() for this case) and unix-specific ones for remote ones. This might be a way to keep the windows support, at least if doing backups to a local folder.

Any ideas welcome.

If this approach works, i can polish it further and add some tests.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (90cd4c8) 99.89% compared to head (ac10458) 99.90%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #161    +/-   ##
========================================
  Coverage   99.89%   99.90%            
========================================
  Files           9       10     +1     
  Lines        1896     2066   +170     
========================================
+ Hits         1894     2064   +170     
  Misses          2        2            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sphuber]

Thanks @eimrek . To me the concept looks fine. I haven't gone in detail over the actual intricacies of the order in which you copy data such that it allows this operation during operation. I think it would be good to document the reasoning at least somewhere (either in the docs or in the docstrings of the backup util functions) to explain why this should be safe. The CLI command help should also explicitly state if it is safe to perform while the OS is being used.

Code duplication w.r.t. aiidateam/aiida-core#6069: i basically added the same utility functions to run commands on local and remote machines & to call rsync. Is this duplication fine or do you have some ideas how to change this design?

Sure there is duplication now, but if we were to merge the code here and make a release, aiida-core could simply reuse the disk_objectstore.backup_utils module, couldn't it? You could directly use those functions in the PsqlDosBackend.backup method, so I don't see how you would have to duplicate the code.

We say windows is supported for this repo (in the README.md), but I think relying on the unix commands breaks this. e.g. i'm using [ -e ] to check if a folder exists. I used this so that I could use the same method for both local and remote paths. However, another option is to use python-specific checks for local paths (e.g. pathlib.Path.exists() for this case) and unix-specific ones for remote ones. This might be a way to keep the windows support, at least if doing backups to a local folder.

This will definitely break, since that is a bash command, which will not run natively on windows. But even if you were to replace this and find a cross-platform solution, you are still using rsync which also doesn't natively run on Windows. So I would say that with this approach, a cross-platform solution will be very difficult. I personally would be fine to just add a disclaimer to the CLI that the backup command is Linux/MacOS only. Don't think it will be very easy to find a cross-platform solution that is as performant and I doubt that there are currently a lot of Windows users that we should cater to.

@giovannipizzi thoughts?

[sphuber]

@eimrek let me know if you want me to have a look at this and or have particular questions. Since you marked it as WIP I haven't gone through it yet

[eimrek]

@sphuber sure. i think the main features are there, i just need to add tests.

[eimrek]

Note after discussion with @edan-bainglass : his usecase (mounting a windows folder to a linux container and backing up there) might not support symlinks. At least in my case, where to mount a windows folder to my linux virtual machine doesn't, by default. Therefore, it might make sense to avoid using symlinks altogether.

[eimrek]

Hi @sphuber @giovannipizzi, this PR is in a state where your feedback would be great. the functionality is there and i added main CLI tests. But some notes/comments regarding things i'm not sure about:

• in many places, I check for success of a subprocess command based on the exit code & if it fails, print something and "return False" from the backup function. I'm not fully sure if this is the best way to handle these errors, suggestions welcome (e.g. maybe assert is better or using exceptions more directly, ...). Additionally, testing all of these failures does not seem straightforward.
• currently we're running tests on "windows-latest", which are failing. how to deal with this?
• additionally, the "ssh localhost" currently is failing on mac-os, i'll soon investigate this further

[eimrek]

Alright, this is finally finalized. I changed the design such that there is a "metaclass" BackupManager that contains all relevant config options and utilities (e.g. calling rsync and other commands) and folder management (using correct rsync link-dest, making backup to a live folder and then moving to final destination when done, etc). The most important method is backup_auto_folders(backup_func: Callable), which argument is the actual backup function and will be different for the disk-objectstore container and aiida-core storage. Will adapt aiidateam/aiida-core#6069 accordingly ASAP. I also added a docs page for the backups.

Any suggestions welcome :)

[eimrek]

@sphuber @giovannipizzi feel free to submit your review if you have a moment to check.

[sphuber]

Thanks @eimrek . As discussed online, although the cyclic dependency between the manager calling a callback which takes the manager itself seems non-ideal, I also cannot see a much better solution now that would still require the generic functionality to be reused in aiida-core, so let's keep that for now. The same goes for the exes functionality which I think is not really the responsibility of this tool, but since it is already there and it works, let's keep it as well.

Only things remaining are minor changes that we should be able to address quickly to get this merged. Thanks a lot!

[sphuber]

Thanks a lot @eimrek