K8SPSMDB-1531: add `hookScript` #2155

pooknull · 2025-12-19T11:24:04Z

https://perconadev.atlassian.net/browse/K8SPSMDB-1531

DESCRIPTION

This PR adds the following fields to the cr.yaml

.spec.replsets[].hookScript
.spec.replsets[].hidden.hookScript
.spec.replsets[].nonvoting.hookScript
.spec.replsets[].arbiter.hookScript
.spec.sharding.configsvrReplSet.hookScript
.spec.sharding.mongos.hookScript
.spec.backup.hookScript

Each hookScript section has the following structure:

    hookScript:
      configMapRef:
        name: hookscript-configmap
      script: |
        #!/usr/bin/env bash
        echo "Hookscript started"

hookScript.script field specifies a shell script that will be executed from the container entrypoint of the corresponding pods. Operator will create a configmap with hook.sh key if this field is specified. The field will be ignored if hookScript.configMapRef.name is used
hookScript.configMapRef.name field specifies a name of the existing configmap with hook.sh key that contains the script data

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?

https://perconadev.atlassian.net/browse/K8SPSMDB-1531

Copilot

Pull request overview

This PR adds the hookScript functionality to the Percona Server MongoDB Operator, allowing users to define custom scripts that execute during pod initialization. The PR also includes cleanup of version-based conditionals for features from older operator versions.

Added hookScript field to the MultiAZ struct in the API, enabling hookscript configuration across different MongoDB components
Implemented ConfigMap-based volume mounting for hook scripts with reconciliation logic in the controller
Cleaned up version checks by removing conditionals for features already available in supported versions (< 1.16.0)

Reviewed changes

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

Show a summary per file

File	Description
pkg/apis/psmdb/v1/psmdb_types.go	Added HookScript field to MultiAZ struct
pkg/psmdb/config/const.go	Added constants for hookscript volume and mount path
pkg/naming/naming.go	Added naming functions for hookscript ConfigMaps
pkg/psmdb/statefulset.go	Added hookscript volume mounting logic and removed old version checks
pkg/psmdb/mongos.go	Added hookscript support for mongos and removed old version checks
pkg/psmdb/container.go	Added hookscript volume mount and removed old version checks
pkg/controller/perconaservermongodb/psmdb_controller.go	Refactored ConfigMap reconciliation and added hookscript ConfigMap management
build/ps-entry.sh	Added hookscript execution and fixed indentation
deploy/cr.yaml	Added example hookscript configuration
deploy/crd.yaml, deploy/bundle.yaml, deploy/cw-bundle.yaml, config/crd/bases/psmdb.percona.com_perconaservermongodbs.yaml, e2e-tests/version-service/conf/crd.yaml	Updated CRD schemas with hookScript field

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

deploy/cr.yaml

build/ps-entry.sh

pkg/controller/perconaservermongodb/psmdb_controller.go

deploy/cr.yaml

pkg/controller/perconaservermongodb/psmdb_controller.go

deploy/cr.yaml

hors

I think we need to add it for PBM agent as well

Copilot

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (1)

pkg/controller/perconaservermongodb/psmdb_controller.go:425

The reconcileBackups call is now made regardless of whether backups are enabled. This creates redundant logic since reconcileBackups internally checks if backups are enabled (line 41-43) and then calls reconcileBackupTasks. However, reconcileBackupTasks is also called separately at line 420 when cr.Spec.Backup.Enabled is true. This means when backups are enabled, reconcileBackupTasks will be called twice - once inside reconcileBackups and once explicitly here. Consider removing the separate reconcileBackupTasks call at lines 419-425 since it's now handled within reconcileBackups.

	}
	if ikCreated {
		log.Info("Created a new mongo key", "KeyName", cr.Spec.Secrets.GetInternalKey(cr))
	}

	created, err := r.ensureSecurityKey(ctx, cr, cr.Spec.Secrets.EncryptionKey, api.EncryptionKeyName, 32, false)
	if err != nil {
		err = errors.Wrapf(err, "ensure mongo Key %s", cr.Spec.Secrets.EncryptionKey)
		return reconcile.Result{}, err
	}
	if created {

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

build/ps-entry.sh

build/pbm-entry.sh

pkg/controller/perconaservermongodb/psmdb_controller.go

pkg/psmdb/statefulset.go

pkg/psmdb/mongos.go

build/ps-entry.sh

egegunes · 2025-12-25T05:38:39Z

pkg/controller/perconaservermongodb/backup.go

+	if err := r.reconcileBackupHookscript(ctx, cr); err != nil {
+		return errors.Wrap(err, "reconcile backup hookscript")
+	}


do we need to do this if backups are disabled?

mayankshah1607 · 2025-12-30T08:09:18Z

pkg/apis/psmdb/v1/psmdb_types.go

 	Configuration            BackupConfig                 `json:"configuration,omitempty"`
 	VolumeMounts             []corev1.VolumeMount         `json:"volumeMounts,omitempty"`
 	StartingDeadlineSeconds  *int64                       `json:"startingDeadlineSeconds,omitempty"`
+	HookScript               string                       `json:"hookScript,omitempty"`


Hmm, do we need to allow specifying a ConfigMap reference also? What happens if a user wants to use the same hookscript everywhere, or maybe across different replsets? They need to copy paste it everytime, maybe specifying a refererence to a Configmap might help improve it?

@egegunes WDYT about it? Support team did not ask to do it but we can be pro active but at the same time we will need to improve it for all operators. And maybe for now we can avoid adding it ...

Its okay to add it later, but in that case, at least we can redesign this field for future-proofing like so:

hookScript: script: | # .. specify script here # Below fields can be added later configMapRef: name: hook-script-configmap

Otherwise later we will end up with something like this which is not very elegant:

hookScript: " ... " hookScriptConfigMap: hook-script-config-map

I like @mayankshah1607's suggestion

gkech · 2026-01-05T08:12:05Z

pkg/controller/perconaservermongodb/backup.go

+	return nil
+}
+
+func (r *ReconcilePerconaServerMongoDB) reconcileBackupHookscript(ctx context.Context, cr *api.PerconaServerMongoDB) error {


Let's use camel case throughout the naming here i.e. reconcileBackupHookScript

gkech · 2026-01-05T08:13:22Z

pkg/controller/perconaservermongodb/backup.go

+			Labels:    naming.ClusterLabels(cr),
+		},
+		Data: map[string]string{
+			"hook.sh": string(cr.Spec.Backup.HookScript),


string casting is not needed

gkech · 2026-01-05T08:15:59Z

pkg/controller/perconaservermongodb/psmdb_controller.go

 		log.Info("Created a new mongo key", "KeyName", cr.Spec.Secrets.EncryptionKey)
 	}

+	err = r.reconcileBackups(ctx, cr)


I'm wondering how this interplays with the following code block i.e. right now we have

err = r.reconcileBackups(ctx, cr) if err != nil { return reconcile.Result{}, errors.Wrap(err, "reconcile backup tasks") } if cr.Spec.Backup.Enabled { err = r.reconcileBackupTasks(ctx, cr) if err != nil { err = errors.Wrap(err, "reconcile backup tasks") return reconcile.Result{}, err } }

However in the new r.reconcileBackups(ctx, cr) function, we are also checking the same things and then we call again reconcileBackupTasks. So seems like that function will be executed x2 if backups are enabled.

Copilot

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

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

pkg/controller/perconaservermongodb/backup.go

pkg/psmdb/statefulset.go

pkg/apis/psmdb/v1/psmdb_types.go

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 7 comments.

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

Copilot · 2026-01-07T13:57:11Z

pkg/psmdb/statefulset.go

+			MountPath: config.BinMountPath,
+			ReadOnly:  !attachHookScript,


The bin volume mount's ReadOnly property is changed from true to !attachHookScript. This means when a hookscript is specified, the bin volume becomes writable. However, there's no clear reason why the hookscript feature requires write access to the bin volume. The hookscript is mounted separately at /opt/percona/pbm-hookscript, not in the bin directory.

If write access is not actually needed, this change unnecessarily weakens the security posture by making a critical system directory writable. Consider keeping ReadOnly: true unless there's a specific requirement for write access.

Copilot · 2026-01-07T13:57:11Z

pkg/apis/psmdb/v1/psmdb_types.go

+}
+
+// ConfigMapReference references a ConfigMap.
+// Usage of corev1.LocalObjectReference is discouraged; prefer this type instead.


The comment "Usage of corev1.LocalObjectReference is discouraged; prefer this type instead" on the ConfigMapReference type is unclear. It doesn't explain why LocalObjectReference is discouraged or what specific advantages this custom type provides. Consider expanding the comment to explain:

Why LocalObjectReference is discouraged in this context

What benefits this custom type provides (e.g., better API versioning, clearer field documentation, etc.)

Any migration guidance for existing users

Without this context, developers might not understand the rationale and might still use LocalObjectReference elsewhere in the codebase.

Suggested change

// Usage of corev1.LocalObjectReference is discouraged; prefer this type instead.

// Usage of corev1.LocalObjectReference in this CRD is discouraged; prefer this

// type instead. LocalObjectReference is a generic core Kubernetes type that

// exposes fields and semantics which are not all supported or versioned as part

// of this operator API. ConfigMapReference provides a narrow, CRD-specific

// contract that:

// * Clearly documents which fields are respected (currently only "name").

// * Avoids leaking k8s core types into the public CRD surface, making API

// evolution and versioning easier.

// * Improves schema clarity and generated documentation for users of this CRD.

//

// Migration guidance:

// * When defining this field, replace usages of corev1.LocalObjectReference

// with ConfigMapReference.

// * Only the Name field is honored; other LocalObjectReference fields (if any)

// are not supported here.

Copilot · 2026-01-07T13:57:11Z

pkg/controller/perconaservermongodb/psmdb_controller.go

+		name := naming.HookScriptConfigMapName(cr, rs, component)
+		if hookscript.Script == "" || hookscript.ConfigMapRef.Name != "" {
+			if err := deleteConfigMapIfExists(ctx, r.client, cr, name); err != nil {
+				return errors.Wrapf(err, "failed to delete mongod config map %s", name)


The error message says "failed to delete mongod config map" but this function is actually deleting a hookscript ConfigMap, not a mongod config map. The error message should be more accurate, such as "failed to delete hookscript config map" to avoid confusion during troubleshooting.

Suggested change

return errors.Wrapf(err, "failed to delete mongod config map %s", name)

return errors.Wrapf(err, "failed to delete hookscript config map %s", name)

Copilot · 2026-01-07T13:57:12Z

pkg/controller/perconaservermongodb/psmdb_controller.go

 		if rs.Hidden.Enabled {
-			name = naming.HiddenConfigMapName(cr, rs)
-			if rs.Hidden.Configuration == "" {
-				if err := deleteConfigMapIfExists(ctx, r.client, cr, name); err != nil {
-					return errors.Wrap(err, "failed to delete hidden config map")
-				}
-
-				continue
+			if err := reconcileConfigMap(rs, naming.HiddenConfigMapName(cr, rs), string(rs.Hidden.Configuration)); err != nil {
+				return errors.Wrap(err, "failed to reconcile config map")
 			}
-			cm := &corev1.ConfigMap{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      name,
-					Namespace: cr.Namespace,
-					Labels:    naming.RSLabels(cr, rs),
-				},
-				Data: map[string]string{
-					"mongod.conf": string(rs.Hidden.Configuration),
-				},
+			if err := reconcileHookscript(rs, naming.ComponentHidden, rs.Hidden.HookScript); err != nil {
+				return errors.Wrap(err, "failed to reconcile hookscript")
 			}
-			err := r.createOrUpdateConfigMap(ctx, cr, cm)
-			if err != nil {
-				return errors.Wrap(err, "create or update hidden config map")
+		}


The reconcileHookscript function doesn't clean up the ConfigMap when Hidden component is disabled (only when it's enabled). This means if a user enables Hidden with a hookscript and then later disables it, the hookscript ConfigMap will not be deleted.

Consider adding cleanup logic before the if rs.Hidden.Enabled check, similar to how it's done for the configuration ConfigMaps. For example:

if !rs.Hidden.Enabled { if err := deleteConfigMapIfExists(ctx, r.client, cr, naming.HookScriptConfigMapName(cr, rs, naming.ComponentHidden)); err != nil { return errors.Wrap(err, "failed to delete hidden hookscript") } }

Copilot · 2026-01-07T13:57:12Z

pkg/apis/psmdb/v1/psmdb_types.go

+type HookScriptSpec struct {
+	Script       string             `json:"script,omitempty"`
+	ConfigMapRef ConfigMapReference `json:"configMapRef,omitempty"`
+}


The HookScriptSpec type lacks documentation explaining what hook scripts are, how they're executed, and what their purpose is. Consider adding a comment that explains:

What hookscripts are and when they execute (e.g., "HookScriptSpec defines a shell script executed during container initialization")

The execution context and environment

Expected use cases

The relationship between Script and ConfigMapRef fields (e.g., "Script takes precedence and ConfigMapRef is ignored if Script is set")

This would help API consumers understand how to use this feature correctly.

Copilot · 2026-01-07T13:57:12Z

pkg/controller/perconaservermongodb/psmdb_controller.go

+		if rs.NonVoting.Enabled {
+			if err := reconcileConfigMap(rs, naming.NonVotingConfigMapName(cr, rs), string(rs.NonVoting.Configuration)); err != nil {
+				return errors.Wrap(err, "failed to reconcile config map")
 			}
-			err := r.createOrUpdateConfigMap(ctx, cr, cm)
-			if err != nil {
-				return errors.Wrap(err, "create or update nonvoting config map")
+			if err := reconcileHookscript(rs, naming.ComponentNonVoting, rs.NonVoting.HookScript); err != nil {
+				return errors.Wrap(err, "failed to reconcile hookscript")
 			}
 		}


The reconcileHookscript function doesn't clean up the ConfigMap when NonVoting or Hidden components are disabled (only when they're enabled). This means if a user enables NonVoting/Hidden with a hookscript and then later disables it, the hookscript ConfigMap will not be deleted.

Consider adding cleanup logic before the if rs.NonVoting.Enabled check, similar to how it's done for the configuration ConfigMaps. For example:

if !rs.NonVoting.Enabled { if err := deleteConfigMapIfExists(ctx, r.client, cr, naming.HookScriptConfigMapName(cr, rs, naming.ComponentNonVoting)); err != nil { return errors.Wrap(err, "failed to delete nonvoting hookscript") } }

Copilot · 2026-01-07T13:57:12Z

pkg/controller/perconaservermongodb/psmdb_controller.go

+		if rs.Arbiter.Enabled {
+			if err := reconcileHookscript(rs, naming.ComponentArbiter, rs.Arbiter.HookScript); err != nil {
+				return errors.Wrap(err, "failed to reconcile hookscript")
 			}
 		}


The reconcileHookscript function doesn't clean up the ConfigMap when Arbiter component is disabled (only when it's enabled). This means if a user enables Arbiter with a hookscript and then later disables it, the hookscript ConfigMap will not be deleted.

Consider adding cleanup logic before the if rs.Arbiter.Enabled check, similar to how it's done for the configuration ConfigMaps. For example:

if !rs.Arbiter.Enabled { if err := deleteConfigMapIfExists(ctx, r.client, cr, naming.HookScriptConfigMapName(cr, rs, naming.ComponentArbiter)); err != nil { return errors.Wrap(err, "failed to delete arbiter hookscript") } }

K8SPSMDB-1531: add hookScript

9b32c64

https://perconadev.atlassian.net/browse/K8SPSMDB-1531

Copilot AI review requested due to automatic review settings December 19, 2025 11:24

pull-request-size bot added the size/XL 500-999 lines label Dec 19, 2025

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

Copilot started reviewing on behalf of pooknull December 19, 2025 11:24 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

hors requested changes Dec 19, 2025

View reviewed changes

pooknull added 6 commits December 22, 2025 14:42

fix cr.yaml

9935e9a

add hookscript to pbm agent

2bf59e0

add hookScript to .spec.backup

cfb9727

make manifests

ea1a08e

fix syntax highlighting

ed60187

add e2e test

39e55b7

Copilot AI review requested due to automatic review settings December 23, 2025 13:49

pull-request-size bot added size/XXL 1000+ lines and removed size/XL 500-999 lines labels Dec 23, 2025

Copilot started reviewing on behalf of pooknull December 23, 2025 13:50 View session

Merge branch 'main' into K8SPSMDB-1531

25830b6

Copilot AI reviewed Dec 23, 2025

View reviewed changes

pooknull requested a review from hors December 24, 2025 08:35

pooknull marked this pull request as ready for review December 24, 2025 08:35

pooknull requested review from egegunes, eleo007, gkech, jvpasinatto, nmarukovich and valmiranogueira as code owners December 24, 2025 08:35

egegunes reviewed Dec 25, 2025

View reviewed changes

mayankshah1607 reviewed Dec 30, 2025

View reviewed changes

gkech reviewed Jan 5, 2026

View reviewed changes

address comments

9b05c5d

pooknull requested review from bogdanjeler-ev and oksana-grishchenko as code owners January 5, 2026 10:10

Merge remote-tracking branch 'origin/main' into K8SPSMDB-1531

1cc8cc8

Copilot AI review requested due to automatic review settings January 5, 2026 10:52

Copilot started reviewing on behalf of pooknull January 5, 2026 10:56 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

pkg/controller/perconaservermongodb/backup.go Outdated Show resolved Hide resolved

pkg/psmdb/statefulset.go Show resolved Hide resolved

pooknull added 2 commits January 6, 2026 11:57

update hookScript structure

fd117cf

goimports-reviser

893ccb1

pooknull requested review from Copilot, gkech and mayankshah1607 January 6, 2026 09:59

Copilot started reviewing on behalf of pooknull January 6, 2026 10:00 View session

pooknull requested a review from egegunes January 6, 2026 10:00

mayankshah1607 reviewed Jan 6, 2026

View reviewed changes

pkg/apis/psmdb/v1/psmdb_types.go Show resolved Hide resolved

mayankshah1607 previously approved these changes Jan 6, 2026

View reviewed changes

Copilot AI reviewed Jan 6, 2026

View reviewed changes

Merge branch 'main' into K8SPSMDB-1531

1c2dff9

gkech previously approved these changes Jan 6, 2026

View reviewed changes

egegunes previously approved these changes Jan 7, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into K8SPSMDB-1531

9fcec4d

Copilot AI review requested due to automatic review settings January 7, 2026 13:48

pooknull dismissed stale reviews from egegunes, gkech, and mayankshah1607 via 9fcec4d January 7, 2026 13:48

Copilot started reviewing on behalf of pooknull January 7, 2026 13:49 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

Merge branch 'main' into K8SPSMDB-1531

880a468

hors requested a review from egegunes January 7, 2026 18:28

-// Usage of corev1.LocalObjectReference is discouraged; prefer this type instead.
+// Usage of corev1.LocalObjectReference in this CRD is discouraged; prefer this
+// type instead. LocalObjectReference is a generic core Kubernetes type that
+// exposes fields and semantics which are not all supported or versioned as part
+// of this operator API. ConfigMapReference provides a narrow, CRD-specific
+// contract that:
+//   * Clearly documents which fields are respected (currently only "name").
+//   * Avoids leaking k8s core types into the public CRD surface, making API
+//     evolution and versioning easier.
+//   * Improves schema clarity and generated documentation for users of this CRD.
+//
+// Migration guidance:
+//   * When defining this field, replace usages of corev1.LocalObjectReference
+//     with ConfigMapReference.
+//   * Only the Name field is honored; other LocalObjectReference fields (if any)
+//     are not supported here.

	return errors.Wrapf(err, "failed to delete mongod config map %s", name)
	return errors.Wrapf(err, "failed to delete hookscript config map %s", name)

K8SPSMDB-1531: add hookScript #2155

Are you sure you want to change the base?

K8SPSMDB-1531: add hookScript #2155

Uh oh!

Conversation

pooknull commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DESCRIPTION

CHECKLIST

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hors left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

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!

K8SPSMDB-1531: add `hookScript` #2155

K8SPSMDB-1531: add `hookScript` #2155

pooknull commented Dec 19, 2025 •

edited

Loading