Finalizers for postgrescluster#1748
Finalizers for postgrescluster#1748limak9182 wants to merge 3 commits intofeature/database-controllersfrom
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
DmytroPI-dev
force-pushed
the
finalizers-for-postgrescluster
branch
from
5de1272 to
f8ec473
Compare
|
|
||
| // ConnectionPoolerConfig defines PgBouncer connection pooler configuration. | ||
| // When enabled, creates RW and RO pooler deployments for clusters using this class. | ||
| // +kubebuilder:validation:XValidation:rule="!self.connectionPoolerEnabled || self.connectionPoolerConfig != null || (self.cnpg != null && self.cnpg.connectionPooler != null)",message="connectionPoolerConfig must be set in cluster spec or class when connectionPoolerEnabled is true" |
There was a problem hiding this comment.
why do we remove this validation?
There was a problem hiding this comment.
Fixed, it was causing an error if placed here, as rule references fields (connectionPoolerEnabled and cnpg) that do not exist within ConnectionPoolerConfig, so placed validation rule on top level
| type: object | ||
| x-kubernetes-validations: | ||
| - message: Storage size cannot be removed and can only be increased | ||
| - messageExpression: '!has(self.postgresVersion) ? ''postgresVersion cannot |
There was a problem hiding this comment.
this seems weird why we have validation for postges here?
There was a problem hiding this comment.
Using these validation rules on field level were failing, because type 'string' does not support field selection, and we have PostrgesVersion as a string. Validation rule for Storage also failed on field level, so I put them on PostgresClusterSpec level, to be able comparing objects. And when generating CRDs, they are shown on ClusterSpec (spec) level, what is expected, I assume.
| # Takes precedence over the class-level connectionPoolerEnabled value | ||
| connectionPoolerEnabled: true No newline at end of file | ||
| connectionPoolerEnabled: true | ||
| connectionPoolerConfig: |
There was a problem hiding this comment.
We agreed with @limak9182 if I recall, that on the cluster side we can only disable or enable config and details are treated as a specific class provider details. What is the reason we changed it?
There was a problem hiding this comment.
Fixed this, now only if pooler is enabled and config is present in Class, creates pooler.
| logger.Error(err, "Failed to add finalizer to PostgresCluster") | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{}, nil // requeue via watch |
There was a problem hiding this comment.
why do we have this comment here?
| // Adding finalizer logic here to ensure poolers and other resources are cleaned up when PostgresCluster is deleted. | ||
| if !postgresCluster.ObjectMeta.DeletionTimestamp.IsZero() { | ||
| // TODO - to delete CNPG Cluster as well, currently relying on owner reference for cleanup, but we might want to have more control and update status during deletion, so implementing explicit deletion here as well. | ||
| if err := r.deleteCNPGCluster(ctx, postgresCluster); err != nil { |
There was a problem hiding this comment.
should we have finalizer logic as a separate function?
There was a problem hiding this comment.
in that case our update status would be also simplified
There was a problem hiding this comment.
Moved to separate method, you are correct.
| } | ||
|
|
||
| // deleteCNPGCluster deletes the CNPG cluster and its associated resources if they exist. | ||
| func (r *PostgresClusterReconciler) deleteCNPGCluster(ctx context.Context, postgresCluster *enterprisev4.PostgresCluster) error { |
There was a problem hiding this comment.
In our ERD https://cisco.sharepoint.com/:w:/r/sites/StructuredStorageWLM/_layouts/15/Doc.aspx?sourcedoc=%7BF5CD0E79-31E6-4AC1-9E10-AA875BCFF112%7D&file=Managed%20Postgres%20Service%20in%20CMP-K%20-%20ERD%20v2.docx&nav=eyJjIjo2MDY2NjM5MTN9&action=default&mobileredirect=true we assume user can provide databaseDeletionPolicy with Retain/Delete if it is retain we should remove our PostgresCluster CR but perceive Cluster CR
| } | ||
|
|
||
| logger.Info("Deleting CNPG Cluster", "name", cnpgCluster.Name) | ||
| if err := r.Delete(ctx, cnpgCluster); err != nil && !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
We need to also decide if we should remove cluster if we have customer databases configured
3 participants
Description
What does this PR have in it?
Key Changes
Highlight the updates in specific files
Testing and Verification
How did you test these changes? What automated tests are added?
Related Issues
Jira tickets, GitHub issues, Support tickets...
PR Checklist