add POSTGRES_HOST_RO to core host if using cnpg cluster #1725
Conversation
WalkthroughAdds a new POSTGRES_HOST_RO environment variable to the core StatefulSet manifest, updates the embedded bundle checksum, and adds logic to set POSTGRES_HOST_RO to the CNPG cluster name suffixed with "-ro" when reconciling CNPG-managed Postgres. Changes
Sequence DiagramsequenceDiagram
participant Reconciler as Reconciler\n(phase2_creating)
participant CNPG as CNPGCluster object
participant Core as Desired Core StatefulSet Env
rect rgb(200,220,255)
Note over Reconciler: Gate - only if shouldReconcileCNPGCluster()
Reconciler->>Reconciler: shouldReconcileCNPGCluster() == true
end
rect rgb(220,240,220)
Note over Reconciler,CNPG: Derive RO host
Reconciler->>CNPG: Read CNPGCluster.Name
CNPG-->>Reconciler: returns <cluster-name>
Reconciler->>Reconciler: compute roHost = <cluster-name> + "-ro"
end
rect rgb(240,240,200)
Note over Reconciler,Core: Configure env
Reconciler->>Core: Set env POSTGRES_HOST_RO = roHost
Core-->>Reconciler: Desired spec updated
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(2 hunks)pkg/system/phase2_creating.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-02T04:34:47.006Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-operator PR: 1607
File: pkg/bundle/deploy.go:0-0
Timestamp: 2025-07-02T04:34:47.006Z
Learning: Default values for `METRICS_AUTH_ENABLED` and `VERSION_AUTH_ENABLED` are set up under `noobaa-config` in the NooBaa operator.
Applied to files:
deploy/internal/statefulset-core.yamlpkg/bundle/deploy.go
🧬 Code graph analysis (1)
pkg/system/phase2_creating.go (1)
pkg/util/util.go (1)
KubeList(646-667)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: golangci-lint
- GitHub Check: run-hac-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
🔇 Additional comments (3)
deploy/internal/statefulset-core.yaml (1)
122-122: LGTM! Environment variable correctly added.The
POSTGRES_HOST_ROenvironment variable is properly added to the core container's environment. The empty value is intentional, as it will be dynamically populated during reconciliation inpkg/system/phase2_creating.go.pkg/bundle/deploy.go (2)
5296-5296: SHA bump looks consistent with the YAML change.Sha256_deploy_internal_statefulset_core_yaml updated; matches the added env below. No action required.
5419-5420: Implementation correctly validates reconciliation and fallback—no issues found.The codebase confirms all review concerns:
Phase2 reconciliation for POSTGRES_HOST_RO:
phase2_creating.go:455-464conditionally populates this env only whenshouldReconcileCNPGCluster()is true, correctly querying services with label selectorapp=noobaa,cnpg.io/cluster=noobaa-db-pg-clusterand selecting the replica service bycnpg.io/instanceRole == "replica"selector.Core fallback tolerance: When CNPG is not enabled, the conditional block is skipped entirely, leaving POSTGRES_HOST_RO unset by design. This implicit tolerance is working as intended.
Architecture documentation:
doc/noobaa-cnpg-integration.mddocuments that read-only and read-write services are automatically created and managed by CNPG, confirming the integration design is sound.The optional e2e/unit test suggestion is a quality-of-life improvement but not required—the current implementation correctly handles both enabled and disabled CNPG scenarios.
| case "POSTGRES_HOST_RO": | ||
| if r.shouldReconcileCNPGCluster() { | ||
| //RO host is not part of the cnpg app secret, so we're getting it here from the service | ||
| services := &corev1.ServiceList{} | ||
| cnpg_label, _ := labels.Parse("app=noobaa,cnpg.io/cluster=noobaa-db-pg-cluster") | ||
| util.KubeList(services, &client.ListOptions{Namespace: r.NooBaaPostgresDB.Namespace, LabelSelector: cnpg_label}) | ||
| for _, service := range services.Items { | ||
| if (service.Spec.Selector["cnpg.io/instanceRole"] == "replica") { | ||
| c.Env[j].Value = service.Name | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix critical error handling and service discovery issues.
This code has several critical issues that could cause runtime failures or silent misconfigurations:
-
Line 460: Error from
labels.Parseis silently ignored. If parsing fails,cnpg_labelwill be invalid, leading to incorrect service queries. -
Line 460: Return value of
util.KubeListis not checked. Per the function signature inpkg/util/util.go:645-666, it returnsfalsewhen the list operation fails. -
Line 460: The cluster name is hardcoded as
"noobaa-db-pg-cluster"instead of usingr.CNPGCluster.Name(which is used elsewhere, e.g., line 637). This will break if the actual cluster has a different name. -
Lines 461-466: If no service with
cnpg.io/instanceRole == "replica"is found, the environment variable remains unset. This should be logged and potentially treated as an error since RO host functionality would be broken. -
Line 462:
service.Spec.Selectorcould benil, leading to a panic. Add a nil check.
Apply this diff to fix the issues:
case "POSTGRES_HOST_RO":
if r.shouldReconcileCNPGCluster() {
- //RO host is not part of the cnpg app secret, so we're getting it here from the service
+ // RO host is not part of the cnpg app secret, so we're getting it here from the service
services := &corev1.ServiceList{}
- cnpg_label, _ := labels.Parse("app=noobaa,cnpg.io/cluster=noobaa-db-pg-cluster")
- util.KubeList(services, &client.ListOptions{Namespace: r.NooBaaPostgresDB.Namespace, LabelSelector: cnpg_label})
+ cnpgLabel, err := labels.Parse(fmt.Sprintf("app=noobaa,cnpg.io/cluster=%s", r.CNPGCluster.Name))
+ if err != nil {
+ r.Logger.Errorf("Failed to parse CNPG label selector: %v", err)
+ break
+ }
+ if !util.KubeList(services, &client.ListOptions{Namespace: r.NooBaaPostgresDB.Namespace, LabelSelector: cnpgLabel}) {
+ r.Logger.Errorf("Failed to list services for CNPG cluster %s", r.CNPGCluster.Name)
+ break
+ }
+ found := false
for _, service := range services.Items {
- if (service.Spec.Selector["cnpg.io/instanceRole"] == "replica") {
+ if service.Spec.Selector != nil && service.Spec.Selector["cnpg.io/instanceRole"] == "replica" {
c.Env[j].Value = service.Name
+ r.Logger.Infof("Set POSTGRES_HOST_RO to %s", service.Name)
+ found = true
break
}
}
+ if !found {
+ r.Logger.Warnf("No replica service found for CNPG cluster %s, POSTGRES_HOST_RO will not be set", r.CNPGCluster.Name)
+ }
}📝 Committable suggestion
‼️ 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.
| case "POSTGRES_HOST_RO": | |
| if r.shouldReconcileCNPGCluster() { | |
| //RO host is not part of the cnpg app secret, so we're getting it here from the service | |
| services := &corev1.ServiceList{} | |
| cnpg_label, _ := labels.Parse("app=noobaa,cnpg.io/cluster=noobaa-db-pg-cluster") | |
| util.KubeList(services, &client.ListOptions{Namespace: r.NooBaaPostgresDB.Namespace, LabelSelector: cnpg_label}) | |
| for _, service := range services.Items { | |
| if (service.Spec.Selector["cnpg.io/instanceRole"] == "replica") { | |
| c.Env[j].Value = service.Name | |
| break | |
| } | |
| } | |
| } | |
| case "POSTGRES_HOST_RO": | |
| if r.shouldReconcileCNPGCluster() { | |
| // RO host is not part of the cnpg app secret, so we're getting it here from the service | |
| services := &corev1.ServiceList{} | |
| cnpgLabel, err := labels.Parse(fmt.Sprintf("app=noobaa,cnpg.io/cluster=%s", r.CNPGCluster.Name)) | |
| if err != nil { | |
| r.Logger.Errorf("Failed to parse CNPG label selector: %v", err) | |
| break | |
| } | |
| if !util.KubeList(services, &client.ListOptions{Namespace: r.NooBaaPostgresDB.Namespace, LabelSelector: cnpgLabel}) { | |
| r.Logger.Errorf("Failed to list services for CNPG cluster %s", r.CNPGCluster.Name) | |
| break | |
| } | |
| found := false | |
| for _, service := range services.Items { | |
| if service.Spec.Selector != nil && service.Spec.Selector["cnpg.io/instanceRole"] == "replica" { | |
| c.Env[j].Value = service.Name | |
| r.Logger.Infof("Set POSTGRES_HOST_RO to %s", service.Name) | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| r.Logger.Warnf("No replica service found for CNPG cluster %s, POSTGRES_HOST_RO will not be set", r.CNPGCluster.Name) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/system/phase2_creating.go around lines 455-467, the service discovery
block must validate inputs and handle errors: parse the label expression and
check the error returned by labels.Parse, use the actual cluster name
(r.CNPGCluster.Name) instead of the hardcoded "noobaa-db-pg-cluster" when
building the label, call util.KubeList and check its boolean return (handle/log
and return on failure), nil-check service.Spec.Selector before indexing it to
avoid panics, and after iterating if no replica service was found log an error
(or set an appropriate error/condition) so the POSTGRES_HOST_RO env var is not
silently left unset. Ensure all failures are logged with context and return or
propagate an error so the caller can react.
Signed-off-by: Amit Prinz Setter <[email protected]>
alphaprinz
force-pushed
the
read_only_pool
branch
from
a103e16 to
eb69db2
Compare
dannyzaken
left a comment
dannyzaken
left a comment
There was a problem hiding this comment.
My previous comments aside, I think that, on the operator side, it should be ok to assume the service name will be r.CNPGCluster.Name+"-ro". You can just put this as the value
Signed-off-by: Amit Prinz Setter <[email protected]>
2 participants
Explain the changes
For a cluster with cnpg pg, add POSTGRES_HOST_RO to core's env.
Unlike other params used by pg client to connect, RO host is not part of cnpg's app secret, so we need to some more work to set it.
Core pr: noobaa/noobaa-core#9237
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.