Add the borg recreate #1030
No reviewers
Labels
No labels
blocked
breaking
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
borgmatic-collective/borgmatic!1030
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "VandalByte/borgmatic:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR aims to add the
borg recreateas a native borgmatic action.Closes #610
This looks like a good start!
@ -0,0 +4,4 @@import borgmatic.borg.environmentimport borgmatic.borg.featureimport borgmatic.borg.flagsimport borgmatic.borg.repo_deleteThis'll happen as part of running testing: Flaking will complain here about unused imports.
@ -0,0 +32,4 @@elif logger.isEnabledFor(logging.INFO):verbosity_flags = ('--info',)command = [local_path, 'recreate', repository]It looks like Borg takes a repository or an archive for its
recreatesubcommand.Additionally, for Borg 2 the repository/archive CLI syntax is completely different from Borg 1. Fortunately, many other borgmatic actions already deal with this so you don't have to reinvent the wheel. (See
borg/flags.py:make_repository_archive_flags().)@ -0,0 +33,4 @@verbosity_flags = ('--info',)command = [local_path, 'recreate', repository]command.extend(verbosity_flags)borgmatic (generally) uses a declarative syntax rather than imperative for constructing arguments like these.
@ -0,0 +34,4 @@command = [local_path, 'recreate', repository]command.extend(verbosity_flags)command.extend(global_arguments)I don't think this will work as-is;
global_argumentsis usually an instance ofargparse.Namespace()and represents the global arguments passed to borgmatic from the CLI. Check out some of the other borgmatic actions for how these are laboriously converted to Borg flags.On the use of
make_flags_from_arguments()here: You'll find out when it comes time to test this, but I'm guessing this will pull in way more arguments than you actually want converted into flags for this!Yeah even I thought of that, I'll keep that in mind. 👍
@ -0,0 +35,4 @@command = [local_path, 'recreate', repository]command.extend(verbosity_flags)command.extend(global_arguments)command.extend(recreate_arguments)Similar here. This is by convention an
argparse.Namepace()of borgmatic's ownrecreatearguments.@ -0,0 +54,4 @@remote_path=None,):'''Recreate a Borg archive with the given repository and configuration.Docstring convention is to document each function argument. See some of the other actions for examples.
@ -0,0 +69,4 @@patterns_file = write_patterns_file(patterns, borgmatic.config.paths.get_runtime_directory())if patterns_file:command.extend(['--patterns-from', patterns_file.name])Nice! Couple of thoughts:
make_recreate_command()since it's part of constructing that command?actions/create.pyis responsible for collecting these patterns, specifically theprocess_patterns(collect_patterns(config), working_directory)call inrun_create(). It might make sense to factor some of that out into a separate file for reuse in your code (say in an eventualactions/recreate.py).I've removed the patterns for now, I'll add it later in
actions/recreate.py@ -0,0 +79,4 @@remote_path=remote_path,borg_local_path=local_path,borg_exit_codes=config.get('borg_exit_codes'))Also part of running tests: The
blackcode formatter!I've made the changes based on the feedback provided. Do let me know if everything looks good. I’m working on adding the recreate to
borgmatic/commands/arguments.pynext.@ -0,0 +46,4 @@+ global_flags+ recreate_flags+ exclude_flags)Nice and declarative!
Left a couple of minor comments, but I'll wait till there's more done for more. Thanks!
I have made some new additions, few things extra that I need to look at are:
recreate_argumentsinborgmatic/borg/recreate.pybecause right now it's not being used in there.recreate_argumentsand add the PATH functionality as mentioned in docs hereborgmatic/actions/recreate.py( any tips regarding this will be helpful :) )I'll finish this by tomorrow.
Thanks, I'll have a look at this when I get the chance!
The overall approach looks good to me! Make sure you try running this though.. As mentioned in a comment, I'm not sure the patterns get passed through to Borg correctly right now, but it'd be hopefully straightforward to address that.
@ -0,0 +2,4 @@import borgmatic.borg.recreateimport borgmatic.config.validatefrom borgmatic.actions.create import collect_patterns, process_patternsJust in terms of organization, it may make sense for some of this to be factored out of
create.pysince it's now used by multiple actions and isn'tcreate-specific. Do not feel strongly.I went with the approach of just importing it to avoid code redundancy. But I do get what you mean, I'll add them in with recreate.py.
Oh I wasn't suggesting duplicating them! Just maybe putting them in a shared file that both
create.pyandrecreate.pyimport and use. But again, I don't feel strongly about doing that. Just making the suggestion.Oh, got it👍
@ -0,0 +27,4 @@else:logger.info('Recreating repository')# collect and process patternsSuper minor: Ideally comments are full sentences. (So "Collect and process patterns." in this case.)
@ -0,0 +31,4 @@patterns = collect_patterns(config)processed_patterns = process_patterns(patterns, borgmatic.config.paths.get_working_directory(config))The general approach here with patterns for
recreatelooks good to me!One thought though is you could call
collect_patterns()inline since thepatternsvariable isn't used again. For that matter, you could even callprocess_patterns()inline below sinceprocessed_patternsis only used once. Do not feel at all strongly about either.@ -0,0 +43,4 @@+ (('--log-json',) if global_arguments.log_json else ())+ (('--lock-wait', str(lock_wait)) if lock_wait else ())+ (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())+ (('--debug', '--show-rc', '--list') if logger.isEnabledFor(logging.DEBUG) else ())What do you think of adding a
--listflag to the acceptedrecreatecommand-line arguments and then making--listpresent here depending solely on that flag? And maybe that could also implicitly pass--filter ...to Borg? See the use ofmake_list_filter_flags()inborg/create.py.This is basically how
createworks now.I did think of doing it that way but, if I remember correctly it was extract.py, where in borg docs
--listwas an optional arg, was just added along as a logging flag. I'll add it separately.Yeah I see what you're saying about
extract.py. My thinking is thatrecreate.pyis maybe a little closer tocreate.pyand users might expect it to work somewhat similarly. Also: It's not your concern at this point, but I'm adding alistconfiguration option for an unrelated feature (#303), and it could ultimately serve as a default for--listcommand-line arguments for actions likecreateandrecreate.@ -0,0 +44,4 @@+ (('--lock-wait', str(lock_wait)) if lock_wait else ())+ (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())+ (('--debug', '--show-rc', '--list') if logger.isEnabledFor(logging.DEBUG) else ())+ pattern_flagsThe way you're constructing these
pattern_flagsunfortunately won't work, because (if I'm reading this correctly) you're passing raw patterns to--patterns-from, which instead expect a patterns filename.The good news is I think the approach used for that in
borgmatic/borg/create.pywill work here too—callingwrite_patterns_file()to write all the patterns to a temporary file and then using that filename with--patterns-from.@ -0,0 +60,4 @@remote_path=remote_path,borg_local_path=local_path,borg_exit_codes=config.get('borg_exit_codes'),)It might make more sense for the
output_log_levelto belogging.INFOinstead oflogging.ANSWERsince this isn't a "answer my question by providing output" type of action like, say,listorinfo.That change would also have the side effect of making tests slightly easier because you wouldn't have to deal with
add_custom_log_levels().I've added most of the requested changes and tests, the big one
tests/unit/borg/test_recreate.pyis a WIP. I feel like I have addressed most of the requested changes, do let me know if something looks odd.Also, should I add
recreateto skip_actions and commands inborgmatic/config/schema.yaml?skip_actions:commands:type: arrayitems:EDIT:
I added it to
skip_actionssince thetest_schema.pywas failing without it. For now, I have added it to bothskip_actionandcommands.Yeah, adding it to the
skip_actionsandcommandswhenenums sounds right to me!WIP: Add the borg recreateto Add the borg recreateAll tests have been completed, and the tests passed locally.
Add the borg recreateto WIP: Add the borg recreateI still have few manual tests I want to do. Other than that testing only things left are:
Where do you say we move this to? Also any suggestion about the file name to use for the new file,
pattern.py? in/borgmatic/action/but onepattern.pyalready exist in/borgmatic/borg/Do you want me to add this
--listto args? in/borgmatic/commands/arguments.pyright now this is what i have inborgmatic/borg/recreate.pyIMO it's fine to have same-named source files in different directories. In this particular case,
actions/pattern.pyseems like as good a spot as any for these functions.That seems reasonable, yeah. Note that I don't think you need the
hasattr()call.Just a few minor comments.
@ -0,0 +31,4 @@if archive is None:logger.error('Please provide a valid archive name.')returnI don't think this is necessary if you make
--archiverequired=Trueinarguments.py.@ -0,0 +41,4 @@patterns, borgmatic.config.paths.get_working_directory(config))recreate_cmd = (Minor code style nit: Spell out variable names (
cmd->command).The issue is, I'm using methods from both
create.pyfrom/borgmatic/action/andborgmatic/borg/. And file borgmatic/borg/pattern.py (class) already exists, so will that be ok if I add it to that?I added that because I was getting error on execution of
borgmatic recreatesaying complaining about no attributelist, I got the same forpathas well. ForpathI'm probably going to set it as required, because when you runborg recreatewith just repo::archive you get this:EDIT:
Now that I explained about path, I just realized it's a positional argument and not an optional one, so need to change that as well.
I think it really depends on what those function are. The way I think about
actions/vs.borg/, the main distinction is thatactions/contains code (including shared utilities) for borgmatic's own action logic, whileborg/contains code (including shared utilities) for interacting with Borg. Sometimes that line can get a little blurry, but for instance I would only add functions toborg/pattern.pythat deal in Borg patterns and not borgmatic's own data structures or concepts (configuration, etc.)So if you're thinking about the existing
parse_pattern()andcollect_patterns()in particular:collect_patterns(): This to me seems like it belongs at theactions/level because it deals directly in processing borgmatic config, which seems like more of a borgmatic concern rather than a Borg concern.parse_pattern(): I could see an argument for moving this into eitherborg/pattern.pyor, say,actions/pattern.py. That's because it operates directly on the data structures currently defined inborg/pattern.py(as a kind of constructor for them), but it's a function that's typically called fromcollect_patterns()and so maybe should live alongside it.I'm happy to weigh in on any other functions you're thinking about, but most likely wherever you put them will be fine. We can always move them around later. You could even leave them where there are for now in
actions/create.py, etc. if all this seems like too big of a refactor for now.Hmm I'm not sure why the attribute (for
list) wouldn't be there if it's not required. My understanding of argparse is the attribute is always there, but if it's not required then the value may just beNone. Standalone example:... which produces:
Is it possible that whatever paths or patterns or excludes gets stuffed into
--patterns-fromare interfering and making Borg think there's nothing to recreate? The docs for recreate seem to indicate that path really is optional. This line from the docs is interesting:Are absolute paths getting passed in, maybe?
Yeah that will be great. I think I'll probably work on that on a new PR after this one.
I figured it out, it was because I hadn't added it to the arguments.
I still have to test out patterns and exclusions. Then only I will know for sure.
So right now I managed to make the basic recreate work, from the examples of borg,
pathis basically the patterns of archive insiderepo::archive, I though of it as a path to place the newly created repo/archive. recreate also supports--pattern. So I'm thinking of removing thepathentirely then use the patten flags instead. I still on the fence about this, whether to keep it in or remove it.I'm also thinking of adding the archive options, or do I let user give them? ( factor in the flags from namespace excluding the repo, archive and list flags) which one do you think is the best? I'm thinking of 1st option. But I though of the second because for borg and borg2 the archive options are quite different except a few being common. So I was thinking of adding those common ones (example:
--comment,--timestampand a few others) and take up the rest, if any, from namespace args.Sounds good.
Ah yeah, that would do it!
Yeah, I could see an argument for either approach. I will say that if you start without
path, I think we can always add it in later if there's a need. And if you start withpathincluded, you could maybe just roll it into the patterns file like we do with everything else (source_directories,excludes, etc.)—although maybe that would be a pain just in terms of where that's done in the code.Here are my thoughts on the archive options:
--match-archives(Borg 2): I think this is worth having, and the code for this exists for several other actions already so it should hopefully be straightforward. Although it might need to be tweaked a little since Borg 1 doesn't support this flag for this action.--first/--last/--oldest/--newest/--older/--newer(Borg 2): IMO nice to have but not critical. Several existing borgmatic actions support these.--sort-by(Borg 2): Not even sure why this is needed on this action? Maybe it influences the--listoutput?--target: Should probably include.--comment: Would be good to include.--timestamp: Nice to have.--compression/--chunker-params: Doesn't need to be on borgmatic's command-line, but should be passed to Borg from the relevant configuration option.--recompress: Could be either command-line or a configuration option. (Note that once #303 is done, all configuration options will be override-able from the command-line at the global scope.)These are just my takes on this. Feel free to make calls as you see fit.
@ -0,0 +37,4 @@Executes the recreate command with the given arguments.'''if recreate_arguments.timestamp and not is_valid_timestamp(recreate_arguments.timestamp):Is there a reason not to let Borg do this validation for you? Presumably if it accepts a timestamp value it also validates it.
@ -0,0 +80,4 @@if recreate_arguments.target and recreate_arguments.archiveelse ())+ (('--comment', f'"{recreate_arguments.comment}"') if recreate_arguments.comment else ())I think the way to do this reliably would be
shlex.quote()?For now, I'm not adding
pathsince the patterns I tested, seem to work very well. And I think I'll leave the flag commented out, just in case we might have to revisit it in the future.Now regarding the flags, I have added
--target,--comment,--timestampand--compression(compression I have to addaction='store_true'since it's taken from config file)Next I'll add
--match-archives,--recompressand--chunker-params. I am skipping--sort-by.--first/--last/ - Since it doesn't seem that critically necessary, I'll try to include it, if not, laterOk, so these (
--oldest/--newest/--older/--newer), use the TIMESTAMP, there's something I came across while I was adding--timestampyesterday, timestamp that I gave was not one one being added to archive.I tested it out in borg 1.2.8, and the issue is when we give the time say,
07:10borg converts it to UTC by adding the offset of our local time. Example: give time in TIMESTAMP07:10, adds the offset7:10 + 5:30 = 12:40, so12:40is reflected when we doborg list, for me it felt a bit weird because I gave a time but something else was being reflected, in borg2 they handled it I guess, by giving option for offset. I thought of adjusting it, but since it's a borg issue I thought shouldn't.Also that would mean different validation, but since you mentioned this, I guess I will go with letting borg handle it. Thanks :)
Leaving it out sounds fine for now, but I'd prefer removing the commented out code entirely, just as a matter of code cleanliness. You can always make a branch or a
git stashif you want to save it off somewhere in case you need it for later.Sounds good, although I think you can probably omit
--compressionsince: 1. It's available from the configuration, and 2. When #303 lands, all config options will be override-able from the command-line. Or if you prefer you can leave--compressionfor now and I can deduplicate flags as part of #303 work.I'd put
--chunker-paramsin the same category as--compressionIMO. And leaving off--first/--lastto start with seems fine to me.Sounds reasonable!
Yeah, so now
--compression,--recompress,--chunker-paramsare taken from config. I should have mentioned that clearly in my last comment.All local unit tests are passing, and I believe it's ready for a full review. It's been an interesting learning curve and experience, I should say :)
WIP: Add the borg recreateto Add the borg recreateThanks for all the exhaustive testing and for sticking with this! I think this is almost ready to merge.
@ -0,0 +36,4 @@# Available recompress MODES: 'if-different' (default), 'always', 'never'recompress = config.get('recompress', None)# Write patterns to a temporary file and use that file with --patterns-from.recompressanywhere in the config schema. 2. If you do want to add it, you might consider making it an enum since it sounds like it can only have certain values.@ -0,0 +61,4 @@# Flag --target works only for a single archive+ (('--target', recreate_arguments.target) if recreate_arguments.target and archive else ())+ (('--comment', shlex.quote(recreate_arguments.comment))Good use of
shlex.quote()!@ -0,0 +8,4 @@from ..test_verbosity import insert_logging_mock# from borgmatic.borg.pattern import Pattern, Pattern_type, Pattern_style, Pattern_source# from borgmatic.borg.create import make_exclude_flags, make_list_filter_flagsRemove commented out code, please!
Oops! might have missed that.
@ -0,0 +26,4 @@def test_recreate_archive_dry_run_skips_execution():flexmock(module.borgmatic.borg.flags).should_receive('make_repository_archive_flags').and_return(('repo::archive',))This is a good start, but IMO all the test functions of
recreate_archive()in this file also need to mock out:make_exclude_flags()write_patterns_file()make_list_filter_flags()make_match_archives_flags()make_repository_flags()(if called)The idea is to isolate the unit under test so we're only testing it and not all the other random borgmatic functions it calls. Python standard library calls usually don't have to be mocked out though (unless they hit disk or whatever).
Hey @witten
I have added the changes you mentioned. Thanks.
Okay, hopefully some final UX/docs-level comments!
@ -1548,0 +1568,4 @@recreate_group.add_argument('--target',metavar='TARGET',help='Name of new archive name',Do you think it's worth adding "to create instead of recreating the existing archive" or something similar? I ask because if I was just looking through the help as-is, I might be confused as to why there's both
--archiveand--target.Yeah you're right, I will make it more clearer.
How's this?
Looks good! You could even shorten the end to
without replacing itif you wanted.@ -1548,0 +1573,4 @@recreate_group.add_argument('--comment',metavar='COMMENT',help='Add a comment text to the archive, if archive not provided, consider all archives',I'm not following the ", if archive not provided, consider all archives" part here. Is that meant for a different flag maybe?
I put that part there because during my testing I noticed that if you don't give an archive name to the borg command, every archive under that repo gets the comment that we just gave. I should make that a bit more clearer I guess.
Oh gotcha! In that case I might suggest tweaking the wording to something more explicit like:
... or even:
@ -1548,0 +1578,4 @@recreate_group.add_argument('--timestamp',metavar='TIMESTAMP',help='Manually specify the archive creation date/time (UTC)',Maybe "override" instead of "specify" here? Or just some way of indicating that you're replacing the existing/default timestamp by using this flag.
@ -287,0 +300,4 @@See https://borgbackup.readthedocs.io/en/stable/usage/recreate.htmlfor details.Not passing --recompress is equivalent to "--recompress never".Defaults to "if-different".These two statements seem to be at odds...? The default can't be both "if-different" and "never". I might be misunderstanding.
Yeah it's a bit confusing to be honest. I took it straight from the docs
I think what they meant is if
recompressflag is not used, it's like using--recompress never, if it's used without a value then it'sif-differentI can remove that "never" part if it's too confusing.
Yeah I don't think it's possible to use without a value, given the current schema and the enum! If I'm reading it correctly, the possibilities are:
recompressis not present in the config at all, which I guess defaults toif-differentneverrecompressis set explicitly to one ofif-different,always, orneverUsing
--recompress neveris same as not using the flag at all, so it shouldn't be an issue right?Right now how I have set it up is:
So if it's not in config then, ignored completely, so same as using
--recompress neverOops, yes, you are correct! So maybe change the description to
Defaults to "never"and remove the bit aboutNot passing --recompress ...?I've made those changes.
Thank you so much!