Added observability-termination-policy to noobaa-operator#1694
Added observability-termination-policy to noobaa-operator#1694aayushchouhan09 merged 1 commit intonoobaa:masterfrom
Conversation
WalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/internal/statefulset-core.yaml (1)
60-60: LGTM: terminationMessagePolicy added to core and log-processorCorrect field/value and placement under each container spec. Small operational note: with FallbackToLogsOnError, up to ~4KiB of recent logs may surface in pod status on failures—ensure sensitive data isn’t logged at error time.
Also applies to: 171-171
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(2 hunks)deploy/internal/statefulset-postgres-db.yaml(1 hunks)deploy/operator.yaml(1 hunks)pkg/bundle/deploy.go(9 hunks)
⏰ 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-hac-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-operator-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: golangci-lint
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-admission-test
🔇 Additional comments (7)
deploy/operator.yaml (1)
44-44: LGTM: operator crash context will be captured on failureAppropriate use of FallbackToLogsOnError for the operator to aid troubleshooting while keeping default terminationMessagePath.
deploy/internal/statefulset-postgres-db.yaml (1)
28-28: LGTM: postgres container now falls back to logs on errorMatches the policy applied elsewhere; helpful for postmortems of DB pod crashes.
deploy/internal/deployment-endpoint.yaml (1)
60-60: LGTM: endpoint container gains FallbackToLogsOnErrorConsistent with other manifests and useful for capturing last-error context.
pkg/bundle/deploy.go (4)
4051-4051: Good addition: terminationMessagePolicy on endpoint containerSetting terminationMessagePolicy: FallbackToLogsOnError improves debuggability for crash scenarios without altering normal behavior.
5160-5160: Consistent policy for core and log-processor containersBoth core and noobaa-log-processor now fallback to logs on error, matching the PR goal and aiding post-mortem analysis.
Also applies to: 5271-5271
5337-5337: Postgres container covered as wellApplying FallbackToLogsOnError to db ensures DB crash loops surface useful logs via the termination message.
6241-6241: Operator container policy alignedOperator container now uses FallbackToLogsOnError too; this is especially helpful for early init failures.
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
45e5631 to
ec3f09d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
deploy/internal/deployment-endpoint.yaml(1 hunks)deploy/internal/statefulset-core.yaml(2 hunks)deploy/internal/statefulset-postgres-db.yaml(1 hunks)deploy/operator.yaml(1 hunks)pkg/bundle/deploy.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/internal/statefulset-core.yaml
- deploy/operator.yaml
- deploy/internal/statefulset-postgres-db.yaml
- deploy/internal/deployment-endpoint.yaml
⏰ 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-core-config-map-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-admission-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (5)
pkg/bundle/deploy.go (5)
4043-4043: Good addition: endpoint now captures error logs on termination.Setting terminationMessagePolicy: FallbackToLogsOnError on the endpoint container is correct and helps surface failure context via the pod’s termination message. No other spec changes introduced.
5152-5152: Good addition: core container termination messages fallback to logs.This aligns with the PR goal and is supported on all modern K8s/OCP versions.
5263-5263: Confirm intent: also applied to noobaa-log-processor.The PR description mentions operator, endpoint, core, and postgres-db. You also added it to noobaa-log-processor. If intentional, nothing else to do; otherwise, please confirm whether this should be reverted.
5329-5329: Good addition: postgres db container covered.Matches the stated scope and improves failure diagnostics on DB container exits.
6233-6233: Good addition: operator container covered.Termination messages will now fallback to logs for operator pod exits.
Explain the changes
terminationMessagePolicy: FallbackToLogsOnErrorto the containers (operator, endpoint, core and postgress-db).Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit