dird: add prev and new jobid variables by SamuelBoerlin · Pull Request #1499 · bareos/bareos

SamuelBoerlin · 2023-06-30T18:37:08Z

Thank you for contributing to the Bareos Project!

This PR adds two new RunScript variables %O and %N which contain the previous and new jobid of a migration/copy job.
Currently there is no convenient way to retrieve this information in RunScripts. This would be useful to e.g. easily archive copied or migrated jobs (especially with always incremental and in combination with #1372):

  Run Script {
        console = "update jobid=%N jobtype=A"
        Runs When = After
        Runs On Client = No
        Runs On Failure = No
  }

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)

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
Check backport line
Required backport PRs have been created

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

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

SamuelBoerlin · 2023-06-30T18:42:02Z

core/src/dird/dird_conf.cc

  static char yes[] = "yes";
  static char no[] = "no";
+
+  static char str[50];


Is using a static array to store and return the result okay? I copied that from another similar callback, so I'm not sure.

I don't really like it, because it won't be thread-safe. i.e. if the threads for multiple jobs enter job_code_callback() around the same time, there's a possible data-race on str which may lead to the wrong value being returned.

However, there's no great way to work around this inside the function right now. I guess the best way to proceed would be to remove the requirement for extern "C" and return a std::string instead of a char*.
I'll see if I can make that change easily.

Alright, thanks for your feedback. I already have a branch where I've refactored this callback to return an std::optional<std::string>, should I merge that into this PR or do you want that somewhere else?

I've only refactored it up to util.cc#edit_job_codes though (SamuelBoerlin@0b7498a#diff-edeceb51c3de23d07b2d5d9e679d111e1238b74cc870a492143e5283e4386ee9R937-R943), after which point it gets more complicated.

That looks promising! You can just add it to this PR as it is the refactoring required to implement the feature you're working on.

You could

drop the extern "C" everywhere, having C-linkage if you need a std::string probably won't help much

return the plain char*/const char*, compiler will take care of conversion on return

Your solution for edit_job_codes() is probably as good as it gets without refactoring the whole thing. However, for a plain char* snprintf() is better suited than Bsnprintf(), as the latter expects a resizable POOLMEM*.

SamuelBoerlin · 2023-07-03T08:56:29Z

Just realized there seems to already exist a %m variable here: https://github.com/bareos/bareos/blob/master/core/src/filed/fileset.cc#L68-L69
but I couldn't find anything about this in the documentation. Technically this shouldn't be a problem because that %m and my proposed %m should never conflict, but it might be confusing from a configuration standpoint.

arogge

btw. I cannot thank you enough for adding the test for your new feature!

arogge · 2023-07-03T12:34:10Z

core/src/dird/dird_conf.cc

  static char yes[] = "yes";
  static char no[] = "no";
+
+  static char str[50];


That looks promising! You can just add it to this PR as it is the refactoring required to implement the feature you're working on.

You could

drop the extern "C" everywhere, having C-linkage if you need a std::string probably won't help much

return the plain char*/const char*, compiler will take care of conversion on return

Your solution for edit_job_codes() is probably as good as it gets without refactoring the whole thing. However, for a plain char* snprintf() is better suited than Bsnprintf(), as the latter expects a resizable POOLMEM*.

arogge · 2023-07-04T11:51:32Z

That looks really really good. The only issue I see is the letters you chose.
We already have %m on the FD and it is hard enough to understand without duplicate letters.
As the set of letters to select from is agkoquyzAEGHIJKLMNOQRSTUWXYZ my proposal is to go with %O for the original jobid and %N for new jobid.
But I'm happy with whatever other non-duplicate letters you come up with.

SamuelBoerlin · 2023-07-04T11:59:46Z

Thanks! Okay, makes sense. I'll go with %O and %N then.

SamuelBoerlin · 2023-07-04T12:28:15Z

I've also added %m to the docs because that one was undocumented as far as I can tell.

arogge · 2023-07-13T08:10:42Z

Sorry for keeping you waiting so long.
That changes look good (I'm going to finish the review today). You can (and probably should) drop the CHANGELOG commit. We have a Bot that adds a ChangeLog record based on the PR title automatically.

arogge

These changes look great. Thank you for the effort you put into this!
You can fix the CHANGELOG.md conflict by just removing your change in that file, the Bot will take care of that.
Do you have a need to see this in Bareos 22? I think we could backport these changes if you have a need for them to become available in the next Bareos 22 release...

SamuelBoerlin · 2023-07-13T09:45:24Z

Great! I've removed the changelog changes again.
Yes, it would be nice to have this in the next Bareos 22 release. Currently I get the new jobid by just adding +1 to the copy/migrate control jobid, but that seems unsafe and prone to breaking, so it'd be nice to have this new variable instead.

SamuelBoerlin commented Jun 30, 2023

View reviewed changes

arogge reviewed Jul 3, 2023

View reviewed changes

SamuelBoerlin marked this pull request as ready for review July 4, 2023 10:00

SamuelBoerlin requested a review from arogge July 4, 2023 10:00

arogge changed the title ~~dird: add prev and new jobid variables %m %M~~ dird: add prev and new jobid variables Jul 4, 2023

SamuelBoerlin force-pushed the migration-prev-new-jobid-vars branch from b7a715f to 5883b45 Compare July 6, 2023 19:06

arogge approved these changes Jul 13, 2023

View reviewed changes

SamuelBoerlin force-pushed the migration-prev-new-jobid-vars branch from 5883b45 to 3d0ada2 Compare July 13, 2023 09:38

bruno-at-bareos assigned arogge Jul 27, 2023

arogge mentioned this pull request Aug 5, 2023

dird: add prev and new jobid variables #1518

Merged

15 tasks

arogge added enhancement requires backport to 22 labels Aug 5, 2023

SamuelBoerlin added 11 commits August 5, 2023 13:05

dird: add prev and new jobid variables %m %M

e86a527

change job_code_callback_t to return optional<string>

e1808c1

refactor: remove unnecessary string conversions

8dc1d2e

refactor: remove extern "C" from job code callbacks

5ec88ca

refactor: use snprintf instead of Bsnprintf

c4bf2c7

refactor: change prev/new jobid variable letters

201bdab

docs: add documentation for %O and %N variables

5d58e72

docs: add documentation for %m variable

abb26ce

format

2187cba

update AUTHORS

d294dd9

docs: fix variable order

f5a9472

arogge force-pushed the migration-prev-new-jobid-vars branch from 3d0ada2 to f5a9472 Compare August 5, 2023 11:06

Update CHANGELOG.md

136a8dd

BareosBot merged commit 98dfabd into bareos:master Aug 5, 2023

Conversation

SamuelBoerlin commented Jun 30, 2023 • 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!

SamuelBoerlin Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

arogge Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

arogge Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

arogge Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

arogge commented Jul 4, 2023

Uh oh!

SamuelBoerlin commented Jul 4, 2023

Uh oh!

SamuelBoerlin commented Jul 4, 2023

Uh oh!

arogge commented Jul 13, 2023

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SamuelBoerlin commented Jun 30, 2023 •

edited by arogge

Loading

SamuelBoerlin Jul 3, 2023 •

edited

Loading

SamuelBoerlin commented Jul 3, 2023 •

edited

Loading

SamuelBoerlin commented Jul 13, 2023 •

edited

Loading