Are you still working on this PR as its labeled as draft ?

Are you still working on this PR as its labeled as draft ?

Yes. I wanted to add systemtests to ensure that ignoreduplicatecheck is set for consolidate/migrate/copy jobs and works as intended, but haven't had the time yet.

Hi @sebsura, the PR would now be ready for review whenever you have time.

While this fixes the problem that you cannot start a virtual full job while another normal job is running, this does not fix the reverse: if you have a virtual full running, you still cannot start a normal job.

I think the best approach to fix the second issue is to ignore jobs that ignore duplicates when checking for duplicates.

Do you want to try to fix this ?

While this fixes the problem that you cannot start a virtual full job while another normal job is running, this does not fix the reverse: if you have a virtual full running, you still cannot start a normal job.

I think the best approach to fix the second issue is to ignore jobs that ignore duplicates when checking for duplicates.

Do you want to try to fix this ?

Thanks for taking a look!

Hm, not quite sure I follow. From my understanding IgnoreDuplicateJobChecking already goes both ways, no?
If it is set then the job is ignored during the duplicate checks: https://github.com/bareos/bareos/blob/master/core/src/dird/job.cc#L885-L904

What you're describing is what I'm testing in the added system tests: a consolidate job is started and then a normal job is started while the consolidate virtual full is still running. Usually the normal job would get cancelled. But after this change it is now no longer cancelled.

Just realized that this change might actually cause an issue if you have consolidate jobs that are running for a long time. Currently, if you have a still running consolidate VF and then a new duplicate consolidation VF is started it would be cancelled if you have something like this:

  Allow Duplicate Jobs = no
  Cancel Lower Level Duplicates = yes
  Cancel Queued Duplicates = no
  Cancel Running Duplicates = no

After this change this would of course no longer be the case because duplicate job checking is disabled. I guess duplicate consolidations could still be mitigated with setting MaxConcurrentJobs = 1 in the Consolidate job.

It would probably be better to prevent duplicate/conflicting consolidation jobs in the first place, though. What do you think?
We could perhaps check at this point here https://github.com/bareos/bareos/blob/master/core/src/dird/consolidate.cc#L276 whether there is already another always-incremental VF job running with overlapping vf_jobids.

Hm, not quite sure I follow. From my understanding IgnoreDuplicateJobChecking already goes both ways, no?

You are right!

We could perhaps check at this point here https://github.com/bareos/bareos/blob/master/core/src/dird/consolidate.cc#L276 whether there is already another always-incremental VF job running with overlapping vf_jobids.

Ill have to think about that for a bit. The code is currently not set up in a way where you can inspect another jobs vf_jobids, e.g. there is no lock protecting this member, and this might fail if vf_jobids is null (which is a special case, see GetVfJobids() in vbackup.cc) or if two of these jobs are started almost at the same time.

I would suggest that we check inside AllowDuplicateJob() whether there are any other always incremental jobs that "run" on the same client and have the same fileset (dir_impl->res.client/fileset).

I've now also added a systemtest for the duplicate consolidation job cancellation.

Ah whoops, only just now saw that the always-incremental-consolidate tests have been rewritten. I'll rebase the changes.

The changes look good. I split up your test as well. There is one thing im currently looking into and afterwards ill push my changes.

I squashed the fixup commits as well as fixing the copyright year on your new test (it was 2021-2024 before).
Let me know what you think of the split up test. Otherwise I would be happy to get this merged.

Looks good to me, thanks!

Hi! Are there problems with the tests? Or does the PR need some more changes?

The PR is fine. Some weird errors popped up during the testing run. I am still trying to reproduce the issue.

Just to give you a small update: Your tests found another bug that should be fixed by #1859 . As soon as that pr is merged, this should get merged as well.