SCF-830: Added state management for cluster hibernation#17
Conversation
Coverage Report for CI Build 29513254178Warning No base build found for commit Coverage: 44.821%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
serdardalgic
left a comment
serdardalgic
left a comment
There was a problem hiding this comment.
Some minor issues @adshin21. After that, I would like to review it again.
| if c.Statefulset == nil || c.Statefulset.Spec.Replicas == nil { | ||
| return nil | ||
| } | ||
| return c.Statefulset.Spec.Replicas |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
Yes, but to get the current status, we need to make an API call to k8s.
| const ( | ||
| LifecycleActionNone LifecycleAction = iota | ||
| LifecycleActionHibernate // Running -> Stopping (initiate hibernate) | ||
| LifecycleActionStoppingCompleted // Stopping -> Stopped (pods fully terminated) |
There was a problem hiding this comment.
Are you using LifecycleActionStoppingCompleted ? If not, please remove it.
…sist lifecycle spec changes in Sync
serdardalgic
left a comment
serdardalgic
left a comment
There was a problem hiding this comment.
So, some stuff that caught my attention, here it is.
| isWakingUpSimple := newSpecLifecycle == nil || newSpecLifecycle.Phase != "stopped" | ||
| hasPreviousInstances := newSpecPreviousNumberOfInstances > 0 | ||
| needsRestore := newSpecNumberOfInstances == 0 | ||
|
|
||
| isWakingUp := isWakingUpSimple && hasPreviousInstances && needsRestore | ||
|
|
||
| if currentStatus.Stopped() || isWakingUp { | ||
| if isWakingUp || newSpecLifecycle == nil || newSpecLifecycle.Phase != "stopped" { | ||
| return LifecycleActionWakeUp | ||
| } | ||
| } | ||
|
|
||
| if newSpecLifecycle != nil && | ||
| newSpecLifecycle.Phase == "stopped" && | ||
| !currentStatus.Stopping() && | ||
| !currentStatus.Stopped() { |
There was a problem hiding this comment.
I'd write this part a bit simpler.
isWakingUp already requires isWakingUpSimple to be true (it's isWakingUpSimple && hasPreviousInstances && needsRestore), so checking isWakingUp || isWakingUpSimple in the inner if is redundant, it's the same as just checking isWakingUpSimple. That makes the nested if hard to follow: you have to trace three variables back to see that one term never actually changes the outcome. Splitting it into two flat, named checks makes the same logic easier to read:
| isWakingUpSimple := newSpecLifecycle == nil || newSpecLifecycle.Phase != "stopped" | |
| hasPreviousInstances := newSpecPreviousNumberOfInstances > 0 | |
| needsRestore := newSpecNumberOfInstances == 0 | |
| isWakingUp := isWakingUpSimple && hasPreviousInstances && needsRestore | |
| if currentStatus.Stopped() || isWakingUp { | |
| if isWakingUp || newSpecLifecycle == nil || newSpecLifecycle.Phase != "stopped" { | |
| return LifecycleActionWakeUp | |
| } | |
| } | |
| if newSpecLifecycle != nil && | |
| newSpecLifecycle.Phase == "stopped" && | |
| !currentStatus.Stopping() && | |
| !currentStatus.Stopped() { | |
| wantsStopped := newSpecLifecycle != nil && newSpecLifecycle.Phase == "stopped" | |
| // The cluster was in the middle of hibernating (already scaled down, | |
| // old replica count saved) when the spec changed its mind and no | |
| // longer wants it stopped. | |
| if !wantsStopped && newSpecPreviousNumberOfInstances > 0 && newSpecNumberOfInstances == 0 { | |
| return LifecycleActionWakeUp | |
| } | |
| // Already stopped and the spec no longer asks to stay stopped. | |
| if currentStatus.Stopped() && !wantsStopped { | |
| return LifecycleActionWakeUp | |
| } | |
| if wantsStopped && !currentStatus.Stopping() && !currentStatus.Stopped() { | |
| return LifecycleActionHibernate | |
| } |
I checked this produces the same result as the original in every case, and all 9 existing detectLifecycleTransition unit tests still pass with no changes needed.
| * Suspend the logical backup CronJob (if enabled) | ||
| * Set the cluster status to "Stopping", then "Stopped" | ||
|
|
||
| When this field is removed from a stopped cluster, the operator will: |
There was a problem hiding this comment.
One small suggestion, just to be more precise here: this doesn't just trigger on removing the field, setting phase to anything other than "stopped" (including "") also wakes the cluster up:
| When this field is removed from a stopped cluster, the operator will: | |
| When this field is removed, or set to any value other than `"stopped"` | |
| (including an empty string), on a stopped cluster, the operator will: |
|
|
||
| // LifecycleSpec describes the lifecycle state of a Postgres cluster. | ||
| type LifecycleSpec struct { | ||
| Phase string `json:"phase,omitempty"` |
There was a problem hiding this comment.
Phase is a bare string and the only value the code ever checks for is "stopped", everything else means "not stopped." Since there's no plan for other phase values at least for now, I'd suggest making that explicit rather than leaving it open-ended:
- A typed constant, e.g.
type LifecyclePhase string+LifecyclePhaseStopped LifecyclePhase = "stopped"added topkg/apis/acid.zalan.do/v1/const.go, used inlifecycle.go:39andcluster.go:1288instead of the literal"stopped". - A CRD
enum: ["", "stopped"]onspec.lifecycle.phase(manifests/postgresql.crd.yaml) so a typo like"Stopped"is rejected at admission instead of silently no-op'ing.
Since this PR is the one introducing spec.lifecycle.phase in the first place, there's no existing-cluster migration risk, worth just adding the enum: now rather than as a follow-up.
|
|
||
|
|
There was a problem hiding this comment.
This is also a mistake, I guess.
3 participants
No description provided.