[percona-xtrabackup] prevent High memory usage for no reason (IO_CLOSE)

[banuchka]

In the case of IO_CLOSE inside the plugin, we don't need to wait for the subprocess to finish, because it trying to get the data from xtrabackup, but the destination for such data to backup is unavailable.
It causes high memory usage and OOM in some cases.

Please check

• Short description and the purpose of this PR is present above this paragraph

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

Helpful documentation and best practices

• Git Workflow
• Automatic Sourcecode Formatting
• Check your commit messages
• Boy Scout Rule

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

Source code quality

• Source code changes are understandable
• Variable and function names are meaningful
• Code comments are correct (logically and spelling)

[sduehr]

Hi, why did you close this PR?
It looks good so far and seems to make sense. I just need to find time to review it properly.
Your contribution is welcome. I will reopen it if you don't mind. If you still want to close it,
it would be nice if you drop a sentence why.

[banuchka]

Hi @sduehr,
I've found an issue with the restore operation. I want to add more logic and checks before killing the subprocess.
Could you please put the review on hold until additional information or changes from my side?
Does it work for you?
Thanks

[sduehr]

Sure, I've set the on-hold label.

We have a systemtest for the plugin, see
https://github.com/bareos/bareos/tree/master/systemtests/tests/py3plug-fd-percona-xtrabackup
which is running automatically, it failed on restore with

xtrabackup: recognized client arguments: --prepare=1 --target-dir=./tmp/bareos-restores/_percona/4/00000000000000000000_00000000000018129147_0000000001 
/usr/bin/xtrabackup version 8.0.26-18 based on MySQL server 8.0.26 Linux (x86_64) (revision id: 4aecf82)
xtrabackup: cd to /var/lib/jenkins/workspace/bareos_PR-1724/build/systemtests/tests/py3plug-fd-percona-xtrabackup/tmp/bareos-restores/_percona/4/00000000000000000000_00000000000018129147_0000000001/
xtrabackup: Error: cannot open ./xtrabackup_checkpoints
xtrabackup: Error: failed to read metadata from './xtrabackup_checkpoints'
exit(1) is called. Set test to failure and end test.

The systemtest is probably too specific to be used outside of the build environment, but
we are planning to allow contributors access to the test results in the future.

Thanks again, waiting for your notification start review.

Regards,
Stephan

[banuchka]

Hi @sduehr, is there any way to rerun tests?
It works for me in our environment, but I may have missed something. And please feel free to remove the on hold status.
Thanks.

[sduehr]

Thanks, build and tests are automatically triggered when new commits are pushed.
The systemtest for the percona-xtrabackup plugin did run successful now.
I'll probably have to suggest some changes during review, as it's required to comply with our guidelines to pass checks for PRs to get merged. For example the first line of a commit message must not be longer than 50 characters, see
https://docs.bareos.org/DeveloperGuide/gitworkflow.html

[banuchka]

I rewrote the commit message and added details and explanations about the importance of the changes.

[sduehr]

Did you consider trying self.stream.terminate() first, and only if that fails use self.stream.kill()?

[banuchka]

Did you consider trying self.stream.terminate() first, and only if that fails use self.stream.kill()?

Good idea. Honestly, I wrote a separate function for the dump process termination with timeouts and TERM signal instead of killing it straightaway.
Can I do that as an additional commit to the current PR or force push(again), which is the preferred way?
I'm asking because you've already started reviewing it, and I don't want to waste your time.

[sduehr]

Thanks, normally an additional commit is ok. But in this case it's only one file, so force push is ok, too.
I'll review again as soon as possible.

[sduehr]

Thanks, looks good so far.
I pushed a small change to comply with our coding guidelines.

[sduehr]

Please check my suggestions. Are you able to reproduce the case where end_dumper() would go in the second timeout, where the process didn't terminate even after sending SIGTERM?

[banuchka]

Please check my suggestions. Are you able to reproduce the case where end_dumper() would go in the second timeout, where the process didn't terminate even after sending SIGTERM?

I'm not sure that is 100% reproducible on my side. I saw an empty return code straight after sending SIGTERM which is why I put an additional 30 seconds waiting for the correct termination.

I agree that that is possible not to wait 2nd 30 sec and jump to the else block with setting 9 to the stream's return code, but prefer to wait a bit more seconds.

[sduehr]

OK, thanks. Yes, setting 9 to the stream's return isn't perfect, but ok for now.
I pushed some changes to make the commit messages compliant with our rules.