consolidate: make virtualfull jobs spawned by consolidate job inherit same priority and max concurrent jobs by SamuelBoerlin · Pull Request #1530 · bareos/bareos

SamuelBoerlin · 2023-08-16T15:43:32Z

Thank you for contributing to the Bareos Project!

This PR makes virtualfull jobs spawned by a consolidate job inherit the consolidate job's priority and allow mixed priority flag.
This change puts it more in line with e.g. migration/copy jobs which already behave this way.
It also allows users to give consolidate jobs a lower priority than the original jobs that are being consolidated, so that they don't block more important jobs (i.e. jobs with a higher priority than the consolidate jobs, but still lower priority than the original jobs being consolidated).

Example use case:
I've configured consolidate jobs to have the same priority as migrate/copy jobs and all of those have a lower priority than regular always incremental backups. This is because the consolidate/migrate/copy jobs can end up running for a very long time and they shouldn't block regular backups from happening. The issue with this is, that when a consolidation is started, it spaws virtualfull jobs with the same priority as the always incremental backups, which then end up blocking my migrate/copy jobs. This issue could be solved with this PR.

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

bruno-at-bareos · 2023-08-22T11:30:29Z

Hello @SamuelBoerlin
We detected a case where extending the way consolidate handle the parameter "Spool Data = yes/no" would be useful and maybe not that hard to be included into this PR.

For example if you define a job with "SpoolData=Yes" when the consolidation run the virtual full, this parameter is always applied. If it can make sense on some setup (like consolidating a lots of small incrementals), some other setup with large full jobs maybe want to skip spooling.
So we would like to see consolidate job able to handle and propagate the "SpoolData=Yes|No" parameter so end-users will be able to override this.

SamuelBoerlin · 2023-08-22T16:00:04Z

Hi @bruno-at-bareos
Indeed, that could also be useful. I've pushed another commit for spooldata.

bruno-at-bareos · 2023-08-22T17:51:07Z

Hi @bruno-at-bareos Indeed, that could also be useful. I've pushed another commit for spooldata.

Great thanks. I know some people will be happy.

SamuelBoerlin · 2023-08-28T10:06:02Z

I've also been experimenting with adding support for MaxConcurrentJobs now (master...SamuelBoerlin:bareos:consolidate-max-concurrent-jobs), which currently cannot be configured for consolidate jobs either. Not yet tested, but if it works I might add it to this PR too.

arogge · 2023-09-18T08:19:15Z

Did I see that correctly? The recently pushed changes are for the maximum concurrent jobs change?
Would you consider this being ready for a review?

SamuelBoerlin · 2023-09-25T06:00:46Z

Yes please, thanks!

arogge

This looks pretty good already.
I'm a bit unhappy wit the allow_mixed_priority and allow_mixed_priority_set, as it is hard to grasp what it is supposed to do at first.
I'm pretty confident we can make this work and merge it so it still goes into Bareos 23.

arogge · 2023-09-25T12:01:46Z

core/src/dird/director_jcr_impl.h

  directordaemon::PoolResource* next_pool{};    /**< Next Pool used for migration/copy and virtual backup */
  directordaemon::FilesetResource* fileset{};   /**< FileSet resource */
  directordaemon::CatalogResource* catalog{};   /**< Catalog resource */
+  directordaemon::runtime_job_status_t* rjs{};  /**< Runtime Job Status. May point to the rjs of another resource (e.g. for consolidation vf jobs this points to the rjs of the parent consolidation job's resource) */


I'm not sure this is the right place for this.

I used to have it in the DirectorJcrImpl at first, but then moved it to the Resources struct because I thought it would be a better fit as the rjs is owned by a JobResource.
Should I move it back to DirectorJcrImpl?

arogge · 2023-09-25T12:12:38Z

core/src/dird/ua.h

+  bool allow_mixed_priority = false;
+  bool allow_mixed_priority_set = false;


if I understand correctly then these two bools mimics a tri-state enum like this:

enum class allow_mixed_priority : int { unset = 0, enable, disable }

or maybe more reusable:

enum class maybe_bool : int { unset = 0, enable, disable }

or maybe more in the mindset of modern C++ a std::optional<bool> could be sufficient.

Yes, good point.
However, it just occurred to me that the allowmixedpriority UA run command parameter (and therefore this code too) isn't even needed because I can just grab it later on from the rc.consolidate_job resource which simplifies things.

These two bools have now been removed.

arogge · 2023-09-25T12:16:21Z

core/src/dird/ua.h

  char* since = nullptr;
  char* StoreName = nullptr;
  char* verify_job_name = nullptr;
+  char* consolidate_job_name = nullptr;


do you really need the name and the pointer to the resource stored here?

No, not really, it's just how verify_job and previous_job e.g. work too and so I followed that. But I can use a local variable instead.

Name is now removed.

arogge

Looks great! Thank you for the effort!

arogge

While I'm pretty happy with your changes, I'll have to request changes again. Sadly, in continuous integration your test fails at least sometimes. For the last build it failed on 9 out of 14 platforms. The requirement for a merge is to pass on all platforms.
I'm not sure what exactly went wrong and as I'll be out-of-office next week I won't have much time to look into it.
I would be glad if you could get in touch with me via e-mail (andreas.rogge@bareos.com) so we can discuss how we can provide access to the CI server's logs for you, so you can at least try to sort out the issue yourself.

SamuelBoerlin · 2023-09-29T08:02:10Z

While I'm pretty happy with your changes, I'll have to request changes again. Sadly, in continuous integration your test fails at least sometimes. For the last build it failed on 9 out of 14 platforms. The requirement for a merge is to pass on all platforms. I'm not sure what exactly went wrong and as I'll be out-of-office next week I won't have much time to look into it. I would be glad if you could get in touch with me via e-mail (andreas.rogge@bareos.com) so we can discuss how we can provide access to the CI server's logs for you, so you can at least try to sort out the issue yourself.

I'll look into it. Yes, would be great if I could take a look at the test logs. Sent you an email.

SamuelBoerlin · 2023-10-09T08:17:34Z

@arogge Thanks for sending me the logs. I couldn't figure out the actual cause yet, but for now I've just increased one of the job's wait time from 5s to 15s.

SamuelBoerlin · 2023-10-09T08:57:01Z

To elaborate on why I have a job that waits 15s: my test starts a priority 20 consolidate job, which then consolidates a usually priority 10 job A (but the consolidate job overrides its priority to be 20) that has the 15s wait built in. Once the consolidate VF job for A has spawned, it starts a regular backup job B with priority 15 (i.e. higher priority than the spawned consolidate/VF job of A but lower priority than A usually would have) and then waits for job B to finish. If job A is still running after B finishes then the test passes. If the priority were not correctly overridden by the consolidate job, then job B could only start after the VF of A has already finished, which would result in a test failure.

SamuelBoerlin · 2023-10-10T20:40:45Z

Couldn't reproduce the issue locally so I decided to just rewrite the test which ended up simplifying things anyways.

arogge · 2023-10-12T16:02:51Z

Thank you. Build was successful. I'll take a look at the updated test tomorrow!

Virtualfull jobs spawned by consolidate now inherit the conoslidate job's priority.

with this change that setting can now be overridden

make virtualfull jobs spawned by consolidate job inherit allow_mixed_priority flag from the consolidate job.

make virtualfull jobs spawned by consolidate job inherit spool_data flag

The AllowMixedPriority setting is now instead taken from the parent consolidate job resource.

SamuelBoerlin marked this pull request as ready for review August 16, 2023 17:02

bruno-at-bareos requested a review from arogge August 17, 2023 09:32

bruno-at-bareos assigned arogge Aug 17, 2023

SamuelBoerlin force-pushed the consolidate-job-priority branch from c260ce7 to b779be0 Compare September 16, 2023 19:55

arogge requested changes Sep 25, 2023

View reviewed changes

SamuelBoerlin changed the title ~~consolidate: make virtualfull jobs spawned by consolidate job inherit same priority~~ consolidate: make virtualfull jobs spawned by consolidate job inherit same priority and max concurrent jobs Sep 26, 2023

SamuelBoerlin requested a review from arogge September 26, 2023 09:30

arogge approved these changes Sep 27, 2023

View reviewed changes

arogge requested changes Sep 28, 2023

View reviewed changes

SamuelBoerlin force-pushed the consolidate-job-priority branch from cafafc8 to b7292b5 Compare October 10, 2023 20:35

SamuelBoerlin requested a review from arogge October 12, 2023 11:15

arogge approved these changes Oct 16, 2023

View reviewed changes

SamuelBoerlin added 7 commits October 19, 2023 15:34

dird: add rjs pointer to jcr

2c94d9d

ua_run: pass consolidate job to spawned virtualfull

4ff821e

consolidate: inherit job priority to virtualfull

9a0c3b7

Virtualfull jobs spawned by consolidate now inherit the conoslidate job's priority.

systemtests: add tests for consolidate job priority

050eaa7

dir: add max_concurrent_jobs to jcr dir_impl

ad38e88

with this change that setting can now be overridden

ua_run: apply max_concurrent_jobs from consolidate job

5f8e7eb

dird: move jcr rjs pointer to jcr res

7b3c47c

SamuelBoerlin and others added 14 commits October 19, 2023 16:10

systemtests: consolidate: also check allow mixed priority

f713e89

format

279d5b6

dird: add allow_mixed_priority flag to jcr

d5db698

ua_run: add allowmixedpriority override flag

cad30ca

consolidate: inherit allow_mixed_priority flag

d163c13

make virtualfull jobs spawned by consolidate job inherit allow_mixed_priority flag from the consolidate job.

apply bareos-check-sources

330acc5

consolidate: inherit spool_data flag

6713989

make virtualfull jobs spawned by consolidate job inherit spool_data flag

Merge branch 'consolidate-max-concurrent-jobs'

f990be9

ua_run: remove allowmixedpriority override flag

953582c

The AllowMixedPriority setting is now instead taken from the parent consolidate job resource.

ua: remove consolidate_job_name from RunContext

2c42a37

tests: try increasing wait time to 15s

883bdb8

dird: add allow mixed priority to job summary

5fe35af

tests: rewrite always incremental consolidate test

f89acf1

systemtests: re-add spooling to ai-consolidate test

9dde571

arogge force-pushed the consolidate-job-priority branch from 8d5fb3d to 9dde571 Compare October 19, 2023 14:18

Update CHANGELOG.md

d496c2f

BareosBot merged commit 46ba223 into bareos:master Oct 19, 2023

		bool allow_mixed_priority = false;
		bool allow_mixed_priority_set = false;

Conversation

SamuelBoerlin commented Aug 16, 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!

bruno-at-bareos commented Aug 22, 2023

Uh oh!

SamuelBoerlin commented Aug 22, 2023

Uh oh!

bruno-at-bareos commented Aug 22, 2023

Uh oh!

SamuelBoerlin commented Aug 28, 2023

Uh oh!

arogge commented Sep 18, 2023

Uh oh!

SamuelBoerlin commented Sep 25, 2023

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!

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!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin commented Sep 29, 2023

Uh oh!

SamuelBoerlin commented Oct 9, 2023

Uh oh!

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

Uh oh!

Uh oh!

SamuelBoerlin commented Oct 10, 2023

Uh oh!

arogge commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SamuelBoerlin commented Aug 16, 2023 •

edited by arogge

Loading

SamuelBoerlin commented Oct 9, 2023 •

edited

Loading