remove observedGeneration from postgresDatabase by DmytroPI-dev · Pull Request #1812 · splunk/splunk-operator

DmytroPI-dev · 2026-04-02T13:57:22Z

Description

This PR aligns PostgresDatabase with the watch-driven reconciliation approach used for PostgresCluster. It removes the reconcile skip based on ObservedGeneration, reacts to owned Secret/ConfigMap drift even when the spec is unchanged, repairs configmap drift declaratively, and degrades status instead of recreating previously provisioned managed secrets that were deleted.

Key Changes

postgresdatabase_controller.go add explicit drift-trigger predicates for owned Secret, ConfigMap, and relevant CNPG Database changes. Conflict retries are requeued without surfacing noisy reconcile errors.
database.go remove the ObservedGeneration short-circuit and revalidated stages on every reconcile. Add drift handling for managed user secrets, configmap repair/re-adoption, retained-resource adoption, conflict-friendly database reconcile handling, and quieter ready/not-found paths.
events.go Add events for drift and retained-resource adoption.
cluster.go reduced log noise on transient status-update conflicts.

Testing and Verification

Update controller tests in postgresdatabase_controller_test.go for:
- configmap drift repair
- deleted configmap recreation
- deleted managed secret degradation without recreation
- secret ownership re-adoption
- adding a new database to an already provisioned resource
- secondary-resource watch predicates
- cluster-missing and role-conflict regressions
Add unit tests in database_unit_test.go for secret/configmap drift and adoption paths.
Verified behavior from controller logs/events during delete/recreate flows with both Delete and Retain policies.
Automated tests were updated. All tests passed

Related Issues

CPI-1950

PR Checklist

Code changes adhere to the project's coding standards.
Relevant unit and integration tests are included.
Documentation has been updated accordingly.
All tests pass locally.
The PR description follows the project's guidelines.

internal/controller/postgresdatabase_controller.go

mploski · 2026-04-07T06:35:45Z

internal/controller/postgresdatabase_controller.go

+		predicate.GenerationChangedPredicate{},
+		predicate.Funcs{
+			UpdateFunc: func(e event.UpdateEvent) bool {
+				oldObj, okOld := e.ObjectOld.(*cnpgv1.Database)


whats the difference between this predicate and GenerationChangedPredicate?

If my understanding is correct, GenerationChangedPredicate is triggered only when CNPG Database spec changes. If we have status-only update, generation might not always change. Owner reference changes on the owned CNPG Database would also stop triggering reconciliation.

mploski · 2026-04-07T07:11:15Z

internal/controller/postgresdatabase_controller_test.go

 }

-func expectReadyStatus(current *enterprisev4.PostgresDatabase, generation int64, expectedDatabase enterprisev4.DatabaseInfo) {
+func expectReadyStatus(current *enterprisev4.PostgresDatabase, expectedDatabase enterprisev4.DatabaseInfo) {


I'd still leave generation in status. Reason - we will know what is the CRD version status was emited for

Returned this back.

mploski · 2026-04-07T07:20:22Z

pkg/postgresql/database/core/database.go

 	c := rc.Client
 	logger := log.FromContext(ctx).WithValues("postgresDatabase", postgresDB.Name)
 	ctx = log.IntoContext(ctx, logger)
+	defer func() {


Why do we want to do defer here? We always catch the errors explicitly close to the called function so what benefit this brings?

Remove defer, add explicit requeue on conflict.

mploski · 2026-04-07T07:21:52Z

pkg/postgresql/database/core/database.go

+		err = nil
+	}()
 	logger.Info("Reconciling PostgresDatabase")
+	wasReady := postgresDB.Status.Phase != nil && *postgresDB.Status.Phase == string(readyDBPhase)


Could you explain what it does and how we use it?

The idea was to reduce EventPostgresDatabaseReady on every successful reconcile.

mploski · 2026-04-07T08:32:44Z

pkg/postgresql/database/core/events.go

 	EventClusterNotReady               = "ClusterNotReady"
 	EventRoleConflict                  = "RoleConflict"
 	EventUserSecretsFailed             = "UserSecretsFailed"
+	EventUserSecretsDriftDetected      = "UserSecretsDriftDetected"


TIL: we should start using roles instead of users

mploski · 2026-04-07T08:48:02Z

pkg/postgresql/database/core/database.go

 }

-func reconcileUserSecrets(ctx context.Context, c client.Client, scheme *runtime.Scheme, postgresDB *enterprisev4.PostgresDatabase) error {
+func reconcileUserSecrets(ctx context.Context, c client.Client, scheme *runtime.Scheme, postgresDB *enterprisev4.PostgresDatabase, existingDatabases map[string]struct{}) error {


this function should not be bothered with logic checking what db is already provisioned, it should be clean

Removed logic checking.

mploski · 2026-04-07T08:50:11Z

pkg/postgresql/database/core/database.go

 }

-func ensureSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, postgresDB *enterprisev4.PostgresDatabase, roleName, secretName string) error {
+func ensureSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, postgresDB *enterprisev4.PostgresDatabase, roleName, secretName string, databaseAlreadyProvisioned bool) error {


this function should not be responsible for checking if db is already provisioned

Removed logic checking.

mploski · 2026-04-07T08:52:01Z

pkg/postgresql/database/core/database.go

+	}
+}
+
+func validateManagedSecret(secret *corev1.Secret, roleName string) error {


I wonder why we need this in the first place? What would be the use-case for example username not matching roleName?

To prevent adopting secret in case of manual changes, for example. Does this makes sense?

M4KIF · 2026-04-09T05:41:24Z

internal/controller/postgresdatabase_controller_test.go

 	Expect(current.Status.Databases[0].AdminUserSecretRef).NotTo(BeNil())
 	Expect(current.Status.Databases[0].RWUserSecretRef).NotTo(BeNil())
 	Expect(current.Status.Databases[0].ConfigMapRef).NotTo(BeNil())
+	Expect(current.Status.ObservedGeneration).NotTo(BeNil())


Isn't that supposed to be removed from the general logic?

Yes, we removed observedGeneration to act as a database readiness check, but left it to have an idea know what is the CRD version status was emitted for. See here

M4KIF · 2026-04-09T05:44:35Z

pkg/postgresql/database/core/database.go

 	case ClusterReady:
 		rc.emitOnConditionTransition(postgresDB, postgresDB.Status.Conditions, clusterReady, EventClusterValidated, "Referenced PostgresCluster is ready")
 		if err := updateStatus(clusterReady, metav1.ConditionTrue, reasonClusterAvailable, "Cluster is operational", provisioningDBPhase); err != nil {
+			if result, conflictErr, ok := requeueOnConflict(err, "persisting cluster ready status"); ok {


Do I understand correctly, adding this logic is a replacement for observed generation?

If asking about this:

if result, conflictErr, ok := requeueOnConflict(err, "persisting cluster ready status"); ok { return result, conflictErr }

this is to catch errors on version conflicts and requeue immediately to soothe reconciliation storm.

M4KIF · 2026-04-09T05:47:14Z

pkg/postgresql/database/core/database.go

+		if result, conflictErr, ok := requeueOnConflict(err, "reconciling user secrets"); ok {
+			return result, conflictErr
+		}
+		var driftErr *secretReconcileError


are we sure, that It will always be drift related?
What I'm after is having It coupled to the name secretReconcileError, which seems quite generic. Even If the underlaying struct could be verbose and drift only in values.

right now, we have reason: reasonSecretsDriftDetected (see here and here ), but this makes sense in general, will rename it as secretErr

M4KIF · 2026-04-09T05:52:19Z

pkg/postgresql/database/core/database.go

 	updateStatus := func(conditionType conditionTypes, conditionStatus metav1.ConditionStatus, reason conditionReasons, message string, phase reconcileDBPhases) error {
 		return persistStatus(ctx, c, postgresDB, conditionType, conditionStatus, reason, message, phase)
 	}
+	requeueOnConflict := func(err error, action string) (ctrl.Result, error, bool) {


more of a nit: Will having a lamda created here not cause to miss scope for us. Ie. If we have k8s conflict error occuring in a few places. Will the action log be sufficient and fully satisfying our business needs? Also, there could be some metrics done in the future from reconcilation misses(conflicts) which could prove useful for system admins.

Add typed conflict categories instead of lambda

DmytroPI-dev requested review from limak9182 and mploski April 2, 2026 14:02

mploski reviewed Apr 3, 2026

View reviewed changes

internal/controller/postgresdatabase_controller.go Outdated Show resolved Hide resolved

mploski reviewed Apr 3, 2026

View reviewed changes

internal/controller/postgresdatabase_controller.go Outdated Show resolved Hide resolved

mploski reviewed Apr 7, 2026

View reviewed changes

DmytroPI-dev added 3 commits April 8, 2026 12:46

remove observedGeneration from postgresDatabase

6ee9c43

resolved reviewer's comments

40c0d8b

address reviewer comments on

c333f1e

DmytroPI-dev force-pushed the check-observed-generation-usage-database-CR branch from 0b9acab to c333f1e Compare April 8, 2026 11:05

fix intergration tests

774d5ec

M4KIF reviewed Apr 9, 2026

View reviewed changes

update requeue on conflict method

f3c1063

Conversation

DmytroPI-dev commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes

Testing and Verification

Related Issues

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmytroPI-dev Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

DmytroPI-dev Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M4KIF Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DmytroPI-dev commented Apr 2, 2026 •

edited

Loading

DmytroPI-dev Apr 7, 2026 •

edited

Loading

DmytroPI-dev Apr 9, 2026 •

edited

Loading

M4KIF Apr 9, 2026 •

edited

Loading