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.

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

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

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.

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

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

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.

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.

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

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.

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.