Skip to content

Modify production environment backup recommendation #3747

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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AkshayGadhaveRH
Copy link
Contributor

Since online backups are supported for production, after snapshot backups were dropped [1], removing the now irrelevant line about online backups being unsupported. [1] https://issues.redhat.com/browse/SAT-25667

JIRA: https://issues.redhat.com/browse/SAT-29507

What changes are you introducing?

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.14/Katello 4.16
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

@AkshayGadhaveRH AkshayGadhaveRH requested a review from evgeni March 27, 2025 05:25
@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels Mar 27, 2025
@AkshayGadhaveRH AkshayGadhaveRH removed the Needs testing Requires functional testing label Mar 27, 2025
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

style-wise LGTM.

@maximiliankolb maximiliankolb added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Mar 27, 2025
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

Line 4 reads "Perform an online backup only for debugging purposes." -- this should also be gone.

I would also re-word the "risks associated" paragraph, but don't have a good suggestion right now.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Mar 31, 2025
@bangelic
Copy link
Contributor

Line 4 reads "Perform an online backup only for debugging purposes." -- this should also be gone.

I would also re-word the "risks associated" paragraph, but don't have a good suggestion right now.

Just a suggestion to reword the "risks associated" paragraph:

When you perform an online backup, the procedure repeats the Pulp database backup until no processes are altering the database.
Because the Pulp database takes the longest to back up in the {Project} backup, any changes to it during this process restart the backup.

@evgeni
Copy link
Member

evgeni commented Apr 1, 2025

Thanks Brian! This definitely reads easier than the previous version.
My concern however was more about the fact that the whole statement is not actually correct.
It's not the Pulp database (PostgreSQL), it's the Pulp data (/var/lib/pulp) that we watch for modifications and re-try the backup if we notice any.
Additionally (and that's why we consider online backup production ready now) we now ensure that no "normal" operations happen during the backup, so the chances that something changes the pulp data are rather small.

So how about this:

When you perform an online backup, most {Project} services remain running and usable.
To ensure a consistent backup, background workers are shut down and operations that require those remain in pending state.
Additionally the backup process ensures that the Pulp data (`/var/lib/pulp`) is not altered during the backup.
Any changes to the Pulp data during the backup will result in restart of the backup process, taking additional time.

@bangelic
Copy link
Contributor

bangelic commented Apr 1, 2025

Thanks Brian! This definitely reads easier than the previous version. My concern however was more about the fact that the whole statement is not actually correct. It's not the Pulp database (PostgreSQL), it's the Pulp data (/var/lib/pulp) that we watch for modifications and re-try the backup if we notice any. Additionally (and that's why we consider online backup production ready now) we now ensure that no "normal" operations happen during the backup, so the chances that something changes the pulp data are rather small.

So how about this:

When you perform an online backup, most {Project} services remain running and usable.
To ensure a consistent backup, background workers are shut down and operations that require those remain in pending state.
Additionally the backup process ensures that the Pulp data (`/var/lib/pulp`) is not altered during the backup.
Any changes to the Pulp data during the backup will result in restart of the backup process, taking additional time.

Looks good, Evgeni. The only suggestion I would make is to change the last sentence to:
Any changes to the Pulp data during this process restart the backup, taking additional time.

Akshay Gadhave added 2 commits April 15, 2025 10:33
Since online backups are supported for production, after snapshot backups were dropped [1], removing the now irrelevant line about online backups being unsupported.
[1] https://issues.redhat.com/browse/SAT-25667

JIRA: https://issues.redhat.com/browse/SAT-29507
@AkshayGadhaveRH AkshayGadhaveRH force-pushed the update_prod_backup_recommendation branch from 1333291 to ad0f091 Compare April 15, 2025 06:43
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Apr 15, 2025
@AkshayGadhaveRH
Copy link
Contributor Author

Thanks @bangelic ! I have edited the para as per your and Evgeni's recommendation. However, I have kept the last line as per Evgeni's suggestion and tweaked the To ensure a consistent backup, line a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs re-review Needs tech review Requires a review from the technical perspective style review done No issues from docs style/grammar perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants