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

Mention that the external database should be upgraded #3374

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

Mention that the external database should be upgraded #3374

wants to merge 7 commits into from
+9 −5

Conversation

AkshayGadhaveRH
Copy link
Contributor

The external database should be upgraded from PostgreSQL 12 to 13. Mentioning this in the ext DB upgrade chapter.

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

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.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13
  • 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)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

@AkshayGadhaveRH AkshayGadhaveRH added the Needs tech review Requires a review from the technical perspective label Oct 15, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 15, 2024 •
edited
Loading

The PR preview for aa0714c is available at theforeman-foreman-documentation-preview-pr-3374.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

Lennonka
Lennonka requested changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't it be better as a second prerequisite?
Something like "Install PostgreSQL version 13 on the new RHEL host"?

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Oct 16, 2024
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 17, 2024
@AkshayGadhaveRH
Copy link
Contributor Author

@Lennonka agreed. Makes sense to me.

Lennonka
Lennonka approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

LGTM

@Lennonka Lennonka added tech review done No issues from the technical perspective style review done No issues from docs style/grammar perspective and removed Needs tech review Requires a review from the technical perspective labels Oct 17, 2024
@Lennonka
Copy link
Contributor

I've set both "review done" labels because it's correct that PG 13 is needed for 3.12/6.16 and I have reviewed the style as well.

@asteflova
Copy link
Contributor

I don't think it's a good idea to set tech review done without an actual technical review. This is a new feature that deserves a closer look from Engineering. Also, the latest comment in https://issues.redhat.com/browse/SAT-28273 suggests that there is additional context to consider. I'm unsetting the tech review done label again. (IMHO docs people should not provide tech review for significant updates or new features, that's why we have two separate types of review.)

@AkshayGadhaveRH please work with the Platform team to secure a proper technical review.

@asteflova asteflova added Needs tech review Requires a review from the technical perspective and removed tech review done No issues from the technical perspective labels Oct 17, 2024
evgeni
evgeni requested changes Oct 18, 2024
View reviewed changes
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.

What we need to tell our users (but do not today) is that they have to upgrade PostgreSQL from 12 to 13 on their external DB server during the upgrade to 6.16/3.12(3.11).

We have the "On {EL} 8: Switch to the PostgreSQL 13 module" step -- either before of after that we should say something like "If you're running an external database setup, upgrade the external database to PostgreSQL 13".

guides/common/modules/proc_upgrading-the-external-database.adoc Show resolved Hide resolved
@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Oct 18, 2024
@maximiliankolb maximiliankolb mentioned this pull request Oct 21, 2024
Open
@AkshayGadhaveRH
Copy link
Contributor Author

@evgeni does this plan look good? (also, small question for you)

  1. The documentation should mention that the EL version of the DB should be the same as that of the Satellite.
  2. We should mention that the postgresql version should be upgraded for EL8.

Will this involve creating backup of old (pgsql12 + RHEL 8) and restoring that on fresh (pgsql13 + EL8)?

  1. The external DB upgrade chapter should talk about EL8 to EL9 upgrade of the external DB OS. RHEL9 already has postgresql by default.

Edit existing proc_upgrading-the-external-database.adoc module.

@evgeni
Copy link
Member

evgeni commented Oct 21, 2024

  1. The OS should be updated, but it's not necessary for the upgrade of the main server.
  2. The PostgreSQLmust be upgraded to 13, which is mandatory for the upgrade of the server.
  3. We don't document how these upgrades are to be done, that's up to the local admin.

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 21, 2024
evgeni
evgeni reviewed Oct 21, 2024
View reviewed changes
guides/common/modules/proc_upgrading-a-connected-project-server.adoc Outdated
Comment on lines 188 to 189
ifdef::satellite[]
. If you are using an external database, upgrade to PostgreSQL 13.
endif::[]
Copy link
Member

Choose a reason for hiding this comment

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

This needs to happen before satellite-maintain upgrade check, as that one will already complain about the wrong version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to mention this step twice in one procedure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just move this to the end for both Foreman and Satellite @evgeni ?

Copy link
Member

Choose a reason for hiding this comment

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

Twice in the same procedure? No.
But Foreman/Katello and Satellite procedures are quite different, and each of them needs that step.

Lennonka
Lennonka reviewed Oct 21, 2024
View reviewed changes
guides/common/modules/proc_upgrading-a-connected-project-server.adoc Outdated Show resolved Hide resolved
guides/common/modules/proc_upgrading-a-connected-project-server.adoc Show resolved Hide resolved
guides/common/modules/proc_upgrading-a-connected-project-server.adoc Outdated
Comment on lines 188 to 189
ifdef::satellite[]
. If you are using an external database, upgrade to PostgreSQL 13.
endif::[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to mention this step twice in one procedure?

@Lennonka
Copy link
Contributor

Looks good to me.

@evgeni Do we have a technical ack?

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Oct 24, 2024
Akshay Gadhave added 6 commits October 25, 2024 13:30
Mention that the external database should be upgraded
8a67914 
The external database should be upgraded from PostgreSQL 12 to 13.
Mentioning this in the ext DB upgrade chapter.

JIRA: https://issues.redhat.com/browse/SAT-28273
Rephrase sentence
00dfa4f
Apply changes suggested in PR
556f2eb 
- Changed external DB upgrade chapter to talk about EL8 to 9 upgrade.
- Mentioned that external DB needs to be upgraded to PostgreSQL 13 in the switch to pgsql 13 module step.
- Replaced PostgreSQL 12 with PostgreSQL 13 in the `Setting up external DB` doc.
Add line about pgsql upgrade for Satellite build
37f5df4
Apply changes suggested in PR
ca12851 
- Move PostgreSQL related steps to the end.
- Unify steps for all builds so that there's no need to repeat the steps.
- Add changes for disconnected upgrade guide as well.
Place the instructions in correct position
cc726f1
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evgeni evgeni evgeni requested changes

@Lennonka Lennonka Lennonka approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AkshayGadhaveRH @Lennonka @asteflova @evgeni