K8SPSMDB-1518: allow specifying logrotate configuration#2151
Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
LogRotateSpectype withConfigurationandExtraConfigfields to the API - Refactored logrotate container creation from
pkg/psmdb/logcollector/container.goto new packagepkg/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.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
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.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/apis/psmdb/v1/psmdb_types.go
Outdated
| // ExtraConfig allows specifying logrotate configuration file in addition to the main configuration file. | ||
| // This should be a reference to a ConfigMap in the same namespace. |
There was a problem hiding this comment.
The documentation states that ExtraConfig should reference a ConfigMap in the same namespace, but does not specify the required structure or key names within that ConfigMap. The entrypoint script expects a file named 'custom.conf' (line 15 of build/logcollector/entrypoint.sh), but when a ConfigMap is projected into a volume, each key in the ConfigMap becomes a filename. Users need to know they must create a ConfigMap with a key named 'custom.conf' for this feature to work correctly. Consider adding this requirement to the field documentation.
| // ExtraConfig allows specifying logrotate configuration file in addition to the main configuration file. | |
| // This should be a reference to a ConfigMap in the same namespace. | |
| // ExtraConfig allows specifying a logrotate configuration file in addition to the main configuration file. | |
| // This should be a reference to a ConfigMap in the same namespace. | |
| // The referenced ConfigMap must contain a key named "custom.conf"; when the ConfigMap is mounted as a volume, | |
| // that key becomes a file named "custom.conf" that is loaded as the extra logrotate configuration. |
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
requested review from
eleo007,
jvpasinatto and
valmiranogueira
as code owners
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
|
@mayankshah1607 I've opened a new pull request, #2170, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExtraConfig corev1.LocalObjectReference `json:"extraConfig,omitempty"` | ||
| // Schedule allows specifying the schedule for logrotate. | ||
| // This should be a valid cron expression. | ||
| //+kubebuilder:default:="0 0 * * *" |
There was a problem hiding this comment.
The Schedule field lacks validation for cron expression format. Invalid cron expressions will be passed directly to go-cron, which may cause runtime errors. Consider adding validation (e.g., using a kubebuilder validation marker like +kubebuilder:validation:Pattern) or documenting that invalid schedules will result in container failures.
| //+kubebuilder:default:="0 0 * * *" | |
| //+kubebuilder:default:="0 0 * * *" | |
| //+kubebuilder:validation:Pattern=`^(\S+\s+){4}\S+$` |
| mkdir -p /tmp/fluentbit/custom | ||
| set +e | ||
| local fluentbit_conf_dir="/opt/percona/logcollector/fluentbit/custom" | ||
| for conf_file in $fluentbit_conf_dir/*.conf; do |
There was a problem hiding this comment.
The loop variable conf_file is not quoted. If the glob pattern doesn't match any files or if filenames contain spaces, this could cause issues. The variable should be quoted: for conf_file in "$fluentbit_conf_dir"/*.conf; do
| for conf_file in $fluentbit_conf_dir/*.conf; do | |
| for conf_file in "$fluentbit_conf_dir"/*.conf; do |
| func Container(cr *api.PerconaServerMongoDB, mongoPort int32) (*corev1.Container, error) { | ||
| if cr.Spec.LogCollector == nil { | ||
| return nil, errors.New("logcollector can't be nil") | ||
| } | ||
|
|
||
| usersSecretName := api.UserSecretName(cr) | ||
|
|
||
| envs := []corev1.EnvVar{ | ||
| { | ||
| Name: "MONGODB_HOST", | ||
| Value: "localhost", | ||
| }, | ||
| { | ||
| Name: "MONGODB_PORT", | ||
| Value: strconv.Itoa(int(mongoPort)), | ||
| }, | ||
| { | ||
| Name: "MONGODB_USER", | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| SecretKeyRef: &corev1.SecretKeySelector{ | ||
| Key: "MONGODB_CLUSTER_ADMIN_USER_ESCAPED", | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: usersSecretName, | ||
| }, | ||
| Optional: ptr.To(false), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "MONGODB_PASSWORD", | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| SecretKeyRef: &corev1.SecretKeySelector{ | ||
| Key: "MONGODB_CLUSTER_ADMIN_PASSWORD_ESCAPED", | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: usersSecretName, | ||
| }, | ||
| Optional: ptr.To(false), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| container := corev1.Container{ | ||
| Name: "logrotate", | ||
| Image: cr.Spec.LogCollector.Image, | ||
| Env: envs, | ||
| ImagePullPolicy: cr.Spec.LogCollector.ImagePullPolicy, | ||
| SecurityContext: cr.Spec.LogCollector.ContainerSecurityContext, | ||
| Resources: cr.Spec.LogCollector.Resources, | ||
| Args: []string{ | ||
| "logrotate", | ||
| }, | ||
| Command: []string{"/opt/percona/logcollector/entrypoint.sh"}, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| { | ||
| Name: config.MongodDataVolClaimName, | ||
| MountPath: config.MongodContainerDataDir, | ||
| }, | ||
| { | ||
| Name: config.BinVolumeName, | ||
| MountPath: config.BinMountPath, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if cr.Spec.LogCollector.LogRotate != nil { | ||
| if cr.Spec.LogCollector.LogRotate.Configuration != "" || cr.Spec.LogCollector.LogRotate.ExtraConfig.Name != "" { | ||
| container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ | ||
| Name: VolumeName, | ||
| MountPath: configDir, | ||
| }) | ||
| } | ||
| if cr.Spec.LogCollector.LogRotate.Schedule != "" { | ||
| container.Env = append(container.Env, corev1.EnvVar{ | ||
| Name: "LOGROTATE_SCHEDULE", | ||
| Value: cr.Spec.LogCollector.LogRotate.Schedule, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return &container, nil | ||
| } |
There was a problem hiding this comment.
There's no test coverage for the new logrotate.Container function. The test file pkg/psmdb/logcollector/container_test.go tests the Containers function which calls logrotate.Container, but direct unit tests for the logrotate package's Container function would improve test coverage and make it easier to verify edge cases like nil LogRotate spec or empty values.
commit: f9b073b |
CHANGE DESCRIPTION
Allows specifying custom configuration for logrotate:
Examples:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability