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

cancel virtual backups if none of the jobids contains files #1070

Merged

Conversation

pstorz
Copy link
Member

@pstorz pstorz commented Feb 11, 2022

Virtual backups consolidating jobids that contain no files failed when creating the bootstrap file.
This PR now checks for the number of files before creating the bootstrap file and cancels the consolidation if no files exist.

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
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
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
  • git status should not report modifications in the source tree after building and testing
Tests
  • Decision taken that a system- or unittest is required (if not, then remove this paragraph)
  • The decision towards a systemtest is reasonable compared to a unittest
  • Testname matches exactly what is being tested
  • Output of the test leads quickly to the origin of the fault

@pstorz pstorz marked this pull request as draft February 11, 2022 17:25
@pstorz pstorz force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch from 36414eb to 9c9b4b6 Compare February 14, 2022 13:41
arogge
arogge previously requested changes Feb 16, 2022
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

I think it is OK to consolidate a job with no files. The problem is consolidating a job with files that were purged.
So we should fail if one or more of the jobs had its files purged.


// Find oldest Jobid, get the db record and find its level
p = strchr(jobids, ','); /* find oldest jobid */
p = strchr(const_cast<char*>(jobids.data()), ','); /* find oldest jobid */
Copy link
Member

Choose a reason for hiding this comment

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

data() does not guarantee a null-terminated string. You'll need c_str() for that.
The const_cast<> is also not needed, as strchr is defined to be char* strchr(const char *s, int c); so it should happily accept the const char* that c_str() will return.

Suggested change
p = strchr(const_cast<char*>(jobids.data()), ','); /* find oldest jobid */
p = strchr(jobids.c_str(), ','); /* find oldest jobid */

Copy link
Member

Choose a reason for hiding this comment

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

at second thought: all of this pointer trickery is used, because we get a comma-separated list of jobids in a string and we need the first and the last of these.
I propose to have a function vector<std::string> split_string(const std::string& str, const std:string& delim) that splits str at delim and puts the pieces into a vector it returns.
We can then simply do:

auto jobid_list = split_string(jobids, ',');
jcr->impl->previous_jr = JobDbRecord{};
jcr->impl->previous_jr.JobId = str_to_int64(jobid_list.front.c_str());

and further below

jcr->impl->previous_jr = JobDbRecord{};
jcr->impl->previous_jr.JobId = str_to_int64(jobid_list.back.c_str());

@pstorz pstorz self-assigned this Feb 17, 2022
@arogge arogge self-assigned this Feb 24, 2022
@arogge arogge force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch from 9c9b4b6 to c363cad Compare February 25, 2022 15:27
@arogge
Copy link
Member

arogge commented Feb 28, 2022

I updated the PR branch with my personal version of choice. We now detect if one of the jobs that should be consolidated had its files pruned or was removed from the catalog (by means of prune, purge or delete) and fail in that case only.

@arogge arogge force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch 2 times, most recently from c1f4c16 to 92a25ed Compare February 28, 2022 16:35
@arogge arogge force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch from 92a25ed to b5f5ade Compare March 7, 2022 16:07
@pstorz pstorz marked this pull request as ready for review March 8, 2022 06:56
@pstorz pstorz force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch from 95abc2e to 8868d7b Compare March 9, 2022 14:24
pstorz and others added 6 commits March 9, 2022 15:26
Run three virtual backups that are supposed to fail:
1. no files in any of the jobs
2. one jobid that doesn't exist in the catalog anymore
3. one job that had its files pruned
You can now construct a PoolMem from a std::string if needed. We also
mark the single-argument constructors explicit, so you can't
accidentially convert an integer oder char* into a PoolMem.
add a templated function so that you can use an object or a lambda as
your query handler.
Add a function to split std::string into std::vector<std::string>.
This patch adds a check that makes sure that the input jobids for the
virtual job are valid (i.e. exist in the database) and did't have their
files pruned.
@pstorz pstorz force-pushed the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch from 8868d7b to 27ad8df Compare March 9, 2022 14:27
@arogge arogge merged commit 18797e4 into master Mar 10, 2022
@arogge arogge deleted the dev/pstorz/master/vbackup-cancel-if-only-zero-file-jobs branch March 10, 2022 09:07
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.

None yet

2 participants