MGMT-16090: upgrade assisted-service postgresql from 12 to 13 #8602
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@omer-vishlitzky: This pull request references MGMT-16090 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/L
WalkthroughPostgreSQL image references updated from version 12-c8s to 13-c9s across deployment manifests and source code. A new startup wrapper script added to orchestrate conditional pg_upgrade during container initialization, triggered only on version mismatch detection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omer-vishlitzky The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/dev/postgresql-upgrade.md (1)
1-139: Excellent documentation! Minor style fix needed.The documentation is comprehensive and well-structured, covering:
- Upgrade mechanism using sclorg's POSTGRESQL_UPGRADE
- Hardlink mode benefits (fast, no extra storage)
- Failure handling and recovery procedures
- Version compatibility and upgrade paths
- Deployment strategy considerations
The recovery section clearly explains that most critical data (Clusters, Hosts, InfraEnvs) is reconciled from etcd, limiting data loss to Events and Logs metadata.
Minor fix: Add language specification to fenced code block.
As flagged by markdownlint, the fenced code block at line 134 is missing a language specification.
🔎 Proposed fix
-``` +```text With this container image you can only upgrade from data directory of version '12', not '10'.</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c6921fac54b33ac5296357d1d446a6a2379d302b and 8b4e76cb28025f9aefa19b68f83c653b6df8f7bb. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` (2 hunks) * `docs/dev/postgresql-upgrade.md` (1 hunks) * `internal/controller/controllers/agentserviceconfig_controller.go` (1 hunks) * `internal/controller/controllers/images.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**</summary> **⚙️ CodeRabbit configuration file** > -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Files: - `internal/controller/controllers/agentserviceconfig_controller.go` - `docs/dev/postgresql-upgrade.md` - `internal/controller/controllers/images.go` - `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/dev/postgresql-upgrade.md</summary> 134-134: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>internal/controller/controllers/images.go (1)</summary><blockquote> `43-45`: **LGTM! PostgreSQL 13 upgrade looks correct.** The default image upgrade from PostgreSQL 12 (RHEL 8) to PostgreSQL 13 (RHEL 9) is properly aligned with the upgrade mechanism documented in `docs/dev/postgresql-upgrade.md`. The sclorg postgresql-13-c9s image supports upgrading from PG12 data via the `POSTGRESQL_UPGRADE` environment variable added in the deployment controller. </blockquote></details> <details> <summary>internal/controller/controllers/agentserviceconfig_controller.go (1)</summary><blockquote> `2079-2082`: **LGTM! Automatic upgrade configuration is well-documented.** The `POSTGRESQL_UPGRADE=hardlink` environment variable correctly enables automatic major version upgrades via sclorg's built-in mechanism. When the new PostgreSQL 13 image starts against existing PG12 data, pg_upgrade runs with `--link` for fast, in-place migration using hardlinks. The inline comments and documentation reference are helpful. The deployment's Recreate strategy ensures safe PVC handling during the upgrade. </blockquote></details> <details> <summary>deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml (1)</summary><blockquote> `1148-1148`: **LGTM! Manifest updates are consistent.** Both DATABASE_IMAGE references in the ClusterServiceVersion manifest are correctly updated to PostgreSQL 13: - Line 1148: Infrastructure operator container environment variable - Line 1287: Related images section for OLM The changes align with the default image update in `internal/controller/controllers/images.go`. Also applies to: 1287-1287 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8602 +/- ##
=======================================
Coverage 43.49% 43.49%
=======================================
Files 411 411
Lines 71244 71246 +2
=======================================
+ Hits 30987 30991 +4
+ Misses 37497 37496 -1
+ Partials 2760 2759 -1
🚀 New features to boost your workflow:
|
| Env: []corev1.EnvVar{ | ||
| // POSTGRESQL_UPGRADE enables automatic major version upgrades via sclorg's built-in mechanism. | ||
| // When set to "hardlink", pg_upgrade runs with --link for in place upgrades. | ||
| // See: https://github.com/sclorg/postgresql-container and docs/dev/postgresql-upgrade.md |
There was a problem hiding this comment.
I can't find this docs. Can you update the link please?
What other options is there? Why this wasn't needed before and now it is?
From the description looks good, just trying to understand if we might want to make this overridable by the user.
There was a problem hiding this comment.
As discussed this is just an implementation details and the users should not be aware of it.
In case they would encounter issue we'll run a manual runbook
omer-vishlitzky
force-pushed
the
MGMT-16090-upgrade-postgres-in-mce
branch
from
8b4e76c to
654a69a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/dev/postgresql-upgrade.md (2)
111-114: Consider adding coordination guidance.The documentation references backplane-operator updates but provides minimal details about coordination requirements. Consider adding a note about ensuring these updates are properly synchronized or providing links to backplane-operator documentation.
134-137: Add language specifier to fenced code block.The fenced code block should specify a language identifier to satisfy markdownlint and improve rendering. Since this is plain text output, use
textorconsoleas the language specifier.🔎 Proposed fix
-``` +```text With this container image you can only upgrade from data directory of version '12', not '10'.</details> As per static analysis hints, this addresses markdownlint rule MD040 (fenced-code-language). </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8b4e76cb28025f9aefa19b68f83c653b6df8f7bb and 654a69a524bd4a6446a802c0a15dbe8f5e5c74a0. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` (2 hunks) * `docs/dev/postgresql-upgrade.md` (1 hunks) * `internal/controller/controllers/agentserviceconfig_controller.go` (1 hunks) * `internal/controller/controllers/images.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * internal/controller/controllers/images.go * internal/controller/controllers/agentserviceconfig_controller.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**</summary> **⚙️ CodeRabbit configuration file** > -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Files: - `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` - `docs/dev/postgresql-upgrade.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/dev/postgresql-upgrade.md</summary> 134-134: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (6)</summary><blockquote> <details> <summary>deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml (2)</summary><blockquote> `1148-1148`: **LGTM!** The DATABASE_IMAGE update to PostgreSQL 13 on RHEL 9 is correct and aligns with the PR objective. --- `1287-1287`: **LGTM!** The relatedImages entry correctly matches the DATABASE_IMAGE environment variable, maintaining consistency across the manifest. </blockquote></details> <details> <summary>docs/dev/postgresql-upgrade.md (4)</summary><blockquote> `1-41`: **LGTM!** The overview and upgrade mechanism explanation is clear and comprehensive. The verification command and hardlink mode benefits are well-documented. --- `43-77`: **LGTM!** The failure handling section appropriately documents the recovery procedure and clearly warns about permanent data loss for Events and Logs metadata. The recovery steps are correct and actionable. --- `79-92`: **LGTM!** The upgrade path documentation clearly shows version compatibility and appropriately notes that PostgreSQL binary compatibility is what matters, not the base OS. --- `118-128`: **LGTM!** The deployment strategy explanation clearly documents why Recreate is used and prevents potential PVC deadlock issues. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
omer-vishlitzky
force-pushed
the
MGMT-16090-upgrade-postgres-in-mce
branch
from
654a69a to
5470939
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/controllers/agentserviceconfig_controller.go (1)
2079-2083: Consider making POSTGRESQL_UPGRADE configurable.The hardcoded
POSTGRESQL_UPGRADE="hardlink"value works well for the standard upgrade path. However, based on the past review comment, consider whether this should be configurable via the AgentServiceConfig spec to support:
- Users who want to use
copymode instead ofhardlink(slower but preserves original data)- Users who want to disable automatic upgrades (
POSTGRESQL_UPGRADE="")- Troubleshooting scenarios where different upgrade modes might be needed
This would provide more flexibility while keeping "hardlink" as the sensible default.
Do you want me to generate a proposal for making this configurable through the AgentServiceConfig spec?
docs/dev/postgresql-upgrade.md (1)
144-148: Add language identifier to fenced code block.The code block at line 144 is missing a language specification. Add a language identifier to ensure proper rendering:
🔎 Suggested fix
-``` +```text With this container image you can only upgrade from data directory of version '12', not '10'.</details> Based on static analysis hints. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 654a69a524bd4a6446a802c0a15dbe8f5e5c74a0 and 5470939ec4f2d83e07acf1c9cfe351abab80a68a. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` (2 hunks) * `docs/dev/postgresql-upgrade.md` (1 hunks) * `internal/controller/controllers/agentserviceconfig_controller.go` (1 hunks) * `internal/controller/controllers/images.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * internal/controller/controllers/images.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**</summary> **⚙️ CodeRabbit configuration file** > -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Files: - `internal/controller/controllers/agentserviceconfig_controller.go` - `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` - `docs/dev/postgresql-upgrade.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/dev/postgresql-upgrade.md</summary> 144-144: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>docs/dev/postgresql-upgrade.md (1)</summary><blockquote> `1-149`: **LGTM! Comprehensive upgrade documentation.** The documentation thoroughly covers the PostgreSQL upgrade mechanism, including: - Clear explanation of the sclorg upgrade mechanism with `POSTGRESQL_UPGRADE=hardlink` - Detailed failure handling and recovery procedures - Helpful tables showing upgrade paths and data loss scenarios - Version skip protection explanation This will be valuable for operators managing assisted-service deployments. </blockquote></details> <details> <summary>deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml (1)</summary><blockquote> `1148-1148`: **LGTM! PostgreSQL image version updated consistently.** The PostgreSQL image has been correctly updated from version 12 to 13 in both locations: - `DATABASE_IMAGE` environment variable (line 1148) - `relatedImages` section (line 1287) The upgrade path from `postgresql-12-c8s` to `postgresql-13-c9s` is supported by sclorg's upgrade mechanism (as documented in `docs/dev/postgresql-upgrade.md`), and the `POSTGRESQL_UPGRADE=hardlink` environment variable added in the controller will handle the automatic migration. Also applies to: 1287-1287 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
omer-vishlitzky
force-pushed
the
MGMT-16090-upgrade-postgres-in-mce
branch
from
5470939 to
0445c42
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/podman/pod-persistent.yml (1)
8-18: Add POSTGRESQL_UPGRADE environment variable for persistent deployment.The PostgreSQL image has been updated to version 13 in a persistent deployment using PVC storage. The
POSTGRESQL_UPGRADE=hardlinkenvironment variable must be added to enable automatic data migration from PostgreSQL 12 to 13.Without this variable, the PostgreSQL 13 container may fail to start or fail to properly migrate existing data from the persistent volume. Add this environment variable to the
configConfigMap referenced by the db container'senvFrom.
♻️ Duplicate comments (2)
deploy/podman/pod-persistent-disconnected.yml (1)
11-11: PostgreSQL image updated; verify upgrade env var not needed.Same concern as noted in deploy/podman/pod.yml: the image is updated but
POSTGRESQL_UPGRADEenvironment variable is not set. Since this pod uses persistent storage (pg-data volume), verify whether the environment variable should be added if this deployment might be used with existing PostgreSQL 12 data.deploy/podman/pod_tls.yml (1)
11-11: PostgreSQL image updated; verify upgrade env var not needed.Same concern as noted in deploy/podman/pod.yml: the image is updated but
POSTGRESQL_UPGRADEenvironment variable is not set. Since this pod uses persistent storage, verify whether the environment variable should be added.
🧹 Nitpick comments (1)
docs/dev/postgresql-upgrade.md (1)
144-147: Add language specification to fenced code block.The static analysis tool correctly identified that this code block is missing a language specification. Add
textor an appropriate language identifier to the opening fence.🔎 Proposed fix
-``` +```text With this container image you can only upgrade from data directory of version '12', not '10'.</details> Based on static analysis hints from markdownlint-cli2. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5470939ec4f2d83e07acf1c9cfe351abab80a68a and 0445c42e2746bdcfdd83f6685b09fea2edacaffc. </details> <details> <summary>📒 Files selected for processing (15)</summary> * `Makefile` (1 hunks) * `config/manager/manager.yaml` (1 hunks) * `config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml` (1 hunks) * `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` (2 hunks) * `deploy/podman/pod-persistent-disconnected.yml` (1 hunks) * `deploy/podman/pod-persistent.yml` (1 hunks) * `deploy/podman/pod.yml` (1 hunks) * `deploy/podman/pod_tls.yml` (1 hunks) * `deploy/postgres/postgres-deployment-ephemeral.yaml` (1 hunks) * `deploy/postgres/postgres-deployment.yaml` (1 hunks) * `docs/dev/postgresql-upgrade.md` (1 hunks) * `internal/common/common_unitest_db.go` (1 hunks) * `internal/common/testcontainers_db_context.go` (1 hunks) * `internal/controller/controllers/agentserviceconfig_controller.go` (1 hunks) * `internal/controller/controllers/images.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**</summary> **⚙️ CodeRabbit configuration file** > -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity. Files: - `internal/common/common_unitest_db.go` - `deploy/postgres/postgres-deployment.yaml` - `deploy/podman/pod-persistent-disconnected.yml` - `internal/controller/controllers/images.go` - `Makefile` - `deploy/podman/pod_tls.yml` - `config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml` - `deploy/podman/pod.yml` - `deploy/podman/pod-persistent.yml` - `internal/common/testcontainers_db_context.go` - `internal/controller/controllers/agentserviceconfig_controller.go` - `deploy/postgres/postgres-deployment-ephemeral.yaml` - `deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml` - `config/manager/manager.yaml` - `docs/dev/postgresql-upgrade.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/dev/postgresql-upgrade.md</summary> 144-144: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (14)</summary><blockquote> <details> <summary>config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml (1)</summary><blockquote> `184-184`: **LGTM: PostgreSQL image reference updated correctly.** The image reference has been updated from PostgreSQL 12 on RHEL 8 to PostgreSQL 13 on RHEL 9, which aligns with the PR's objective to upgrade the database version. </blockquote></details> <details> <summary>docs/dev/postgresql-upgrade.md (1)</summary><blockquote> `1-148`: **Consider documenting alternative upgrade modes and the rationale for hardlink.** In response to the past review comment asking about other options and user configurability: The sclorg PostgreSQL container supports different `pg_upgrade` modes: - `hardlink` (chosen here): Uses `--link` flag for fast, in-place upgrades with no extra storage - `copy` mode: Copies data files (slower, requires 2x storage) The hardlink mode is an appropriate default because it's fast and space-efficient. However, you may want to document: 1. Why hardlink was chosen as the default 2. Whether there are edge cases where copy mode would be preferable 3. Whether user configurability should be considered for future enhancements The documentation is comprehensive otherwise. Consider adding a brief section explaining the rationale for using hardlink mode by default. </blockquote></details> <details> <summary>deploy/podman/pod.yml (1)</summary><blockquote> `9-15`: **Verify POSTGRESQL_UPGRADE env var is not needed for podman pods.** The PostgreSQL image has been updated to version 13, but the `POSTGRESQL_UPGRADE` environment variable is not set in this pod spec. This differs from the kubernetes deployment (deploy/postgres/postgres-deployment.yaml) and the controller-generated deployment (agentserviceconfig_controller.go), which both include `POSTGRESQL_UPGRADE=hardlink`. If this podman pod might be used with existing PostgreSQL 12 data, the upgrade won't happen automatically. Please verify: 1. Are these podman pods only for fresh installs? 2. Should `POSTGRESQL_UPGRADE=hardlink` be added to the db container's envFrom or env section? The same concern applies to deploy/podman/pod-persistent-disconnected.yml and deploy/podman/pod_tls.yml, which also lack this environment variable. </blockquote></details> <details> <summary>deploy/postgres/postgres-deployment.yaml (2)</summary><blockquote> `18-18`: **LGTM: PostgreSQL image updated correctly.** The container image has been updated from PostgreSQL 12 to PostgreSQL 13, matching the PR's upgrade objective. --- `23-24`: **LGTM: POSTGRESQL_UPGRADE environment variable added correctly.** The `POSTGRESQL_UPGRADE=hardlink` environment variable enables automatic major version upgrades using the sclorg container's built-in mechanism. The hardlink mode provides fast, in-place upgrades with no extra storage required, as documented in docs/dev/postgresql-upgrade.md. </blockquote></details> <details> <summary>config/manager/manager.yaml (1)</summary><blockquote> `38-38`: **LGTM: DATABASE_IMAGE environment variable updated correctly.** The infrastructure operator's DATABASE_IMAGE environment variable has been updated to reference PostgreSQL 13 on RHEL 9, ensuring the operator deploys the correct database version. </blockquote></details> <details> <summary>internal/controller/controllers/agentserviceconfig_controller.go (1)</summary><blockquote> `2079-2083`: **LGTM: POSTGRESQL_UPGRADE environment variable implementation is correct and well-documented.** The addition of the `POSTGRESQL_UPGRADE=hardlink` environment variable to the PostgreSQL container enables automatic major version upgrades using the sclorg container's built-in mechanism. The inline comments provide excellent context and reference the new documentation file. Regarding the past review comment: - The documentation link now points to the correct file being added in this PR (docs/dev/postgresql-upgrade.md) - The upgrade mechanism is necessary because PostgreSQL's on-disk format changes between major versions, requiring data migration - The hardlink mode is hardcoded as a sensible default (fast, no extra storage). While user configurability could be considered for future enhancements, the current implementation is appropriate for the common case </blockquote></details> <details> <summary>internal/controller/controllers/images.go (1)</summary><blockquote> `43-45`: **LGTM: PostgreSQL 13 image reference updated correctly.** The default database image has been updated from PostgreSQL 12 to 13, aligning with the repository-wide upgrade. The change is straightforward and maintains the existing function interface. </blockquote></details> <details> <summary>internal/common/common_unitest_db.go (1)</summary><blockquote> `130-142`: **LGTM: Test container image updated to PostgreSQL 13.** The PostgreSQL container image for the Kubernetes-based test database has been correctly updated. Since this test context creates fresh database instances, no upgrade mechanism is required. </blockquote></details> <details> <summary>deploy/postgres/postgres-deployment-ephemeral.yaml (1)</summary><blockquote> `16-20`: **LGTM: Ephemeral PostgreSQL deployment updated correctly.** The PostgreSQL image has been updated to version 13. Since this is an ephemeral deployment with in-memory storage, no upgrade mechanism is required as the database is recreated fresh on each deployment. </blockquote></details> <details> <summary>internal/common/testcontainers_db_context.go (1)</summary><blockquote> `18-29`: **LGTM: Test container image updated to PostgreSQL 13.** The Docker test container image has been correctly updated. Since test containers create fresh database instances, the upgrade mechanism is not applicable here. </blockquote></details> <details> <summary>Makefile (1)</summary><blockquote> `42-42`: **LGTM: Makefile PostgreSQL image updated correctly.** The default PSQL_IMAGE variable has been updated to PostgreSQL 13, ensuring consistency between development, testing, and production environments. </blockquote></details> <details> <summary>deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml (2)</summary><blockquote> `1143-1149`: **LGTM: Operator DATABASE_IMAGE environment variable updated correctly.** The DATABASE_IMAGE environment variable for the infrastructure operator has been updated to PostgreSQL 13, ensuring the operator deploys the correct database version. --- `1282-1288`: **LGTM: Related image reference updated correctly.** The postgresql image reference in the related images list has been updated to version 13, maintaining consistency with the DATABASE_IMAGE environment variable. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
omer-vishlitzky
force-pushed
the
MGMT-16090-upgrade-postgres-in-mce
branch
from
0445c42 to
0c55875
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (18)
Makefile(1 hunks)config/manager/manager.yaml(1 hunks)config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml(1 hunks)deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml(2 hunks)deploy/podman/configmap.yml(1 hunks)deploy/podman/pod-persistent-disconnected.yml(1 hunks)deploy/podman/pod-persistent.yml(1 hunks)deploy/podman/pod.yml(1 hunks)deploy/podman/pod_tls.yml(1 hunks)deploy/postgres/postgres-deployment-ephemeral.yaml(1 hunks)deploy/postgres/postgres-deployment.yaml(1 hunks)docs/dev/README.md(1 hunks)docs/dev/postgresql-upgrade.md(1 hunks)docs/dev/testing.md(1 hunks)internal/common/common_unitest_db.go(1 hunks)internal/common/testcontainers_db_context.go(1 hunks)internal/controller/controllers/agentserviceconfig_controller.go(1 hunks)internal/controller/controllers/images.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/common/common_unitest_db.go
- deploy/podman/pod-persistent-disconnected.yml
- deploy/postgres/postgres-deployment.yaml
- internal/controller/controllers/agentserviceconfig_controller.go
- internal/controller/controllers/images.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/dev/README.mddocs/dev/testing.mdinternal/common/testcontainers_db_context.goMakefiledocs/dev/postgresql-upgrade.mddeploy/podman/pod_tls.ymldeploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yamldeploy/podman/configmap.ymlconfig/manager/manager.yamldeploy/postgres/postgres-deployment-ephemeral.yamlconfig/manifests/bases/assisted-service-operator.clusterserviceversion.yamldeploy/podman/pod.ymldeploy/podman/pod-persistent.yml
🔇 Additional comments (17)
docs/dev/README.md (1)
39-39: LGTM!The documentation correctly reflects the PostgreSQL image upgrade to version 13-c9s.
internal/common/testcontainers_db_context.go (1)
23-23: LGTM!The test container image update to PostgreSQL 13-c9s is correct. Test containers start fresh each time, so no upgrade mechanism is needed.
deploy/postgres/postgres-deployment-ephemeral.yaml (1)
18-18: LGTM!The image update is correct for the ephemeral deployment. Since this deployment uses ephemeral storage (emptyDir), no upgrade mechanism is needed—the container starts fresh each time.
deploy/podman/pod-persistent.yml (1)
11-11: LGTM!The PostgreSQL image upgrade is correct. The container properly references the configmap (via
envFromat lines 13-15), which includes thePOSTGRESQL_UPGRADE: hardlinksetting needed to trigger the in-place upgrade for persistent storage.Makefile (1)
42-42: LGTM!The default PSQL_IMAGE update to PostgreSQL 13-c9s is correct and maintains consistency with other deployment configurations.
deploy/podman/configmap.yml (1)
28-28: LGTM!The addition of
POSTGRESQL_UPGRADE: hardlinkcorrectly enables the in-place upgrade mechanism. When the PostgreSQL 13 container starts against existing v12 data, sclorg'srun-postgresqlscript will invokepg_upgrade --linkto migrate the data efficiently using hardlinks.deploy/podman/pod_tls.yml (1)
11-11: LGTM!The PostgreSQL image update is correct. The container references the configmap (via
envFromat lines 14-15), ensuring the upgrade mechanism is properly configured.docs/dev/testing.md (1)
7-7: LGTM!The documentation correctly reflects the PostgreSQL test container image upgrade to version 13-c9s, matching the actual code changes.
config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml (1)
184-185: LGTM!The PostgreSQL image reference update in the relatedImages section is correct and consistent with the broader upgrade.
deploy/podman/pod.yml (1)
9-16: LGTM!The PostgreSQL container image update is correct. The container will automatically handle the upgrade from PG12 to PG13 data when it starts, as configured via the
POSTGRESQL_UPGRADEenvironment variable in the referenced ConfigMap.docs/dev/postgresql-upgrade.md (5)
1-41: LGTM!The overview and upgrade mechanism description is technically accurate and clearly explains how the sclorg PostgreSQL upgrade works with hardlink mode.
42-87: LGTM!The failure handling and recovery guidance is comprehensive and accurate. The data loss table correctly identifies which data can be reconciled from CRs versus what's PostgreSQL-only.
105-127: LGTM!The upgrade instructions are clear and comprehensive, covering both the assisted-service repository and downstream backplane-operator changes.
128-149: LGTM!The deployment strategy and version skip protection sections accurately describe the safeguards in place to ensure reliable upgrades.
88-104: No action needed; the upgrade path table is complete and accurate. PostgreSQL 14 images do not exist in the sclorg repository, so its absence from the table is correct.Likely an incorrect or invalid review comment.
deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml (2)
1147-1148: LGTM!The DATABASE_IMAGE environment variable update is correct and consistent with the base manifest.
1287-1288: LGTM!The relatedImages postgresql entry update is correct and maintains consistency across the manifest.
| - name: DATABASE_IMAGE | ||
| value: quay.io/sclorg/postgresql-12-c8s:latest | ||
| value: quay.io/sclorg/postgresql-13-c9s:latest |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
quay.io/sclorg/postgresql-13-c9s latest image registry
💡 Result:
The sclorg CentOS-stream images live on Quay under quay.io/sclorg. The repo for PostgreSQL is maintained here and CentOS‑Stream‑9 images include postgresql-13 (tag suffix c9s). See the project README. [1]
Note: upstream notes state the project removed some "latest" tags in favor of explicit c9s tags (so quay.io/sclorg/postgresql-13-c9s:latest may not be present). Use the explicit tag (c9s) or check the repository on Quay to confirm available tags. Example pull commands:
- podman pull quay.io/sclorg/postgresql-13-c9s:c9s
- podman pull quay.io/sclorg/postgresql-15-c9s:latest (example of current latest-tag usage). [1][2]
Sources:
[1] sclorg/postgresql-container README (Quay repos described).
[2] sclorg/related discussion/announcements noting "latest" tag removal in favor of c9s.
🏁 Script executed:
# Search for any PostgreSQL documentation or constraints in the repo
find . -type f \( -name "*.md" -o -name "*.yaml" -o -name "*.yml" \) | head -20 | xargs grep -l -i "postgresql\|database.*image" 2>/dev/null | head -10Repository: openshift/assisted-service
Length of output: 81
🏁 Script executed:
# Check the manager.yaml file for context
cat -n config/manager/manager.yamlRepository: openshift/assisted-service
Length of output: 12515
Replace :latest tag with explicit :c9s tag.
The quay.io/sclorg/postgresql-13-c9s:latest reference is incorrect. The sclorg project removed "latest" tags in favor of explicit version tags. Use quay.io/sclorg/postgresql-13-c9s:c9s instead.
🤖 Prompt for AI Agents
In config/manager/manager.yaml around lines 37 to 38, the DATABASE_IMAGE value
uses the removed :latest tag; update the image value to use the explicit tag by
replacing quay.io/sclorg/postgresql-13-c9s:latest with
quay.io/sclorg/postgresql-13-c9s:c9s so the manifest references the supported
explicit tag.
There was a problem hiding this comment.
this image does exist.
❯ podman pull quay.io/sclorg/postgresql-13-c9s:latest
Trying to pull quay.io/sclorg/postgresql-13-c9s:latest...
Getting image source signatures
Copying blob 93d0d641f1b2 skipped: already exists
Copying blob 5dce88cbadbe done |
Copying blob da80ec85690c done |
Copying config 6975cf9771 done |
Writing manifest to image destination
6975cf9771977fc69e643df3067ace9c5e0bd4656a36307be6c03be59020aa10
There was a problem hiding this comment.
@omer-vishlitzky Thank you for verifying! You're absolutely right—the image exists and can be pulled successfully. I apologize for the incorrect information in my review. The quay.io/sclorg/postgresql-13-c9s:latest tag is valid and your change is correct. No action needed here.
|
/lgmt |
openshift-ci
bot
added
the
do-not-merge/hold
|
please unhold when tested |
|
also thank you for clarifying that we can jump OS and PG version at once ❤️ |
|
/retest |
1 similar comment
|
/retest |
See docs/dev/postgresql-upgrade.md for details on the upgrade mechanism, auto-recovery behavior, and future version roadmap.
omer-vishlitzky
force-pushed
the
MGMT-16090-upgrade-postgres-in-mce
branch
from
0c55875 to
d3054ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/controllers/postgres_startup.sh (1)
1-31: The conditional upgrade logic is well-designed.The script correctly handles all scenarios (fresh install, same version restart, version mismatch upgrade) by conditionally enabling
pg_upgradeonly when needed. This elegantly solves the problem of the sclorg container failing whenPOSTGRESQL_UPGRADEis set permanently.Consider making PGDATA more defensive by respecting any existing environment variable:
🔎 Suggested enhancement for PGDATA handling
-PGDATA=/var/lib/pgsql/data/userdata +PGDATA=${PGDATA:-/var/lib/pgsql/data/userdata}This allows the script to work even if the sclorg container's PGDATA default changes in future versions, while maintaining the current behavior when PGDATA is not set.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
Makefile(1 hunks)config/manager/manager.yaml(1 hunks)config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml(1 hunks)deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml(2 hunks)deploy/podman/pod-persistent-disconnected.yml(1 hunks)deploy/podman/pod-persistent.yml(1 hunks)deploy/podman/pod.yml(1 hunks)deploy/podman/pod_tls.yml(1 hunks)deploy/postgres/postgres-deployment-ephemeral.yaml(1 hunks)deploy/postgres/postgres-deployment.yaml(1 hunks)docs/dev/README.md(1 hunks)docs/dev/postgresql-upgrade.md(1 hunks)docs/dev/testing.md(1 hunks)internal/common/common_unitest_db.go(1 hunks)internal/common/testcontainers_db_context.go(1 hunks)internal/controller/controllers/agentserviceconfig_controller.go(1 hunks)internal/controller/controllers/images.go(1 hunks)internal/controller/controllers/postgres_startup.sh(1 hunks)internal/controller/controllers/postgres_startup_script.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- deploy/podman/pod-persistent-disconnected.yml
- deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml
- internal/common/testcontainers_db_context.go
- internal/common/common_unitest_db.go
- Makefile
- config/manifests/bases/assisted-service-operator.clusterserviceversion.yaml
- internal/controller/controllers/agentserviceconfig_controller.go
- deploy/postgres/postgres-deployment-ephemeral.yaml
- internal/controller/controllers/images.go
- deploy/podman/pod_tls.yml
- config/manager/manager.yaml
- deploy/postgres/postgres-deployment.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/dev/testing.mdinternal/controller/controllers/postgres_startup_script.gointernal/controller/controllers/postgres_startup.shdocs/dev/README.mddeploy/podman/pod-persistent.ymldeploy/podman/pod.ymldocs/dev/postgresql-upgrade.md
🪛 markdownlint-cli2 (0.18.1)
docs/dev/postgresql-upgrade.md
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
docs/dev/testing.md (1)
7-7: LGTM!The PostgreSQL image version update from 12-c8s to 13-c9s is consistent with the PR's upgrade objective and aligns with changes across other deployment manifests.
docs/dev/README.md (1)
39-39: LGTM!The image reference update is consistent with the PostgreSQL 13 upgrade and matches changes in deployment manifests.
deploy/podman/pod.yml (1)
11-11: LGTM!The image version update in the Pod manifest is correct and consistent with the PostgreSQL 13 upgrade.
deploy/podman/pod-persistent.yml (1)
11-11: LGTM!The image version update is correct and maintains consistency across all Pod manifests.
internal/controller/controllers/postgres_startup_script.go (1)
1-13: LGTM!The go:embed implementation is clean and properly exposes the startup script. The documentation clearly explains the purpose and references the detailed upgrade documentation.
docs/dev/postgresql-upgrade.md (1)
1-154: Excellent comprehensive documentation.The documentation thoroughly covers the PostgreSQL upgrade process, including the problem, solution, workflow, failure handling, and recovery procedures. The tables and examples are clear and helpful for developers and operators.
| ``` | ||
| With this container image you can only upgrade from data directory | ||
| of version '12', not '10'. | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Add language identifier to fenced code block.
The fenced code block is missing a language identifier, which causes a linting warning and reduces syntax highlighting.
🔎 Proposed fix
-```
+```text
With this container image you can only upgrade from data directory
of version '12', not '10'.</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/dev/postgresql-upgrade.md around lines 155 to 159, the fenced code block
lacks a language identifier which triggers a lint warning and disables syntax
highlighting; update the opening fence to include a language identifier (for
example use ```text) so the block becomes ```text and leave the content
unchanged, ensuring the linter recognizes the code block language.
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
Should we run the upgrade script in other scenarios as well? Like podman?
|
Outside of figuring out if we want to run the script in scenarios other than operator-managed use cases I'm fine with these changes. Sort out if you want to be able to upgrade podman users and don't wait on my approval to merge. |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
@omer-vishlitzky: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
Upgrade PostgreSQL from 12 to 13 using sclorg's native upgrade mechanism.
Changes:
postgresql-13-c9s(PG13 on RHEL9)POSTGRESQL_UPGRADE=hardlinkenv var to postgres containerdocs/dev/postgresql-upgrade.mdHow it works:
When the new image starts against existing PG12 data, sclorg's
run-postgresqlscript automatically runspg_upgrade --linkto migrate the data in-place. The hardlink mode is fast (seconds) and requires no extra storage.Test plan