K8SPSMDB-1518: allow specifying logrotate configuration #2151

mayankshah1607 · 2025-12-17T12:21:44Z

CHANGE DESCRIPTION

Problem:
Short explanation of the problem.

Cause:
Short explanation of the root cause of the issue if applicable.

Solution:
Short explanation of the solution we are providing with this PR.

CHECKLIST

Jira

Is the Jira ticket created and referenced properly?
Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

Is an E2E test/test case added for the new feature/change?
Are unit tests added where appropriate?
Are OpenShift compare files changed for E2E tests (compare/*-oc.yml)?

Config/Logging/Testability

Are all needed new/changed options added to default YAML files?
Are all needed new/changed options added to the Helm Chart?
Did we add proper logging messages for operator actions?
Did we ensure compatibility with the previous version or cluster upgrade process?
Does the change support oldest and newest supported MongoDB version?
Does the change support oldest and newest supported Kubernetes version?

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

github-actions · 2025-12-17T12:21:59Z

build/logcollector/entrypoint.sh

[shellcheck (suggestion)] _{reported by reviewdog 🐶}

percona-server-mongodb-operator/build/logcollector/entrypoint.sh

Line 25 in cfd50e0

exec "$@" $fluentbit_opt

github-actions · 2025-12-17T12:22:17Z

pkg/psmdb/logcollector/logrotate/container.go

+	"fmt"
+	"strconv"
+
+	api "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1"


[goimports-reviser] _{reported by reviewdog 🐶}

Suggested change

api "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1"

corev1 "k8s.io/api/core/v1"

api "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1"

github-actions · 2025-12-17T12:22:17Z

pkg/psmdb/logcollector/logrotate/container.go

+
+	api "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1"
+	"github.com/percona/percona-server-mongodb-operator/pkg/psmdb/config"
+	corev1 "k8s.io/api/core/v1"


[goimports-reviser] _{reported by reviewdog 🐶}

Suggested change

corev1 "k8s.io/api/core/v1"

Copilot

Pull request overview

This PR adds support for customizable logrotate configuration in the Percona Server MongoDB Operator. It introduces new API fields to allow users to specify custom logrotate configurations either inline or via external ConfigMaps, and refactors the logrotate container code into a dedicated package for better organization.

Key changes:

Added LogRotateSpec type with Configuration and ExtraConfig fields to the API
Refactored logrotate container creation from pkg/psmdb/logcollector/container.go to new package pkg/psmdb/logcollector/logrotate
Enhanced configuration hash calculation to include logrotate configurations for pod restart triggers

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
pkg/apis/psmdb/v1/psmdb_types.go	Adds LogRotateSpec type with configuration fields
pkg/apis/psmdb/v1/zz_generated.deepcopy.go	Auto-generated deep copy methods for LogRotateSpec
pkg/psmdb/logcollector/logrotate/container.go	New package with logrotate container creation logic and volume mounts
pkg/psmdb/logcollector/container.go	Removes logRotationContainer function, delegates to logrotate package
pkg/psmdb/statefulset.go	Adds logrotate config params, volumes, and enhanced hash calculation
pkg/controller/perconaservermongodb/statefulset.go	Retrieves logrotate configs and passes to statefulset creation
pkg/controller/perconaservermongodb/psmdb_controller.go	Adds reconcileLogRotateConfigMaps for managing logrotate ConfigMap
build/logcollector/entrypoint.sh	Adds support for custom logrotate config directory in entrypoint script
config/crd/bases/psmdb.percona.com_perconaservermongodbs.yaml	Updates CRD schema with logRotate fields
deploy/crd.yaml	Updates CRD schema with logRotate fields
deploy/bundle.yaml	Updates CRD schema with logRotate fields
deploy/cw-bundle.yaml	Updates CRD schema with logRotate fields
e2e-tests/version-service/conf/crd.yaml	Updates CRD schema with logRotate fields

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T12:29:11Z

deploy/crd.yaml

+                    properties:
+                      configuration:
+                        type: string
+                      extraConfigs:


The field name is "extraConfigs" (plural) but based on the API design and the comment in psmdb_types.go, this should be "extraConfig" (singular) since it references a single ConfigMap, not multiple configs. This inconsistency matches the issue in psmdb_types.go and should be corrected for consistency.

Copilot · 2025-12-17T12:29:12Z

deploy/bundle.yaml

+                    properties:
+                      configuration:
+                        type: string
+                      extraConfigs:


The field name is "extraConfigs" (plural) but based on the API design and the comment in psmdb_types.go, this should be "extraConfig" (singular) since it references a single ConfigMap, not multiple configs. This inconsistency matches the issue in psmdb_types.go and should be corrected for consistency.

Copilot · 2025-12-17T12:29:12Z

e2e-tests/version-service/conf/crd.yaml

+                    properties:
+                      configuration:
+                        type: string
+                      extraConfigs:


The field name is "extraConfigs" (plural) but based on the API design and the comment in psmdb_types.go, this should be "extraConfig" (singular) since it references a single ConfigMap, not multiple configs. This inconsistency matches the issue in psmdb_types.go and should be corrected for consistency.

Copilot · 2025-12-17T12:29:12Z

pkg/apis/psmdb/v1/psmdb_types.go

+	// AdditionalConfig allows specifying an additional configuration file for logrotate.
+	// This should be a reference to a ConfigMap in the same namespace.


The comment says "AdditionalConfig" but the field is named "ExtraConfig". The comment should be updated to match the field name for consistency. Additionally, since the field references a single ConfigMap, the comment should clarify this is for "an additional configuration" rather than multiple configs.

Copilot · 2025-12-17T12:29:12Z

pkg/psmdb/statefulset.go

+	if cr.CompareVersion("1.22.0") >= 0 && configs.LogRotateExtraConf.Type.IsUsable() {
+		volumes = append(volumes, corev1.Volume{
+			Name:         logrotate.CustomVolumeName,
+			VolumeSource: configs.LogRotateConf.Type.VolumeSource(cr.Spec.LogCollector.LogRotate.ExtraConfig.Name),


The volume source uses "configs.LogRotateConf.Type.VolumeSource" instead of "configs.LogRotateExtraConf.Type.VolumeSource". This is inconsistent with the check on line 184 which tests "configs.LogRotateExtraConf.Type.IsUsable()". The volume source should use LogRotateExtraConf to match the condition being checked.

Copilot · 2025-12-17T12:29:13Z

config/crd/bases/psmdb.percona.com_perconaservermongodbs.yaml

+                    properties:
+                      configuration:
+                        type: string
+                      extraConfigs:


The field name is "extraConfigs" (plural) but based on the API design and the comment in psmdb_types.go, this should be "extraConfig" (singular) since it references a single ConfigMap, not multiple configs. This inconsistency matches the issue in psmdb_types.go and should be corrected for consistency.

Copilot · 2025-12-17T12:29:13Z

deploy/cw-bundle.yaml

+                    properties:
+                      configuration:
+                        type: string
+                      extraConfigs:


The field name is "extraConfigs" (plural) but based on the API design and the comment in psmdb_types.go, this should be "extraConfig" (singular) since it references a single ConfigMap, not multiple configs. This inconsistency matches the issue in psmdb_types.go and should be corrected for consistency.

Copilot · 2025-12-17T12:29:14Z

pkg/apis/psmdb/v1/psmdb_types.go

+	Configuration string `json:"configuration,omitempty"`
+	// AdditionalConfig allows specifying an additional configuration file for logrotate.
+	// This should be a reference to a ConfigMap in the same namespace.
+	ExtraConfig corev1.LocalObjectReference `json:"extraConfigs,omitempty"`


The JSON tag is "extraConfigs" (plural) but the Go field name is "ExtraConfig" (singular). This creates an inconsistency in the API. The JSON tag should be "extraConfig" to match the field name and maintain consistency, or the field should be renamed to ExtraConfigs.

Copilot · 2025-12-17T12:29:14Z

pkg/psmdb/logcollector/logrotate/container.go

+
+	MongodbConfig = "mongodb.conf"
+
+	custonConfigDir = "/opt/percona/logcollector/logrotate/conf.d"


The constant name "custonConfigDir" contains a typo. It should be "customConfigDir" with "custom" spelled correctly.

Copilot · 2025-12-17T12:29:14Z

pkg/psmdb/logcollector/logrotate/container.go

+			container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
+				Name:      VolumeName,
+				MountPath: custonConfigDir,
+			})
+		}
+		if cr.Spec.LogCollector.LogRotate.ExtraConfig.Name != "" {
+			container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
+				Name:      CustomVolumeName,
+				MountPath: custonConfigDir,
+			})


Both volume mounts for Configuration and ExtraConfig use the same mount path "custonConfigDir". This will cause a conflict because Kubernetes does not allow multiple volumes to be mounted at the same path. These should either use different mount paths, or the volumes should be merged somehow. The second volume mount will likely override the first one, causing the Configuration volume to be inaccessible.

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T13:05:33Z

build/logcollector/entrypoint.sh

 		rm -rf /tmp/passwd
 	fi
-	exec go-cron "0 0 * * *" sh -c "logrotate -s /data/db/logs/logrotate.status /opt/percona/logcollector/logrotate/logrotate.conf;"
+	exec go-cron "0 0 * * *" sh -c "logrotate -s /data/db/logs/logrotate.status /opt/percona/logcollector/logrotate/logrotate.conf ${LOGROTATE_CONF_DIR};"


The logrotate command is passed a directory path in LOGROTATE_CONF_DIR, but logrotate expects configuration file paths, not directories. This should use a glob pattern like "${LOGROTATE_CONF_DIR}/*" to include all files in the directory, or the directory should be included via an "include" directive in the main configuration file.

Copilot · 2025-12-17T13:05:33Z

pkg/psmdb/logcollector/logrotate/container.go

+		if cr.Spec.LogCollector.LogRotate.ExtraConfig.Name != "" {
+			container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
+				Name:      CustomVolumeName,
+				MountPath: custonConfigDir,
+			})


Both volume mounts use the same MountPath (custonConfigDir). This will cause a conflict where the second volume mount will override the first. If both configuration sources need to be mounted, they should have different mount paths, or they should be mutually exclusive.

Copilot · 2025-12-17T13:05:33Z

build/logcollector/entrypoint.sh

 export PATH="$PATH":/opt/fluent-bit/bin

+LOGROTATE_CONF_DIR=""
+if [ -f /opt/percona/logcollector/logrotate/conf.d ]; then


The condition uses "-f" flag which checks if the path is a regular file, but the path "/opt/percona/logcollector/logrotate/conf.d" is expected to be a directory. This should use "-d" flag instead to check if it's a directory.

Copilot · 2025-12-17T13:05:34Z

pkg/psmdb/statefulset.go

+	if cr.CompareVersion("1.22.0") >= 0 && configs.LogRotateExtraConf.Type.IsUsable() {
+		volumes = append(volumes, corev1.Volume{
+			Name:         logrotate.CustomVolumeName,
+			VolumeSource: configs.LogRotateConf.Type.VolumeSource(cr.Spec.LogCollector.LogRotate.ExtraConfig.Name),


Using configs.LogRotateConf.Type.VolumeSource but should use configs.LogRotateExtraConf.Type.VolumeSource instead. The volume is for the extra configuration, not the main logrotate configuration.

Copilot · 2025-12-17T13:05:34Z

pkg/psmdb/statefulset.go

+	if cr.CompareVersion("1.22.0") >= 0 && configs.LogRotateExtraConf.Type.IsUsable() {
+		volumes = append(volumes, corev1.Volume{
+			Name:         logrotate.CustomVolumeName,
+			VolumeSource: configs.LogRotateConf.Type.VolumeSource(cr.Spec.LogCollector.LogRotate.ExtraConfig.Name),
+		})
+	}


Potential nil pointer dereference. The code accesses cr.Spec.LogCollector.LogRotate.ExtraConfig.Name without checking if cr.Spec.LogCollector or cr.Spec.LogCollector.LogRotate are nil. While configs.LogRotateExtraConf.Type.IsUsable() is checked, this doesn't guarantee that the nested LogCollector fields are non-nil. Add nil checks for cr.Spec.LogCollector and cr.Spec.LogCollector.LogRotate before accessing ExtraConfig.Name.

JNKPercona · 2025-12-17T16:44:09Z

Test Name	Result	Time
arbiter	passed	00:11:36
balancer	passed	00:18:10
cross-site-sharded	passed	00:18:43
custom-replset-name	passed	00:10:14
custom-tls	passed	00:14:08
custom-users-roles	passed	00:10:59
custom-users-roles-sharded	passed	00:11:51
data-at-rest-encryption	passed	00:12:55
data-sharded	passed	00:23:25
demand-backup	passed	00:16:22
demand-backup-eks-credentials-irsa	passed	00:00:08
demand-backup-fs	passed	00:22:40
demand-backup-if-unhealthy	passed	00:08:32
demand-backup-incremental	failure	00:25:31
demand-backup-incremental-sharded	passed	01:00:59
demand-backup-physical-parallel	passed	00:08:29
demand-backup-physical-aws	passed	00:11:59
demand-backup-physical-azure	passed	00:12:01
demand-backup-physical-gcp-s3	passed	00:12:12
demand-backup-physical-gcp-native	passed	00:12:06
demand-backup-physical-minio	passed	00:19:42
demand-backup-physical-minio-native	passed	00:20:03
demand-backup-physical-sharded-parallel	passed	00:11:00
demand-backup-physical-sharded-aws	passed	00:18:00
demand-backup-physical-sharded-azure	passed	00:16:57
demand-backup-physical-sharded-gcp-native	passed	00:17:04
demand-backup-physical-sharded-minio	passed	00:17:20
demand-backup-physical-sharded-minio-native	passed	00:17:38
demand-backup-sharded	passed	00:25:12
expose-sharded	passed	00:33:27
finalizer	passed	00:09:48
ignore-labels-annotations	passed	00:07:41
init-deploy	passed	00:12:44
ldap	passed	00:09:04
ldap-tls	passed	00:12:40
limits	passed	00:06:14
liveness	passed	00:08:05
mongod-major-upgrade	passed	00:13:14
mongod-major-upgrade-sharded	passed	00:23:33
monitoring-2-0	passed	00:26:41
monitoring-pmm3	passed	00:28:55
multi-cluster-service	passed	00:13:45
multi-storage	passed	00:18:54
non-voting-and-hidden	passed	00:18:33
one-pod	passed	00:07:40
operator-self-healing-chaos	passed	00:15:27
pitr	passed	00:32:03
pitr-physical	passed	01:01:46
pitr-sharded	passed	00:22:07
pitr-to-new-cluster	passed	00:24:48
pitr-physical-backup-source	passed	00:55:01
preinit-updates	passed	00:05:41
pvc-resize	passed	00:14:02
recover-no-primary	passed	00:28:12
replset-overrides	passed	00:16:33
rs-shard-migration	passed	00:13:55
scaling	passed	00:11:07
scheduled-backup	passed	00:18:00
security-context	passed	00:07:31
self-healing-chaos	passed	00:15:27
service-per-pod	passed	00:18:58
serviceless-external-nodes	passed	00:08:05
smart-update	passed	00:08:54
split-horizon	passed	00:08:09
stable-resource-version	passed	00:05:00
storage	passed	00:07:35
tls-issue-cert-manager	passed	00:32:17
upgrade	passed	00:09:59
upgrade-consistency	passed	00:06:34
upgrade-consistency-sharded-tls	passed	00:55:14
upgrade-sharded	passed	00:19:13
upgrade-partial-backup	passed	00:16:03
users	passed	00:17:52
version-service	passed	00:26:15

Summary	Value
Tests Run	74/74
Job Duration	03:45:40
Total Test Time	21:59:16

commit: 135050b
image: perconalab/percona-server-mongodb-operator:PR-2151-135050b2

mayankshah1607 added 2 commits December 17, 2025 16:33

add LogRotate spec to PSMDB CR

e29e544

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

implement logrotate config override

cfd50e0

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

Copilot AI review requested due to automatic review settings December 17, 2025 12:21

pull-request-size bot added the size/L 100-499 lines label Dec 17, 2025

github-actions bot reviewed Dec 17, 2025

View reviewed changes

github-actions bot added tests build labels Dec 17, 2025

github-actions bot reviewed Dec 17, 2025

View reviewed changes

Copilot started reviewing on behalf of mayankshah1607 December 17, 2025 12:22 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

mayankshah1607 added 2 commits December 17, 2025 18:02

fix field name

8f2e658

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

update comment

135050b

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

Copilot AI review requested due to automatic review settings December 17, 2025 12:57

Copilot started reviewing on behalf of mayankshah1607 December 17, 2025 12:58 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

		// AdditionalConfig allows specifying an additional configuration file for logrotate.
		// This should be a reference to a ConfigMap in the same namespace.


		MongodbConfig = "mongodb.conf"

		custonConfigDir = "/opt/percona/logcollector/logrotate/conf.d"

K8SPSMDB-1518: allow specifying logrotate configuration #2151

Are you sure you want to change the base?

K8SPSMDB-1518: allow specifying logrotate configuration #2151

Uh oh!

Conversation

mayankshah1607 commented Dec 17, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CHANGE DESCRIPTION

CHECKLIST

Uh oh!

github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

JNKPercona commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mayankshah1607 commented Dec 17, 2025 •

edited by atlassian bot

Loading