-
Notifications
You must be signed in to change notification settings - Fork 162
K8SPSMDB-1531: add hookScript
#2155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hookScriptfield 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.
hors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add it for PBM agent as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
commit: 25830b6 |
| if err := r.reconcileBackupHookscript(ctx, cr); err != nil { | ||
| return errors.Wrap(err, "reconcile backup hookscript") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to do this if backups are disabled?
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.hookScriptEach
hookScriptfield is a shell script that will be executed from the container entrypoint of the corresponding podsCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability