Conversation
| printf '%s %s\n' "$schedule" "$cmd" > /tmp/crontab | ||
| exec supercronic /tmp/crontab | ||
| else | ||
| exec go-cron "$schedule" sh -c "$cmd" | ||
| fi |
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
| printf '%s %s\n' "$schedule" "$cmd" > /tmp/crontab | |
| exec supercronic /tmp/crontab | |
| else | |
| exec go-cron "$schedule" sh -c "$cmd" | |
| fi | |
| printf '%s %s\n' "$schedule" "$cmd" >/tmp/crontab | |
| exec supercronic /tmp/crontab | |
| else | |
| exec go-cron "$schedule" sh -c "$cmd" | |
| fi |
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector" |
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
| "github.com/percona/percona-postgresql-operator/v2/percona/logcollector" |
5f0a128 to
7ddc3cd
Compare
| # - name: s3-ca | ||
| # secret: | ||
| # secretName: my-s3-ca | ||
| # logRotate: |
There was a problem hiding this comment.
@gkech Can we use the same naming as in PSMDBO https://github.com/percona/percona-server-mongodb-operator/blob/main/deploy/cr.yaml
And also, please add the possibility of adding custom liveness/readyness probes for two new sidecars (we have added it for PSMDB as well).
| local logrotate_additional_conf_files=() | ||
| local conf_d_dir="/opt/crunchy/logcollector/logrotate/conf.d" | ||
|
|
||
| # Operator-managed postgres.conf overrides the default when present. |
There was a problem hiding this comment.
[shfmt] reported by reviewdog 🐶
| # Operator-managed postgres.conf overrides the default when present. | |
| # Operator-managed postgres.conf overrides the default when present. |
There was a problem hiding this comment.
Pull request overview
Introduces a new spec.logCollector feature to wire Fluent Bit + logrotate sidecars into PostgreSQL instance pods, including CRD/API updates, operator reconcile logic, container assets in the operator image, and E2E coverage.
Changes:
- Add
LogCollectorSpec/LogRotateSpecto the PerconaPGCluster API and regenerate deepcopy + CRD/bundle manifests. - Implement reconciliation to (a) inject log collector sidecars/volumes and (b) create/update/delete backing ConfigMaps.
- Add build assets (entrypoint + fluent-bit/logrotate configs), default CR example, and a new KUTTL E2E test suite for log collection/rotation/S3 shipping.
Reviewed changes
Copilot reviewed 32 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go | Adds deepcopy support for new log collector API types. |
| pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go | Adds spec.logCollector API types and helper LogCollectorEnabled(). |
| percona/logcollector/reconcile.go | Implements sidecar/volume wiring and ConfigMap reconciliation for log collector. |
| percona/logcollector/reconcile_test.go | Unit tests for reconcile behavior (ConfigMap lifecycle + sidecar wiring + version gate). |
| percona/logcollector/logrotate/container.go | Builds the logrotate sidecar container spec. |
| percona/logcollector/container.go | Builds fluent-bit + logrotate containers, volumes, and security context defaults. |
| percona/logcollector/container_test.go | Unit tests for constructed containers/volumes. |
| percona/controller/pgcluster/controller.go | Hooks logcollector reconciliation into main PGCluster reconcile flow. |
| internal/naming/names.go | Removes unused PGBackRestRepoName constant. |
| e2e-tests/vars.sh | Adds IMAGE_LOGCOLLECTOR test image variable. |
| e2e-tests/functions | Wires IMAGE_LOGCOLLECTOR into generated CRs for tests. |
| e2e-tests/run-release.csv | Enables logcollection scenario in release E2E runs. |
| e2e-tests/run-pr.csv | Enables logcollection scenario in PR E2E runs. |
| e2e-tests/tests/logcollection/00-assert.yaml | Asserts operator deployment/CRD presence for logcollection scenario. |
| e2e-tests/tests/logcollection/00-deploy-operator.yaml | Deploys operator + client + MinIO prerequisites for logcollection scenario. |
| e2e-tests/tests/logcollection/01-assert.yaml | Asserts cluster readiness and expected ConfigMaps. |
| e2e-tests/tests/logcollection/01-create-cluster.yaml | Creates a cluster with logCollector enabled + custom fluent-bit/logrotate config and MinIO TLS wiring. |
| e2e-tests/tests/logcollection/02-assert.yaml | Asserts backup success used to generate logs. |
| e2e-tests/tests/logcollection/02-generate-logs.yaml | Generates Postgres and pgBackRest logs (SQL + on-demand backup). |
| e2e-tests/tests/logcollection/03-check-collected-logs.yaml | Verifies sidecars exist and logs/config are actually being consumed. |
| e2e-tests/tests/logcollection/04-rotate-logs.yaml | Forces logrotate run inside sidecar. |
| e2e-tests/tests/logcollection/05-check-rotated-logs.yaml | Verifies rotated files exist on the data volume. |
| e2e-tests/tests/logcollection/06-check-s3-logs.yaml | Verifies logs are shipped to MinIO (S3). |
| e2e-tests/tests/logcollection/99-remove-cluster-gracefully.yaml | Cleanup step ensuring no finalizers/panics and operator teardown. |
| deploy/crd.yaml | Updates CRD schema to include spec.logCollector. |
| deploy/bundle.yaml | Updates bundle schema to include spec.logCollector. |
| deploy/cr.yaml | Adds example/default logCollector configuration to sample CR. |
| build/postgres-operator/logcollector/logrotate/logrotate.conf | Provides default logrotate rules for Postgres + pgBackRest logs. |
| build/postgres-operator/logcollector/fluentbit/parsers_multiline.conf | Adds fluent-bit multiline parser configuration. |
| build/postgres-operator/logcollector/fluentbit/fluentbit.conf | Top-level fluent-bit config with includes (including custom override dir). |
| build/postgres-operator/logcollector/fluentbit/fluentbit_pg.conf | Fluent-bit inputs for Postgres and pgBackRest logs + stdout output. |
| build/postgres-operator/logcollector/fluentbit/custom/default.conf | Placeholder custom config file in image layout. |
| build/postgres-operator/logcollector/entrypoint.sh | Sidecar entrypoint: runs fluent-bit or scheduled logrotate with config validation. |
| build/postgres-operator/init-entrypoint.sh | Copies logcollector assets into shared bin volume when present in operator image. |
| build/postgres-operator/Dockerfile | Packages logcollector assets into the operator image at /logcollector. |
| func securityContext(cr *v2.PerconaPGCluster) *corev1.SecurityContext { | ||
| if cr.Spec.LogCollector != nil && cr.Spec.LogCollector.ContainerSecurityContext != nil { | ||
| return cr.Spec.LogCollector.ContainerSecurityContext | ||
| } | ||
|
|
||
| sc := &corev1.SecurityContext{ | ||
| RunAsNonRoot: new(true), | ||
| AllowPrivilegeEscalation: new(false), | ||
| Privileged: new(false), | ||
| Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}, | ||
| SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}, | ||
| } | ||
|
|
||
| // OpenShift assigns one shared UID to every container in the pod. | ||
| if cr.Spec.OpenShift == nil || !*cr.Spec.OpenShift { | ||
| sc.RunAsUser = new(postgresUserID) | ||
| sc.RunAsGroup = new(postgresUserID) | ||
| } | ||
|
|
||
| return sc | ||
| } |
There was a problem hiding this comment.
not true, it was with values: https://go.dev/play/p/dTtSaU19JXe
| for i := range cr.Spec.InstanceSets { | ||
| cr.Spec.InstanceSets[i].Sidecars = append(cr.Spec.InstanceSets[i].Sidecars, containers...) | ||
| cr.Spec.InstanceSets[i].SidecarVolumes = append(cr.Spec.InstanceSets[i].SidecarVolumes, volumes...) | ||
| } |
| Path ${PG_LOG_DIR}/*.log,${PG_LOG_DIR}/*.csv | ||
| Tag ${POD_NAMESPACE}.${POD_NAME}.postgres | ||
| Refresh_Interval 5 | ||
| DB /tmp/flb_pg.db |
There was a problem hiding this comment.
should we persist this across restarts?
There was a problem hiding this comment.
@gkech Do we need to add ignore_older option in this case? Who will remove this DB if it becomes big? What we will do in case of a major Fluentbit upgrade, which can require a DB update as well. Will we have a real advantage with the persist approach? If yes, we need to change it for all operators.
There was a problem hiding this comment.
Do we need to add ignore_older option in this case?
Yes
There was a problem hiding this comment.
Who will remove this DB if it becomes big?
if we use ignore_older, the size of the db will be max the amount of days written there, and in general it will not grew thaaat big I guess
| # extra time-based cap. | ||
| /pgdata/pg*/log/*.log | ||
| /pgdata/pg*/log/*.csv { | ||
| daily |
There was a problem hiding this comment.
i wonder how this plays with postgresql's daily rotation according to weekday
CHANGE DESCRIPTION
Jira: https://perconadev.atlassian.net/browse/K8SPG-851
Helm PR: TBD
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability