filed: skip stripped top level directories by SamuelBoerlin · Pull Request #1686 · bareos/bareos

SamuelBoerlin · 2024-01-26T18:08:38Z

Thank you for contributing to the Bareos Project!

Currently when you have a fileset with an include with e.g. File = /some/path and Strip Path = 2 the backup will always end up with an empty directory called /some/path in it.
This PR changes this behaviour to skip saving top level directories like in the example if they're stripped away exactly.
Like so the backup won't contain an unnecessary empty directory and the restored files match exactly with what was backed up.
Before this change a restore with where=/some/path would create an empty /some/path/some/path directory (which didn't exist before the restore) whereas the rest of the files are restored correctly at e.g. /some/path/somefile just like where they were before the restore.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
~~Check backport line~~
~~Required backport PRs have been created~~

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

sebsura · 2024-01-30T11:51:45Z

I think the problem here is that the name of a directory is "too short". For your example the name of the directory is /some/path but the "linkname", which is the one we actually care about, is /some/path/. In fact stripping /some/path/ works correctly (if strippath <= 2)!

It seems like our strip function should take that fact into consideration. Could you verify whether this change fixes the issue for you ?

@@ -1789,9 +1790,12 @@ void StripPath(FindFilesPacket* ff_pkt)
    * is a different link string, attempt to strip the link. If it fails,
    * back them both back. Do not strip symlinks. I.e. if either stripping
    * fails don't strip anything. */
-  if (!do_strip(ff_pkt->StripPath, ff_pkt->fname)) {
-    UnstripPath(ff_pkt);
-    goto rtn;
+  if ((ff_pkt->type != FT_DIREND && ff_pkt->type != FT_REPARSE)
+      || ff_pkt->fname == ff_pkt->link) {
+    if (!do_strip(ff_pkt->StripPath, ff_pkt->fname)) {
+      UnstripPath(ff_pkt);
+      goto rtn;
+    }
   }

SamuelBoerlin · 2024-01-30T12:46:34Z

I think the problem here is that the name of a directory is "too short". For your example the name of the directory is /some/path but the "linkname", which is the one we actually care about, is /some/path/. In fact stripping /some/path/ works correctly (if strippath <= 2)!

It seems like our strip function should take that fact into consideration. Could you verify whether this change fixes the issue for you ?
@@ -1789,9 +1790,12 @@ void StripPath(FindFilesPacket* ff_pkt)
    * is a different link string, attempt to strip the link. If it fails,
    * back them both back. Do not strip symlinks. I.e. if either stripping
    * fails don't strip anything. */
-  if (!do_strip(ff_pkt->StripPath, ff_pkt->fname)) {
-    UnstripPath(ff_pkt);
-    goto rtn;
+  if ((ff_pkt->type != FT_DIREND && ff_pkt->type != FT_REPARSE)
+      || ff_pkt->fname == ff_pkt->link) {
+    if (!do_strip(ff_pkt->StripPath, ff_pkt->fname)) {
+      UnstripPath(ff_pkt);
+      goto rtn;
+    }
   }

Hi, thanks. I'll give that a try.

Earlier I did try using /some/path/ in the fileset includes, which indeed kind of works.
However, when you then do restore jobid=1 and ls you then get the following strange result:

$ls

build/
weird-files/

Note the empty first line. This is probably because it stores an entry for / (i.e. what's left after stripping; this can be seen when doing list files jobid=1), which is otherwise usually never backed up as far as I know, right?
That's why I made the change of not sending the directory to the SD if it's exactly stripped away.

sebsura · 2024-01-30T12:52:24Z

You are right. There is this entry. Thankfully the restore itself works correctly and the permissions/etc. of the "root" directory are also restored.

I'm not sure what the best approach is here but i think not backing up a directory that was explicitly included in the backup is not a good idea (normally being top level actually ignores some checks that would cause such a file not to be backed up for example).

SamuelBoerlin · 2024-01-30T13:02:21Z

Ah I see - if that's the intended behaviour then that's fine! Makes things a lot simpler.
I'll update the PR with your suggested change.

SamuelBoerlin · 2024-01-31T13:45:18Z

I've now undone my changes and applied your fix instead 👍

SamuelBoerlin · 2024-01-31T15:25:41Z

Hm, looks like Verify jobs with level DiskToCatalog don't work together with Strip Path.
VerifyFile in filed/verify.cc doesn't strip the path before sending it to the director.
I'll also have a try at fixing that.

SamuelBoerlin · 2024-02-03T13:14:13Z

I've now also added a fix and tests for the issue with the verify jobs.

sebsura

Thanks for the work! There are some small issues but as a whole the changes are great.

systemtests/tests/strippath/testrunner-01-setup

systemtests/tests/strippath/testrunner-02-restore

systemtests/tests/strippath/testrunner-03-verify

systemtests/tests/strippath/etc/bareos/bareos-sd.d/storage/bareos-sd.conf.in

systemtests/tests/strippath/etc/bareos/bareos-fd.d/client/myself.conf.in

systemtests/tests/strippath/etc/bareos/bareos-dir.d/director/bareos-dir.conf.in

sebsura · 2024-02-09T06:47:07Z

We had some CI changes, so i rebased it onto master so it could be build again.

sebsura

Thanks for the great work! The only thing left is to squash the fixup commits!

SamuelBoerlin · 2024-02-12T09:44:35Z

Done. Thanks for your help!

SamuelBoerlin · 2024-02-16T10:38:28Z

Should I create backports of this PR? Currently DiskToCatalog verify jobs don't work at all when StripPath is used.

sebsura · 2024-02-19T05:41:10Z

Since both StripPath and Verify jobs are very rarely done, we decided on not backporting these changes.

sebsura · 2024-02-19T12:18:19Z

@SamuelBoerlin If you create a backport to 23, we can merge it.

bruno-at-bareos requested a review from sebsura January 30, 2024 10:52

bruno-at-bareos assigned sebsura Jan 30, 2024

sebsura requested changes Feb 8, 2024

View reviewed changes

sebsura force-pushed the skip-stripped-dirs branch from ccb99d2 to ada50f7 Compare February 9, 2024 06:46

sebsura approved these changes Feb 12, 2024

View reviewed changes

SamuelBoerlin force-pushed the skip-stripped-dirs branch from 813b3a0 to 39828bf Compare February 12, 2024 09:43

SamuelBoerlin and others added 12 commits February 13, 2024 16:52

filed: skip stripped top level directories

48b17c5

filed: undo strip path changes

45ce593

systemtests: clean up strippath test

bff3cf3

filed: fix stripping top level directories

1cfbd52

systemtests: split strippath tests

f9c7625

systemtests: add strippath verify job test

57223a9

filed: fix DiskToCatalog job with StripPath

8053f84

systemtests: strippath tests for all verify levels

c06488e

systemtests: clean up config comments

cbbd5e6

systemtests: fix strippath test setup

bec1106

dird: fix trying to write batch if it does not exist

15b76d7

Update CHANGELOG.md

3bcd414

BareosBot force-pushed the skip-stripped-dirs branch from 5f8ed2c to 3bcd414 Compare February 13, 2024 16:52

BareosBot merged commit 198d77c into bareos:master Feb 13, 2024

SamuelBoerlin mentioned this pull request Mar 1, 2024

filed: skip stripped top level directories #1726

Merged

8 tasks

Conversation

SamuelBoerlin commented Jan 26, 2024 • edited by sebsura Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Tests

Uh oh!

sebsura commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamuelBoerlin commented Jan 30, 2024

Uh oh!

sebsura commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamuelBoerlin commented Jan 30, 2024

Uh oh!

SamuelBoerlin commented Jan 31, 2024

Uh oh!

SamuelBoerlin commented Jan 31, 2024

Uh oh!

SamuelBoerlin commented Feb 3, 2024

Uh oh!

sebsura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebsura commented Feb 9, 2024

Uh oh!

sebsura left a comment

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin commented Feb 12, 2024

Uh oh!

SamuelBoerlin commented Feb 16, 2024

Uh oh!

sebsura commented Feb 19, 2024

Uh oh!

sebsura commented Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SamuelBoerlin commented Jan 26, 2024 •

edited by sebsura

Loading

sebsura commented Jan 30, 2024 •

edited

Loading

sebsura commented Jan 30, 2024 •

edited

Loading