Skip to content

fix file count mismatch on restores that use recycled volumes#1330

Merged
arogge merged 6 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/fix-file-count-mismatch
Dec 15, 2022
Merged

fix file count mismatch on restores that use recycled volumes#1330
arogge merged 6 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/fix-file-count-mismatch

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Dec 11, 2022

Thank you for contributing to the Bareos Project!

Description

When using recycled volumes, the restore process may report wrong expected number of files.
This PR fixes the issue.

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)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Check backport line
  • Is the PR title usable as CHANGELOG entry?
  • Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.
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
  • bareos-check-sources --since-merge does not report any problems
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

@alaaeddineelamri alaaeddineelamri changed the title dird: stored: fix file count mismatch on restores using recycled volumes dird: stored: fix file count mismatch on restores that use recycled volumes Dec 12, 2022
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/fix-file-count-mismatch branch from a13a88c to 8356e5c Compare December 12, 2022 14:18
@alaaeddineelamri alaaeddineelamri marked this pull request as ready for review December 12, 2022 16:09
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

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

See comments.


if (JobCanceled(jcr)) { CancelStorageDaemonJob(jcr); }

if (jcr->dir_impl->ExpectedFiles != jcr->JobFiles) { TermCode = JS_Warnings; }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to set a boolean variable filecount_does_mismatch to true here and use that in line 442 so that we only have this check once.

Copy link
Member

Choose a reason for hiding this comment

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

Strictly thinking, we have two possibilities here:
1: TermCode is currently JS_Terminated, then we update the TermCode to JS_Warnings and the -- warnings file count mismatch message
2.: Term Code is already JS_Warning and we have also a file count mismatch . Then we probably want to have -- with warnings and file count mismatch as message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about I just make the file count mismatch a joblog warning instead of a termination message?

Copy link
Member

Choose a reason for hiding this comment

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

If we can do that, it would be great. "Restore OK -- with warnings" with a reasonable error message would work for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we can do that, it would be great. "Restore OK -- with warnings" with a reasonable error message would work for me.

already done.

Comment on lines +110 to +112
"FirstIndex = (CASE when firstindex=0 then %lu else firstindex end), "
"StartBlock = (CASE when startblock=0 then %lu else startblock end), "
"StartFile = (CASE when startfile=0 then %lu else startfile end), "
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that firstindex, startblock and startfile are always > 0 in normal operation, and only the dummy place holder sets them to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only the dummy placeholder creates zero-filled jobmedia record, no other place in the code requires those fields to be zero.

Copy link
Member

Choose a reason for hiding this comment

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

as we're overwriting placeholder data with real data, I would consider this safe (even though it isn't nice).

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/fix-file-count-mismatch branch from 8356e5c to ba90cfe Compare December 14, 2022 09:37
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/fix-file-count-mismatch branch from ba90cfe to bb9aed5 Compare December 14, 2022 13:42
@pstorz pstorz self-requested a review December 15, 2022 09:38
Alaa Eddine Elamri and others added 4 commits December 15, 2022 13:13
A file count mismatch during restore only changed the result message,
but not the job term code.

Now the job term code is changed to warning, and now also this is issued
if the number of files restored are **different** to the number of files
expected.
Before, the warning was only issued if **less files than expected** were
restored.

Co-authored-by: Philipp Storz <philipp.storz@bareos.com>
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/fix-file-count-mismatch branch 2 times, most recently from b8bf03c to 40c13c6 Compare December 15, 2022 13:54
@arogge arogge force-pushed the dev/alaaeddineelamri/master/fix-file-count-mismatch branch from 40c13c6 to 415bb1d Compare December 15, 2022 14:47
@arogge arogge changed the title dird: stored: fix file count mismatch on restores that use recycled volumes fix file count mismatch on restores that use recycled volumes Dec 15, 2022
@arogge arogge merged commit 97f6fa6 into bareos:master Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants