Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dird: cats: remove copy and migration jobs with no data from catalog #1262

Merged
merged 14 commits into from
Oct 21, 2022
Merged

dird: cats: remove copy and migration jobs with no data from catalog #1262

merged 14 commits into from
Oct 21, 2022

Conversation

alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Sep 16, 2022 •
edited by pstorz
Loading

Thank you for contributing to the Bareos Project!

Description

Copy and migration jobs always leave empty jobs behind in the catalog even after volume pruning. Those jobs can only be removed manually or with job pruning.
This PR makes sure:

  • the "administrative job" or "migration/copy control job" that spawns the migration/copy jobs is deleted upon completion,
  • the copy jobs are deleted when actual related copies are upgraded to backups,
  • the migration/copy jobs are deleted when their new migrated/copied jobs are pruned

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)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Check backport line
  • Is the PR title usable as CHANGELOG entry?
  • Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.
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
  • bareos-check-sources --since-merge does not report any problems
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

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch from 859e9b5 to e519ca6 Compare September 16, 2022 16:37
@alaaeddineelamri alaaeddineelamri marked this pull request as ready for review September 19, 2022 09:49
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch from 8707e03 to 71230b6 Compare September 19, 2022 11:07
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch 2 times, most recently from 3a7f61d to b578a1e Compare October 5, 2022 12:58
pstorz
pstorz previously requested changes Oct 19, 2022
View reviewed changes
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work! Just some small remarks

Suggestion for commit descripton of b578a1e :

"docs: MigrationAndCopy.rst: clarify that control job gets removed upon completition"

core/src/cats/sql_update.cc Outdated Show resolved Hide resolved
core/src/cats/sql_update.cc Outdated Show resolved Hide resolved
core/src/dird/migrate.cc Show resolved Hide resolved
core/src/dird/ua_prune.cc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/MigrationAndCopy.rst Outdated
@@ -39,7 +39,7 @@ A migration job can be started manually or from a Schedule, like a backup job. I

Normally three jobs are involved during a migration:

- The Migration control Job which starts the migration child Jobs.
- The Migration control Job which starts the migration child Jobs (removed upon completion).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the job also removed if completes with error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we want to add the information that the "migration child jobs" will be removed when the original jobid is removed from the database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the job enters MigrationCleanup(...) in migrate.cc:1659 at the end of the job, it will be deleted.
A migration cleanup is part of the natural cycle of the job, so whether successful or failed, it gets called, and thus the control job gets removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we want to add the information that the "migration child jobs" will be removed when the original jobid is removed from the database?

I'll see where I can fit that information in the docs

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch from b578a1e to 496a848 Compare October 19, 2022 12:41
pstorz
pstorz reviewed Oct 20, 2022
View reviewed changes
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work. Reading the docs about the migration jobs I was not happy with how everything was described so I updated the docs. Could you please check if what I wrote makes sense?

@pstorz pstorz dismissed their stale review October 20, 2022 12:19

I'm happy so far and added docs myself

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch from 3c85fa2 to bff5f4d Compare October 20, 2022 13:49
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch from bff5f4d to ee5d007 Compare October 20, 2022 14:02
@pstorz pstorz merged commit b0bb6d2 into bareos:master Oct 21, 2022
@pstorz pstorz deleted the dev/alaaeddineelamri/master/s5202-remove-jobs-with-no-data branch October 21, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstorz pstorz pstorz approved these changes

Assignees

@pstorz pstorz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alaaeddineelamri @pstorz