-
Notifications
You must be signed in to change notification settings - Fork 259
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
NDMP_BAREOS: support autoxflate plugin #1013
Conversation
a12db95
to
252fa19
Compare
05662ff
to
a4f1123
Compare
a4f1123
to
27b492e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure I understand what really happens here 100%.
However, I already have a few remarks.
core/src/stored/ndmp_tape.cc
Outdated
if (GeneratePluginEvent(jcr, bSdEventReadRecordTranslation, dcr, | ||
/* reverse= */ true) | ||
!= bRC_OK) { | ||
ok = false; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean we will ignore the plugin event's error and continue reading?
Is that out intent here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to uset the ok variable to end the loop as it is in the original code.
core/src/stored/ndmp_tape.cc
Outdated
DeviceRecord* rec = rctx->rec; | ||
// Perform record translations only if data is compressed | ||
// as NDMP needs to be decompressed in any case | ||
if (rctx->rec->maskedStream == STREAM_COMPRESSED_DATA) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When limiting the GeneratePluginEvent() to STREAM_COMPRESSED_DATE, then it will work for autoxflate, but would probably fail (or simply not do the right thing) with other potential plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeneratePluginEvent() is now called no matter what the masked stream type.
However, my tries to adapt the autoxflate settings automatically did not work. I left the commits for that so that we might have a look again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate fixing this issue by duplicating even more code. Shouldn't we extract the record reading out of ReadRecords() and use that functionality here?
core/src/stored/ndmp_tape.cc
Outdated
continue; | ||
} | ||
if (dcr->after_rec) { // translation happened? | ||
rec = dcr->after_rec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if GeneratePluginEvent()
set dcr->after_rec
, doesn't that mean it allocated a new record that we need to free with FreeRecord()
?
35dd2cc
to
96d1b4f
Compare
if (!record_was_swapped) { | ||
if (AutoxflateModeContainsOut(dcr->autodeflate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!record_was_swapped) { | |
if (AutoxflateModeContainsOut(dcr->autodeflate)) { | |
if (!record_was_swapped && AutoxflateModeContainsOut(dcr->autodeflate)) { |
if (!record_was_swapped) { | ||
if (AutoxflateModeContainsOut(dcr->autodeflate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!record_was_swapped) { | |
if (AutoxflateModeContainsOut(dcr->autodeflate)) { | |
if (!record_was_swapped && AutoxflateModeContainsOut(dcr->autodeflate)) { |
switch (rec->maskedStream) { | ||
case STREAM_COMPRESSED_DATA: | ||
case STREAM_WIN32_COMPRESSED_DATA: | ||
case STREAM_SPARSE_COMPRESSED_DATA: | ||
break; | ||
default: | ||
goto bail_out; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this can be safely removed.
Auto Inflate = in | ||
Auto Deflate = out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have a testcase with Auto Deflate = out
and no Auto Inflate
at all.
I guess we will also need to always force Auto Inflate = in
for NDMP jobs, so NDMP-data is always decompressed if needed (and the autoxflate plugin is loaded).
The ndmp systemtest now has autoxflate enabled on the storages.
- extract functions - remove unneeded switch statements - shorten too long comments
When doing an NDMP restore the data must be sent to the client inflated. This patch changes the autoxflate initialization so that an incompatible configuration, that might send compressed data to the client, will be overridden with safe defaults.
7c46994
to
b8b4a4d
Compare
NDMP_BAREOS AutoXFlate support (backport PR #1013)
Thank you for contributing to the Bareos Project!
Please check
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)
NDMP backups (using NDMP BAREOS) could be configured to use the autoxflate plugin on used storage devices
Unfortunately, the stream was not deflated during restore resulting in non-recoverable Backups.
This PR adds the feature to deflate compressed packages before sending them back to the NDMP data agent.
General
Source code quality
bareos-check-sources --since-merge
does not report any problemsgit status
should not report modifications in the source tree after building and testingTests