SCF-830: Added state management for cluster hibernation by adshin21 · Pull Request #17 · cybertec-postgresql/postgres-operator

adshin21 · 2026-05-13T09:32:20Z

No description provided.

coveralls · 2026-05-13T09:36:56Z

Coverage Report for CI Build 29651611343

Warning

No base build found for commit d16504e on scalefield_v18.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 44.986%

Details

Patch coverage: 108 uncovered changes across 7 files (272 of 380 lines covered, 71.58%).

Uncovered Changes

File	Changed	Covered	%
pkg/cluster/lifecycle.go	242	187	77.27%
pkg/cluster/util.go	14	0	0.0%
pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go	23	12	52.17%
pkg/cluster/cluster.go	57	47	82.46%
pkg/apis/acid.zalan.do/v1/util.go	9	3	33.33%
pkg/cluster/sync.go	29	23	79.31%
pkg/util/k8sutil/k8sutil.go	6	0	0.0%

Coverage Regressions

Requires a base build to compare against. How to fix this →

Coverage Stats


Relevant Lines:	15747
Covered Lines:	7084
Line Coverage:	44.99%
Coverage Strength:	16.37 hits per line

💛 - Coveralls

…er too

… in update

serdardalgic

Some minor issues @adshin21. After that, I would like to review it again.

serdardalgic · 2026-06-22T10:23:55Z

+	if c.Statefulset == nil || c.Statefulset.Spec.Replicas == nil {
+		return nil
+	}
+	return c.Statefulset.Spec.Replicas


Suggested change

return c.Statefulset.Spec.Replicas

return c.Statefulset.Status.Replicas

Shouldn't this be Status as you want to get the current replica count of the StatefulSet?

Yes, but to get the current status, we need to make an API call to k8s.

serdardalgic · 2026-06-22T11:02:06Z

+const (
+	LifecycleActionNone LifecycleAction = iota
+	LifecycleActionHibernate          // Running -> Stopping (initiate hibernate)
+	LifecycleActionStoppingCompleted // Stopping -> Stopped (pods fully terminated)


Are you using LifecycleActionStoppingCompleted ? If not, please remove it.

…sist lifecycle spec changes in Sync

serdardalgic

So, some stuff that caught my attention, here it is.

serdardalgic · 2026-07-16T15:52:50Z

+
+

Suggested change

This is also a mistake, I guess.

This one still remains @adshin21

Replace the four-line flag-juggling (isWakingUpSimple / hasPreviousInstances / needsRestore) with a wantsStopped flag plus three self-documenting guards. This removes the latent bug where the inner isWakingUp check was being dropped (Stopped + phase=stopped incorrectly returned WakeUp).

…enum Introduce LifecyclePhase string type with LifecyclePhaseStopped constant and Phase.Stopped() helper; the CRD enum accepts "" or "stopped" via +kubebuilder:validation:Enum="";stopped. Tighten blockLifecycleUpdate error message and cluster_manifest.md wake-up text to mention setting lifecycle.phase to an empty string. Generate DeepCopy for PostgresStatus.PreviousPoolerInstances and use it in Postgresql.DeepCopyInto so the operator's in-memory cache does not share map state with the cluster object.

Tighten the second wake-up guard in detectLifecycleTransition to require PreviousNumberOfInstances > 0. Without a stored replica count, transitioning to Updating with 0 replicas leaves the cluster stuck indefinitely: the sync defer skips Running when oldSpec had 0 instances, and the next periodic Sync sees the same dead-end state. The "Stopped + cleared + no previous instances" test case is renamed and updated to expect LifecycleActionNone, since the previous behavior was a recovery attempt on a corrupted state that produced a worse outcome than the cluster staying in Stopped. Update initiateWakeUp's doc comment to note that the PreviousNumberOfInstances == 0 branch is now unreachable in practice (detect refuses the transition upstream).

* Rewrite the misleading comment in syncStateLocked's defer. The old wording claimed oldSpec.Spec.NumberOfInstances == 0 during wake-up because initiateWakeUp hadn't run yet; the real reason is that c.Postgresql was already hibernated when the reconcile started. * Skip 0-replica entries in scalePoolerDown's stored map. A pooler already at 0 has nothing to restore on wake-up; storing it produced a spurious no-op patch on the way up. Update the corresponding test case to expect nil instead of {"master": 0}. * Document the PreviousNumberOfInstances == 0 limitation in cluster_manifest.md: the operator will not transition out of Stopped if the field is missing (e.g. due to a partial write or a manual edit); the user must edit spec.numberOfInstances to wake the cluster up manually.

…dates Fix bug encountered in e2e tests: user annotations (e.g. last-major-upgrade-failure) were silently dropped when a watch event arrived mid-lifecycle, causing the v16->v18 major version upgrade to proceed despite the failure annotation.

serdardalgic

Here is my review.

serdardalgic · 2026-07-23T19:49:46Z

+//   - (true, nil) if update is blocked and caller should return early
+//   - (false, nil) if update can proceed
+//   - (false, error) on error
+func (c *Cluster) blockLifecycleUpdate(newSpec *acidv1.Postgresql) (bool, error) {


This function basically doesn't block anything, the name is a bit misleading.
Shouldn't it be better to name it something like shouldBlockLifecycleUpdate or isLifecycleUpdateBlocked ?

serdardalgic · 2026-07-23T19:50:26Z

+
+

This one still remains @adshin21

serdardalgic · 2026-07-23T20:43:57Z

+// iteration refreshes c.Statefulset so the in-memory cache stays current.
+// NotFound is treated as success (the cluster may have been deleted entirely).
+// On timeout, returns an error so callers can surface it to the controller.
+func (c *Cluster) waitStatefulsetPodsGone() error {


So, AFAICS, currently hibernate implementation is as follows:
A cluster is patched to hibernate, operator calls Update():

Lock the cluster, so nothing else can touch it.

handleHibernateAndWakeUp() detects hibernate request and calls syncStateLocked(), which calls prepareLifecycleTransition() that initiates Hibernation via initiateHibernate() that sets the Spec's numberOfInstances to 0.

Then the handleHibernateAndWakeUp() calls completeStoppingTransition() when the action is Hibernate. This one calls waitStatefulsetPodsGone() which is a blocking function. In a Retry loop, it keeps asking "are the pods gone yet?" every 3 seconds (ResourceCheckInterval default value), until 10 minutes timeout (ResourceCheckTimeout default).

The lock is released only after the loop ends, either with success or a timeout.

This blocking step 3 is a bit tricky. The pod might be stuck, any referenced resource might be taking a long time to release, maybe the pod gets stuck on a node that's being drained by the autoscaler, yada yada.. there are various scenarios where a single pod ends up blocking the whole reconcilation loop for this cluster.

I think a more solid version would be a non-blocking implementation. Check this one out:

Lock the cluster, same as before

syncStateLocked(), initiateHibernate(), same as before

Now, you can ask Kubernetes are the pods gone yet? once, with something like checkStoppingCompleted() instead of completeStoppingTransition()/waitStatefulsetPodsGone()

If yes, write Stopped, done, unlock. Same persistStoppingCompletedTransition() implementation.

If no, just write/leave Stopping and unlock immediately, No waiting, no loop.

This time, you don't need to babysit the pods until they're gone. Sync() already has a Stopping() recovery branch (if c.Status.Stopping() { return c.completeStoppingTransition(newSpec) }) meant for operator-restart recovery, it can just call checkStoppingCompleted() too, and since it runs on every regular reconcile, it naturally re-checks without needing a loop anywhere.

This design was already there, but it was checking the Spec.Replicas instead of Status.Replicas. This was probably the reason you redesigned this part. You're right that it needs a live API call, I just don't think we need to implement it in a retry loop.

So, I'd suggest implementing a non-blocking version of this check instead.

SCF-830: Added state management for cluster hibernation

19ca281

adshin21 added 5 commits May 13, 2026 15:20

SCF-830: Steamlined cluster update function

8d75f56

SCF-830: Handled pooler hibernation while stopping/starting the clust…

599a263

…er too

SCF-830: Added feature documentation for cluster hibernation

ea8438f

SCF-830: Added unit test from hibernate

9424f7c

SCF-830: Added cronjob pause/resume while cluster hibernation

0f4eabe

adshin21 requested review from ermajn, hlihhovac and serdardalgic May 21, 2026 07:26

adshin21 marked this pull request as ready for review May 21, 2026 07:26

adshin21 requested a review from hughcapet as a code owner May 21, 2026 07:26

SCF-830: Refactored into a common function for spec update, pesisting…

8c8147a

… in update

serdardalgic requested a review from ants May 22, 2026 09:20

Retriggering pipeline

18a42b8

serdardalgic requested changes Jun 22, 2026

View reviewed changes

adshin21 added 2 commits July 14, 2026 00:40

SCF-830: Refactor detect lifecycle, fix Update 409 follow-up, and per…

8e7018b

…sist lifecycle spec changes in Sync

SCF-830: Added fresh replica status checking while stopping

356aa78

adshin21 requested a review from serdardalgic July 14, 2026 08:15

adshin21 added 3 commits July 16, 2026 01:58

SCF-830: Refactored and made hibernation sync cycle free

293ebfc

SCF-830: Updated the user.md and cluster_manifest.md

0af47e5

SCF-830: Ran go fmt for lifecycle and cluster changes

858b873

serdardalgic requested changes Jul 16, 2026

View reviewed changes

adshin21 added 5 commits July 18, 2026 15:22

adshin21 force-pushed the scalefield_v18-SCF-830-cluster-hibernation branch from 7f48801 to c378463 Compare July 18, 2026 16:15

serdardalgic requested changes Jul 23, 2026

View reviewed changes

	return c.Statefulset.Spec.Replicas
	return c.Statefulset.Status.Replicas

Uh oh!

Conversation

adshin21 commented May 13, 2026

Uh oh!

coveralls commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 29651611343

Coverage: 44.986%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

serdardalgic left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serdardalgic left a comment

Choose a reason for hiding this comment

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!

Uh oh!

serdardalgic left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented May 13, 2026 •

edited

Loading