-
Notifications
You must be signed in to change notification settings - Fork 101
Remove references to postgresql-evr #3167
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
Remove references to postgresql-evr #3167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this was the only reason why we installed the Satellite repositories and enabled the satellite module on the external DB. Please simply it so it only uses RHEL base OS & AppStream.
I'd also like to see it in the release notes as a headline feature, because it enables managed databases (like in cloud environments) to be used.
9969c52
to
3a439ca
Compare
Which part of the docs does this live in? I'm not seeing any references to Satellite at least in https://docs.theforeman.org/nightly/Installing_Server/index-katello.html#using-external-databases_foreman, which seems to contain all of the external DB docs. |
https://github.com/ekohl/foreman-documentation/blob/move-external-db-params/guides/common/modules/proc_preparing-a-host-for-external-databases.adoc which I think can pretty much go entirely.
|
I see, I thought you meant we advised users to install Satellite repositories on normal Foreman smart proxies. |
@ekohl if I'm not wrong, I think that means we can also stop having users install the Katello repositories as well on external databases. I've removed references to installing our repos from both the Katello and Satellite docs. |
guides/common/modules/proc_preparing-a-host-for-external-databases.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_preparing-a-host-for-external-databases.adoc
Outdated
Show resolved
Hide resolved
2d172de
to
50b7976
Compare
50b7976
to
fd58d72
Compare
Conceptually speaking, it’s strange that a section named "Preparing" includes prerequisites. Because "Preparing" itself is a prerequisite by definition. If you think the two bullets should indeed be considered prerequisites applicable to all the modules in 4.10. Using external databases with Foreman, you can split them off into a separate module and include it at the beginning of assembly_using-external-databases.adoc. See Prerequisites for bare-metal provisioning (source here) for an example of prereqs included at the assembly level. If you feel the two bullets are actually procedure steps, you can merge them with
Then, even if you hide the last step for upstream builds, the rest of the module will still make sense for these builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think you can drop lines 6-9 because now a base RHEL subscription gives you access to the right repositories. Though with service level agreement I'm a bit uncertain.
Do we still need procedure? After removing the specific Satellite repositories, isn't it sufficient to say you need a RHEL 8 or 9 system with an attached subscription? Essentially, drop the whole chapter Preparing a host for external databases
and make them prerequisites in the Installing PostgreSQL
procedure
@asteflova @apinnick what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've ripped out so much of this page that I think I could just include this info as prerequisites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me from the docs perspective. Looking at the procedure steps, it looks like you could save a lot of space by shrinking it into a prerequisite bullet without losing much value. More importantly (to me), anything that's a prerequisite should be labeled as "prerequisite", not "preparing" or anything like that. We are supposed to apply the same wording and terminology consistently (this helps make the content predictable).
=== postgresql-evr extension no longer required | ||
|
||
Installation of the Katello database on remote systems where root access is not available is now possible. | ||
Only a basic PostgreSQL installation is required, which should enable installs on systems like Amazon RDS or Azure Database for PostgreSQL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of scope, but we have a guide https://docs.theforeman.org/nightly/Deploying_Project_on_AWS/index-katello.html and using Amazon RDS would be a good thing to document, after it's been verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maximiliankolb Just FYI, this might be useful to know with regards to your plans for the AWS guide.
It's looking likely that I'll need to also add information for users upgrading their remote databases due to Katello/katello#11098. Looks like https://docs.theforeman.org/nightly/Upgrading_Project/index-katello.html#Upgrading_the_External_Database_upgrading-connected is what will need updating. |
We do. I forgot which extension they use, but it's shipped in |
323cbdc
to
b6e08ea
Compare
b6e08ea
to
478958a
Compare
ifdef::orcharhino[] | ||
* The prepared host has the same content available as your {ProjectServer}. | ||
endif::[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is no longer true. Previously it was needed for postgresql-evr
but now base OS repos should suffice.
ifdef::orcharhino[] | |
* The prepared host has the same content available as your {ProjectServer}. | |
endif::[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The information is wrapped in an orcharhino
check as it was on the old page -- @maximiliankolb does this need to stay for some orcharhino-specific reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Ewoud's suggestion is correct because external DBs no longer need packages from Foreman/Katello.
|
||
Installation of the Katello database on remote systems where root access is not available is now possible. | ||
Only a basic PostgreSQL installation is required. | ||
With this feature, you can now install on systems like Amazon RDS or Azure Database for PostgreSQL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: since Pulp 3.22 uses the HStore extension (which is why you need postgresql-contrib
). It's on both https://docs.aws.amazon.com/AmazonRDS/latest/PostgreSQLReleaseNotes/postgresql-extensions.html and https://learn.microsoft.com/en-us/azure/postgresql/extensions/concepts-extensions-versions#hstore so I'd expect it to indeed work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some suggestions on your PR.
With the following commit, I got the build to work:
commit 63f7aaa499a8f65a7ce5e32592e3443b982e63a3 (HEAD -> pr_3167_ian_external_databases)
Author: Maximilian Kolb <kolb@atix.de>
Date: Wed Dec 18 12:02:43 2024 +0100
PATCH
diff --git a/guides/common/modules/proc_upgrading-the-external-database.adoc b/guides/common/modules/proc_upgrading-the-external-database.adoc
index 9f578a393e..d258060d23 100644
--- a/guides/common/modules/proc_upgrading-the-external-database.adoc
+++ b/guides/common/modules/proc_upgrading-the-external-database.adoc
@@ -15,8 +15,8 @@ If your {Project} deployment uses an external database, the database will be upg
"psql -d foreman -c \"UPDATE pg_extension SET extowner = (SELECT oid FROM pg_authid WHERE rolname='foreman') WHERE extname='evr';\""
----
ifdef::upgrading-connected[]
- . Follow xref:upgrading_a_connected_{project-context}_server_{context}[].
- endif::[]
- ifdef::upgrading-disconnected[]
- . Follow xref:upgrading_a_disconnected_{project-context}_server_{context}[].
- endif::[]
+. Follow xref:upgrading_a_connected_{project-context}_server_{context}[].
+endif::[]
+ifdef::upgrading-disconnected[]
+. Follow xref:upgrading_a_disconnected_{project-context}_server_{context}[].
+endif::[]
@@ -1,25 +1,22 @@ | |||
[id="Upgrading_the_External_Database_{context}"] | |||
= Upgrading the external database | |||
|
|||
You can upgrade an external database from {EL} 8 to {EL} 9 while upgrading {Project} from {ProjectVersionPrevious} to {ProjectVersion}. | |||
If your {Project} deployment uses an external database, the database will be upgraded when running {foreman-installer} on the {Project} server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your {Project} deployment uses an external database, the database will be upgraded when running {foreman-installer} on the {Project} server. | |
If your {ProjectServer} uses an external database, the database will be upgraded when running `{foreman-installer}` on your {ProjectServer}. |
stupid question: can you also use an external DB for a Smart Proxy Server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we support external DBs for smart proxies, but it is technically possible. I don't see docs in the proxy installation guide for external DBs, at least.
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
[id="Upgrading_the_External_Database_Operating_System{context}"] | ||
= Upgrading the external database operating system | ||
|
||
If your {Project} uses an external database, you can upgrade the database from {EL} 8 to {EL} 9 while upgrading {Project} from {ProjectVersionPrevious} to {ProjectVersion}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that still true for Foreman/Katello nightly? I am asking because it does not run on EL8 anymore. Latest release is 3.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The external DB has more flexible requirements than Foreman since it just needs to be a running postgres server, so I think it's still okay that users would have EL8 machines running their DBs for some time. @ekohl can probably keep me honest here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @ianballou has indicated, it sort of has its own lifecycle. Technically I think we should loosen the requirement to any PostgreSQL 13+ compatible server instead of some (RH)EL server running PostgreSQL.
Within the platform team we've had some discussions on the level of support for the external DB server. Now that it's just a plain PostgreSQL server we can consider not having any instructions on setting up the PostgreSQL server. For example, we don't document how you do backups, partition, etc. Where do we draw the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see our external DB documentation being the recommendation that we can provide the best support for. Perhaps we should be strict with what the docs recommend but ensure users know that other solutions may be possible, just with limited support.
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_upgrading-the-external-database-operating-system.adoc
Outdated
Show resolved
Hide resolved
478958a
to
8eca831
Compare
@maximiliankolb I believe I've addressed your comments. I addressed @ekohl 's earlier one too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tiny suggestions, rest LGTM.
guides/common/modules/proc_upgrading-the-external-database.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_upgrading-the-external-database.adoc
Outdated
Show resolved
Hide resolved
8eca831
to
ef0b160
Compare
@maximiliankolb I've added your last suggestions in. @ekohl you've got a change request on here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ian, LGTM!
@ianballou Do you want us to cherry-pick this to 3.13 and 3.12? Your PR states that it's part of Katello 4.14.
Thanks all, no need to do any cherry-picks, that comment is old. Merging since I believe I've addressed the other requested changes. |
Thanks @ianballou! |
Otherwise DB restore using foreman-maintain will fail. Inspired by theforeman#3167
Otherwise DB restore using foreman-maintain will fail. Inspired by #3167
With the merging of Katello/katello#11087 , the evr database type is now being added to postgres directly in Katello. This means users no longer need to install the RPM. So, for Katello 4.14, it will be safe to remove this reference.
Q: do we still need postgresql-contrib? I think we may need it for Pulpcore...
Please cherry-pick my commits into: