NDMP_BAREOS: support autoxflate plugin by pstorz · Pull Request #1013 · bareos/bareos

pstorz · 2021-12-06T16:33:10Z

Thank you for contributing to the Bareos Project!

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)

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

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

arogge

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

arogge · 2022-01-10T16:56:50Z

core/src/stored/ndmp_tape.cc

+                              /* reverse= */ true)
+          != bRC_OK) {
+        ok = false;
        continue;


Doesn't this mean we will ignore the plugin event's error and continue reading?
Is that out intent here?

I updated the code to uset the ok variable to end the loop as it is in the original code.

arogge · 2022-01-10T17:03:37Z

core/src/stored/ndmp_tape.cc

+    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) {


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.

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.

arogge

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?

arogge · 2022-01-11T13:36:03Z

core/src/stored/ndmp_tape.cc

+        continue;
+      }
+      if (dcr->after_rec) {  // translation happened?
+        rec = dcr->after_rec;


if GeneratePluginEvent() set dcr->after_rec, doesn't that mean it allocated a new record that we need to free with FreeRecord()?

arogge · 2022-02-01T15:26:41Z

core/src/plugins/stored/autoxflate/autoxflate-sd.cc

+  if (!record_was_swapped) {
+    if (AutoxflateModeContainsOut(dcr->autodeflate)) {


Suggested change

if (!record_was_swapped) {

if (AutoxflateModeContainsOut(dcr->autodeflate)) {

if (!record_was_swapped && AutoxflateModeContainsOut(dcr->autodeflate)) {

arogge · 2022-02-01T15:27:04Z

core/src/plugins/stored/autoxflate/autoxflate-sd.cc

+  if (!record_was_swapped) {
+    if (AutoxflateModeContainsOut(dcr->autodeflate)) {


Suggested change

if (!record_was_swapped) {

if (AutoxflateModeContainsOut(dcr->autodeflate)) {

if (!record_was_swapped && AutoxflateModeContainsOut(dcr->autodeflate)) {

arogge · 2022-02-01T15:32:15Z

core/src/plugins/stored/autoxflate/autoxflate-sd.cc

-  switch (rec->maskedStream) {
-    case STREAM_COMPRESSED_DATA:
-    case STREAM_WIN32_COMPRESSED_DATA:
-    case STREAM_SPARSE_COMPRESSED_DATA:
-      break;
-    default:
-      goto bail_out;
-  }


I'm not sure this can be safely removed.

arogge · 2022-02-01T16:11:17Z

systemtests/tests/ndmp/etc/bareos/bareos-sd.d/device/FileStorage2.conf

+  Auto Inflate = in
+  Auto Deflate = out


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.

NDMP_BAREOS AutoXFlate support (backport PR #1013)

pstorz marked this pull request as draft December 6, 2021 16:33

pstorz force-pushed the dev/pstorz/master/ndmp-autoxflate-on-restore branch from a12db95 to 252fa19 Compare December 7, 2021 12:08

pstorz changed the title ~~NDMP: support autoxflate plugin~~ NDMP_BAREOS: support autoxflate plugin Dec 7, 2021

pstorz force-pushed the dev/pstorz/master/ndmp-autoxflate-on-restore branch from 05662ff to a4f1123 Compare December 7, 2021 12:50

pstorz requested a review from arogge December 7, 2021 13:33

pstorz force-pushed the dev/pstorz/master/ndmp-autoxflate-on-restore branch from a4f1123 to 27b492e Compare December 23, 2021 09:54

arogge reviewed Jan 10, 2022

View reviewed changes

arogge reviewed Jan 11, 2022

View reviewed changes

pstorz self-assigned this Jan 20, 2022

pstorz force-pushed the dev/pstorz/master/ndmp-autoxflate-on-restore branch 2 times, most recently from 35dd2cc to 96d1b4f Compare February 1, 2022 13:20

pstorz marked this pull request as ready for review February 1, 2022 14:29

pstorz requested a review from arogge February 1, 2022 14:29

arogge reviewed Feb 1, 2022

View reviewed changes

pstorz and others added 14 commits February 24, 2022 13:43

update CHANGELOG.md

7a9dbd4

ndmp_tape: enable autoinflate on NDMP restores

c909a4f

ndmp systemtest: enable autoxflate

3c0430e

The ndmp systemtest now has autoxflate enabled on the storages.

autoxflate-sd.cc: refactoring and cleanup

d335185

- extract functions - remove unneeded switch statements - shorten too long comments

ndmp_tape.cc: shorten comments where possible

ddbfe19

compression.cc: shorten long comment

2929888

console.cc,bsmtp.cc: init vars to avoid -O0 compile warnings

3c044b6

ndmp_tape.cc: use ok variable

5b22a99

ndmp_tape.cc: call GeneratePluginEvent for all streams

e38c2b7

ndmp_tape.cc: add message how device needs to be setup in error message

6099ca5

ndmp_tape: cleanup

f54a490

read_record.cc: remove unneeded free records

382f73c

stored: use M_FATAL on compressed NDMP datastream

3be6ab6

arogge force-pushed the dev/pstorz/master/ndmp-autoxflate-on-restore branch from 7c46994 to b8b4a4d Compare February 24, 2022 12:46

arogge approved these changes Feb 24, 2022

View reviewed changes

arogge merged commit 16da019 into master Feb 24, 2022

arogge deleted the dev/pstorz/master/ndmp-autoxflate-on-restore branch February 24, 2022 12:51

bruno-at-bareos mentioned this pull request Feb 28, 2022

NDMP_BAREOS AutoXFlate support (backport PR #1013) #1090

Merged

7 tasks

pstorz added a commit that referenced this pull request Mar 1, 2022

Merge pull request #1090

cfa52ac

NDMP_BAREOS AutoXFlate support (backport PR #1013)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NDMP_BAREOS: support autoxflate plugin#1013

NDMP_BAREOS: support autoxflate plugin#1013
arogge merged 14 commits intomasterfrom
dev/pstorz/master/ndmp-autoxflate-on-restore

pstorz commented Dec 6, 2021 •

edited by arogge

Loading

Uh oh!

arogge left a comment

Uh oh!

Uh oh!

arogge Jan 10, 2022

Uh oh!

pstorz Jan 31, 2022

Uh oh!

arogge Jan 10, 2022

Uh oh!

pstorz Jan 31, 2022

Uh oh!

arogge left a comment

Uh oh!

arogge Jan 11, 2022

Uh oh!

arogge Feb 1, 2022

Uh oh!

arogge Feb 1, 2022

Uh oh!

arogge Feb 1, 2022

Uh oh!

arogge Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if (!record_was_swapped) {
		if (AutoxflateModeContainsOut(dcr->autodeflate)) {

	if (!record_was_swapped) {
	if (AutoxflateModeContainsOut(dcr->autodeflate)) {
	if (!record_was_swapped && AutoxflateModeContainsOut(dcr->autodeflate)) {

Conversation

pstorz commented Dec 6, 2021 • edited by arogge 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!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pstorz commented Dec 6, 2021 •

edited by arogge

Loading