Cluster sync adj in p&a flavour

[M4KIF]

Description

Rewritten the sync logic

• moved each component checks to separate objects all implementing the Condition interface by which we iterate and check the health.

Key Changes

• interfaced the health check
• implemented the above for provisioner, pooler, configmap, secret
• short unit test added
• added constants and state check via bitmask

Testing and Verification

local integ suite passes (make test) and units (pkg/postgres/cluster/core go test) passes

Related Issues

Jira tickets, GitHub issues, Support tickets...

PR Checklist

• [nie wiem] 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.

[github-actions]

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contribution License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment with the exact sentence copied from below.

---

I have read the CLA Document and I hereby sign the CLA

---

_{You can retrigger this bot by commenting recheck in this Pull Request}

[mploski]

sound like a great thing to add to the specific port capabilities :-)

[M4KIF]

Then the interface here would be implemented by the ports. Clean and nice idea

[mploski]

I think our secondary ports should reflect that we create cluster and database and we should map our interfaces around it.

[mploski]

I think the idea here was to decouple status check from cnpg status, At the same time we also check health after every stage and we move forward only if we are ok, if it is still in progress we requeue or raise error. Here we have a code we can use to check where we are with status iteratively, but I dont see yet how it solve our core problem

[M4KIF]

we build our state here, after checking all ports for readyness/not dying there is the moment for us to decide what happened and how It happened.
We don't set our state as state == cnpgVariable mapped to ours, we decide what do we want to do with the fact that cm's, secrets, provisioner etc. are ready.

[mploski]

tbh business string is redundat here. Core itself is already a domain

[M4KIF]

I agree It's redundant, here It's a tradeoff for verbosity and segregation of components. And a service pattern at once, ie. the service/ is the primary port (reconciler that we provide) implementation. core/ is core, and ports/ are the contracts that we need for the core to work. They can grow large, hence the whole separate dir for ports.

[mploski]

I think all of this conditions check should be part of our Ports, also how you want to map condition to phase?

[M4KIF]

I agree, they were placed here as what I need. Solving It like you say is the thing I'm hoping for. For the provisioner/cluster etc. ports to include an interface for checking It's state.

Then the adapter would be essentially mapping the dependency state to our abstraction of It's state. Ie. we have cluster ready, provisioning, failover, cupcake, coffee etc.
Mapping condition to phase would be the job of the facade, ie. the cluster.go. That would be the whole operational brain behind. Ie. lot's of individual pieces funneled into our business decisions.

[M4KIF]

Sth. like a stateMapper, or another objects that specialises in deciding on what phase/condition we're in could also be born. The bit mask could be used for covering the phase 1, ie. state = FinaliserNotAdded && !ClusterProvisioning etc.
Phase 2 -> state == Finaliser && ClusterReady etc.

[mploski]

how do we want to check the actual condition component has in status?

[M4KIF]

wdym? It's kind of the job of the adapter to test and provide that the state is actual. Ie. If we place this as a method of a port, and implement It via adapters. We actually won't work on the real state of the component in our core. Only on our understanding of It.

[M4KIF]

Currently It would be just to copy paste the thing that we do inside cluster.go, ie. the resource obj. of the Pooler, k8s.Get(obj, ...) and all similar. As there is no abstraction currently.

[mploski]

what we try to achieve here? Is it a bitmask? Since you use IOTA we endup in having just random integer?
That would be probably simpler to just use struct with keeping the state like that:

 type ClusterState struct {                                                                                                                                         
      Provisioner ComponentPhase                                                                                                                                     
      Pooler      ComponentPhase                                                                                                                                     
      ConfigMap   ComponentPhase                                                                                                                                     
      Secret      ComponentPhase                                                                                                                                     
  }  

[M4KIF]

It's a bitmask. And It does the same job of keeping a struct with aditional field.
And It kinda solves the case of having to create new types just for each component.
Just adding additional states to the state machine, ie. the values in the "enum". The iota usage is an enum in go: https://yourbasic.org/golang/iota/
It's the first usage in this file, hence It's basically an enum from 0

[mploski]

And It kinda solves the case of having to create new types just for each component.

We already create types for a new component for many reasons, so what problem it really solve? I agree it is smart way of doing this, but neverthless if new component arise you need to add it to the const and extend types. I feel like we trade go readability for a really small c-like optimisation especially with this model of bitwise comparision later:

state |= componentHealth.State
if state&pgcConstants.ComponentsReady == pgcConstants.ComponentsReady

Also, if we build state incrementally it means that the LAST successful state in state machine is an final success. With this in mind we dont need to check every other component state afterwards.

Can we do this simpler so when Im wake up at 5am in the morning I easily understand the code?

[M4KIF]

I agree that this state check, after the iteration health check passes is redundant here.
And taking into consideration the potential future work. Which could include a file division.
I could expand the *healthCheck types with them returning the *(component)StateDto instead of relying on generic state bits.

[M4KIF]

Because later, in the very near future It seems that the project could follow this footstep of having some separation in phases and It's crucial elements. Like we've discussed on the p&a ideas brainstorm.

[mploski]

Not sure if this logic is not broken. What happens if we set ProvisionerReady but later in stage we set failed for something. Sue to way we set state and components this would pass. I think it is because iota incrementing by 1 not by power of 2? I think we should rely not on bitmasking here, but simple struct with state for every stage and if all is good we are good

[M4KIF]

with unsetting the bit's at any space, this condition starts failing. As well as with not setting the bits, the values don't AND, hence if we mark ProvisionerReady and then set masks for ConfigMapFailed, it won't fire.
And to prove how this logic would work, there would be tests that would make sure any misfires aren't possible.

[mploski]

so after every phase that is not immediate like cluster creation we should also incorporate state check rediness. I think we discussed that we dont really need to check at the end assuming we check intermediary statuses per phase?

[M4KIF]

I agree with that, but Isn't then the scope == refactor the reconciler?
I've tried to stick with changing the sync logic and doing the ground prep for more changes in coming tickets and potential p&a rework.

[mploski]

If we run this at the end of reconcillation it seems that some of the code is dead i.e if we are here, we cannot have a configmap or secret orphaned. If we do it should be discovered during this phase and requeue/err

[mploski]

Please follow our logging strategy: https://splunk.atlassian.net/wiki/spaces/CCP/pages/1079831167399/PostgreSQL+Controllers+Logging+Strategy

[mploski]

If we run this code on every phase separately, here we should requeue

[mploski]

thois could be potentially method on a state so it reads natually at 4 am i.e
func (s State) HasAll(required State) bool {
return s&required == required
}
if state.HasAll(pgcConstants.ComponentsReady) {}

[M4KIF]

after changing the state idea, It currently isn't used that much.

[mploski]

I like providing interface like that! One doubt I have is about pureness and responsiblity of those condition methods. They check conditions but also fetch k8s objects. IMO k8s objects should be evaluated and the reconciller kickoff and propagated

[M4KIF]

I've proposed another approach. Which is quite similiar to the current tbh

[mploski]

I like the direction we are heading two!
Few concerns two discuss:

1. Should we aggregate the status check at the end or we should check per phase and when we pass everything in reconcile then we know we are good. Considering we build state incrementally and sequentally this make sense IMO
2. Function pureness
3. bitmask usage in that form - if 1 is true then we might not need it. If we need it mabe we can wrap it in helper functions/refactor the approach so we understand the code better in the morning when we have P0
4. I believe we should start with drowing current state machine and how we set a phase and conditions. Did you do that?

[M4KIF]

I like the direction we are heading two! Few concerns two discuss:

1. Should we aggregate the status check at the end or we should check per phase and when we pass everything in reconcile then we know we are good. Considering we build state incrementally and sequentally this make sense IMO
2. Function pureness
3. bitmask usage in that form - if 1 is true then we might not need it. If we need it mabe we can wrap it in helper functions/refactor the approach so we understand the code better in the morning when we have P0
4. I believe we should start with drowing current state machine and how we set a phase and conditions. Did you do that?

1. I see and understand that, per phase check seems reasonable. Double checking is "majtki na majtki"
2. yup, health which does business.
3. simplified that a bit
4. I didn't draw the current state machine.

[vivekr-splunk]

I found one blocking regression in the status/event flow.

[vivekr-splunk]

This refactor appears to drop cluster phase transition events entirely. Before the change, the controller captured the old/new phase around the final status sync and called rc.emitClusterPhaseTransition(...); that helper still exists in events.go, but I no longer see it invoked on the happy path or on degraded transitions. From the product point of view, that means users lose the ClusterReady / ClusterDegraded event stream even though status still changes, which looks like a behavior regression rather than just an internal refactor.

[M4KIF]

I agree, I've missed this when attempting the rewrite. Thank you for your input and I agree, I've split the logic and focused only on the buzzwords/keywords and totally went dark on observability/event part.

[mploski]

I see that the condition types SecretReady, ManagedRolesReady, and ConfigMapReady have not been added.

At the moment, we only define clusterReady and poolerReady. The functions secretModel.Converge, configMapModel.Converge, and the provisioner all update clusterReady, which means the status does not reflect the more granular state described in the ticket.

According to the ticket, the expected status snapshot should be:

SecretReady = True (SuperUserSecretReady)
ManagedRolesReady = True (ManagedRolesReconciled)
ConfigMapReady = True (ConfigMapReconciled)

The reason values also need adjustment. Currently, secretModel.Converge uses:

reasonClusterBuildSucceeded for the success case, and
reasonUserSecretFailed for the "ref not yet written" pending case.

Both are inherited from the old clusterReady vocabulary. However, the ticket specifies that the reasons should be SuperUserSecretReady and SuperUserSecretFailed, so these should be updated accordingly.

[mploski]

ManagedRoles (Step 3) is currently handled inside provisionerModel.Actuate, but it would be clearer if it were implemented as a separate pipeline step.

Right now:

There is no ManagedRolesReady condition.
There is no distinct Step 3 in the pipeline.
Observers cannot distinguish between a failure in provisioning CNPG and a failure during role reconciliation.

Separating this into its own step would make the pipeline state clearer and improve observability of failures and it is also one of the acceptance criteria :-)

[mploski]

Thing to fix once we split phases per step.

manageComponentHealth never writes Condition=True on the success path.

The function has three branches:

error → writes False
pending state → writes False
ready → writes nothing

As a result, the pipeline only ever stamps conditions as False. The only place where Condition=True is written during reconciliation is the final call.

This happens only after all steps succeed, and it updates clusterReady only.

This breaks the audit-trail invariant described in the ticket. After a fully healthy reconcile, conditions like SecretReady, ManagedRolesReady, and ConfigMapReady never appear in etcd.

If CNPG enters a switchover during the next reconcile, the provisioner returns early with ClusterReady=False, and those provisioning conditions are still absent, not True. As a result, an operator running kubectl describe cannot distinguish between:

“the secret was provisioned successfully and is still healthy”, and
“the secret provisioning step has never run.”

[mploski]

previously we were setting:

  cluster.Status.ConnectionPoolerStatus = &ConnectionPoolerStatus{Enabled: true}                                                                         

when everything was fine, currently we miss it. We use it in database.go to check if we should use connection pooler. Without it we will be always using direct cnpg endpoint

[mploski]

we should guard this logic in unit tests/integration tests against future regressions in contract

[mploski]

this seem to be fragile if we change order.

[mploski]

I think this gate can never actually fire, If CNPG is not healthy, provisionerModel.Converge returns a pending/failed state → phase() returns a non-zero result → the first loop returns early.

Also this should set its own condition not clusterReady

[M4KIF]

Yes, that's the case under usual condition. It's a double caution safety. So It can be deleted if the system is to be trusted enough. My bet would be to leave It for now.
Condition altered.

[mploski]

ok can you share what would be the case it fires? My understanding is that this is dead code, what do I miss? If this is dead code, we simply dont need it

[M4KIF]

It mostly dead code, shouldn't be fired. So yeah, deleted.

[mploski]

Not sure if I read it right but The helper reads exactly one event from the channel per call and returns on the first read regardless of whether it matches. If the target event is the
second event in the channel, the Eventually loop never finds it.

[M4KIF]

the eventually loop calls It repeatedly until it finds or times out

[mploski]

why not to do this in one run? I.e pick all events and iterate over them. In current situation received can be also not accurate i.e recorder.Events != received if you return true early. Plus if we take one event why for :-)

[mploski]

Ah I just realised that the received will not be accurate as we do copy inside of the function by appending. we should pass * if you want to update this value

[M4KIF]

i agree with the slice pointer. I didn't understand, but yeah, the fake recorder is just a channel to which we write some strings.

[M4KIF]

eventually + for single exit seems like an overkill, as It's an await for a real async scenario.
But, we write to the channel, w/o async. So a for w/ contains is enough

[mploski]

why we are not iterating over full list of components in line 180?

[M4KIF]

because managedRoles, Pooler, ConfigMap models need some information from previous stages and injecting It needs to happen in-between.
The later phases should utilise ports in later iterations of the source to use the information they need to function.

[mploski]

Id rather name it clusterComponent not provisionerComponent to provide better code readability. Provisioner is notion of external postgres provider that can do many things including cluster,roles,database, pooler etc

[M4KIF]

yeah, a good idea tbh

[mploski]

I wonder if we cannot merge this interfaces into one i.e

type component interface {
Actuate(ctx context.Context) error
Converge(ctx context.Context) (componentHealth, error)
EvaluatePrerequisites(ctx context.Context) (prerequisiteDecision, error)
Namer() string
}

they seem to be logically bounded and I dont see a use-case to use this interfaces in separation i.e we cannot have component that doesnt have function to actuate, converge etc.We can always split it later if it grows too much, or we have different usecase

[M4KIF]

I agree, I tend to go over the board with verbosity. I agree component containing the above methods is bound, logical and consistent.

[mploski]

this should be Provisioning state, and it is also not an error

[M4KIF]

ack

[mploski]

this should be probably reasonSuperUserSecretReady not reasonClusterBuildSucceeded

[M4KIF]

yeah, as we saw in the kubectl describe. Thanks! adjusted

[mploski]

this should be probably reasonConfigMapReady reason

[M4KIF]

ack

[mploski]

i see you removed poolerInstanceCount usage, but did we delete the function?

[M4KIF]

It was left hanging in the units. Deleted It

[mploski]

@M4KIF did a review + additionally passed pr through LLM. Few points brought:

A. managedRolesModel.EvaluatePrerequisites uses State=Failed for a blocked gate

func (m *managedRolesModel) EvaluatePrerequisites(_ context.Context) (prerequisiteDecision, error) {
if m.runtime == nil || !m.runtime.IsHealthy() {
return prerequisiteDecision{
Allowed: false,
Health: componentHealth{
State: pgcConstants.Failed, // ← wrong
Condition: managedRolesReady,
Reason: reasonManagedRolesFailed,
Phase: failedClusterPhase,
Result: ctrl.Result{RequeueAfter: retryDelay},

The gate blocks because CNPG isn't healthy yet — that's a transient wait, not a failure. But because the phase() driver calls component.Converge(ctx) when a
gate blocks, and Converge also checks !m.runtime.IsHealthy() and returns State=Failed with err != nil, this path returns a non-nil error from phase() and
propagates up as a hard failure. The controller will log an error and return from the reconcile with an error, which triggers exponential backoff rather than
the RequeueAfter: retryDelay specified in the health. The state should be Provisioning with a requeue, matching the pattern every other blocked gate uses.

B. managedRolesModel.Converge duplicates the prerequisite check and returns an error for a wait state

func (m *managedRolesModel) Converge(ctx context.Context) (health componentHealth, err error) {
...
if m.runtime == nil || !m.runtime.IsHealthy() {
m.health.State = pgcConstants.Failed
...
return m.health, fmt.Errorf("managed roles blocked until CNPG cluster is healthy")
}

This is the same condition as the prerequisite gate — structurally unreachable when called normally (gate would have blocked), but reached in the blocked-gate
path where phase() calls Converge directly when !gate.Allowed. The err != nil causes phase() to return an error rather than health.Result. This results in
exponential backoff for a completely normal "wait for CNPG to be healthy" state.

Compare how the provisioner handles this: its blocked gate path sets State=Pending, err=nil so the driver returns the requeue result cleanly.

C. configMapModel.EvaluatePrerequisites is structurally unreachable but still has a blocking problem

The gate is now dead code (structurally can't fire since the provisioner gates the first loop). However it uses configMapsReady (correct condition type). The dead code concern remains — if the pipeline is reorganised, the gate will incorrectly return early with State=Provisioning and then the blocked gate path in phase() calls Converge, which also checks !IsHealthy() and returns State=Provisioning, err=nil, safely causing a requeue.

[M4KIF]

⏳ CLA signed — now checking Code of Conduct status...

[limak9182]

correct me if I'm wrong but is this event emitted on every reconcile? Normal events should be emitted only once. In this case only when pooler is actually created

[M4KIF]

yes, in this iteration It hasn't been guarded from firing every time.

[DmytroPI-dev]

we can use tagged switch on op, like this:

	switch op {
case controllerutil.OperationResultCreated:
		c.events.emitNormal(c.cluster, EventConfigMapReady, fmt.Sprintf("ConfigMap %s created", desiredCM.Name))
	case controllerutil.OperationResultUpdated:
		c.events.emitNormal(c.cluster, EventConfigMapReady, fmt.Sprintf("ConfigMap %s updated", desiredCM.Name))
	}

[M4KIF]

this block got changes after this comment, but your idea is good. Thanks.

[DmytroPI-dev]

We have configKeyClusterREndpoint in types.go ,which looks unused.

[DmytroPI-dev]

Looks like this is not used anymore with your changes?

[M4KIF]

again, a big thing. Reverted to using them, no magic values left. Thank you!

[DmytroPI-dev]

Isn't it not used anymore?

[M4KIF]

great catch, It's true. And It should be, as I was using magic values instead of those two. Placed them back into logic.

[DmytroPI-dev]

These looks like unused, same for reasonCNPGClusterNotHealthy conditionReasons = "CNPGClusterNotHealthy"

[DmytroPI-dev]

This one is unused anymore, I guess?

[DmytroPI-dev]

Are we using defer() instead of explicit error handling?

[M4KIF]

wdym error handling?
Deffer here is to supply status write at every possible branch exit of the Converge function. It's more of a decorator pattern, ie. the method does what It wants, and we like to ensure the status write happens no matter what.

[DmytroPI-dev]

Failed + RequeueAfter is the contradiction here, I guess. syncManagedRolesStatusFromCNPG just ran above this and always sets a non-nil ManagedRolesStatus — so this nil branch can only be hit if syncManagedRolesStatusFromCNPG itself received a nil cnpgCluster (guarded by the early return inside it). In practice it's a transient "CNPG hasn't populated its status yet" case, which should be Provisioning, not Failed, WDYT?

[M4KIF]

So in theory It's a dead case, as It would have to have cnpgCluster == nil and above It is such a condition:

But, as a double caution, I will change the Failed to provisioning as It indeed seems more transient.

[M4KIF]

You know what, I would leave It at failed. As we seem guarded enough against cnpg not healthy etc.
And having a generic failure at nil check seems more like It for me.
If we ever execute this code (status == nil ) because of cnpg not being healthy, that's a sideeffect and a signal something was missed by us.

[mploski]

I agree with Dmytro here. This branch is dead, because status cannot be nil. i.e postgresCluster cannot be nil as this is why we are called in first place :-) cnpgCluster cannot be nil since we passed the clusterModel.

Even if we constructed a hypothetical where nil was reached, the branch sets m.health and returns an error — but the defer writeComponentStatus would then call c.Status().Update(ctx, cluster) on a nil cluster, which would panic. It's not defensive code IMO but it is a bit misleading code that implies a real production scenario.

[DmytroPI-dev]

@M4KIF , reasonCNPGClusterNotHealthy conditionReasons = "CNPGClusterNotHealthy" (line 124 in types.go can be removed

[DmytroPI-dev]

The previous version emitted EventConfigMapReady on create and update. That's now gone — a configmap drift correction or first-time creation happens with no event. Converge only emits when the key check passes on the next reconcile. The gap means ops has no signal that a configmap was actually written.

[M4KIF]

I suggest to fix this with a sort of "contracts as tests PR" to have the behavior locked in. It seems final and It constantly get's missed by me as I don't have the hands on results available via tests.
It would be TDD, ie. propose tests with expected events per n reconciles/n behaviours (drift correction)/unhealthy transitions etc. -> align the code one final time -> make tests work and lock in the contract.
Now I'm constantly hitting and missing while shifting code with different intentions.

[DmytroPI-dev]

p.cluster.Status.Conditions is read before the deferred writeComponentStatus has persisted poolerReady=True. The transition detection inside emitPoolerReadyTransition may fire against a stale slice. Capture oldConditions before the ready assignment if the intent is to detect the False→True edge.

[mploski]

TransientError is not necessary anymore since we handle it on the Reconcile level at least for isconflict? Also isconflict shouldnt be treat the same way as toomany request the first can be retries automatically the second should go with backoff. We can clear it in the next PR though

[M4KIF]

so, collecting from the review in general ->

• tests for event emission to have them locked in without the possibility to cause side-effects with source changes any more
• event emission final alignment
• rollback the isTransient to simple isConflict(?)
• p.cluster.Status.Conditions is read before the deferred writeComponentStatus -> rethink the few loc so they can't cause undefined state in k8s(?)

[mploski]

One potential gap. What if actuate logic fails? For example, lets assume CNPG is healthy and reconcileManagedRoles fails. Then we return early here with error, but failure state you set in health is never persisted. Is it on purpose?

[M4KIF]

then writeComponentStatus doesn't fire and as far as I understand, k8s state is left "hanging" without an update and just with a plain/error requeue, right?

fe.
func (m *managedRolesModel) Actuate(ctx context.Context) error { if rolesErr := reconcileManagedRoles(ctx, m.client, m.cluster, m.runtime.Cluster()); rolesErr != nil { m.events.emitWarning(m.cluster, EventManagedRolesFailed, fmt.Sprintf("Failed to reconcile managed roles: %v", rolesErr)) m.health.State = pgcConstants.Failed m.health.Reason = reasonManagedRolesFailed
So It would be best to update status everytime, before the reconcile requeue, right?

[mploski]

if we are raising the error I believe we should do this yes.

[mploski]

Im a bit confused by this check as it seems again to be a dead code and in general about the Converge responsibility. IMO the health check for cnpg/cluster is done elsewhere, we should not duplicate this in next steps who depends on previous one. Otherwise we mix responsibilities of prerequisiteCheck and Converge. Converge should be responsible only for its resource state. prerequisite for dependencies. But there is a arch tension here i.e:
We would need to remove running Converge on gate not allowed and probably run directly status via writeComponentStatus(updateStatus, gate.Health) which also breaks boundry who can write. We need to pick one explicitly, currently the split is blured so we need to make sure we know what we are doing for future us

After second thought I started even questioning we need EvaluatePrerequisites everywhere. What problem it actually solve that the pipeline structure doesn't? the pipeline loop ordering is the prerequisite mechanism to the great exntent

[M4KIF]

I agree, the boundary seems blurred right now as well as the responsibility inside the models isn't exactly their responsibility to be fair.
In general, I would like to adjust It to the "phase locked until healthy 250% sure", ie. if we proceed further then It is a given that we don't have to worry anymore about cluster having unhealthy state 3 components later. That's a smell to be fair.
I wouldn't necessarily want to do this in this PR, as It will become even fatter and even more side-effect packed. It lacks precission rn. But If It needs to be this way I can.

[mploski]

I guess we are missing CLUSTER_R_ENDPOINT?

[M4KIF]

You're right, nice catch. Went back into the code, not into the tests. Pushing this rn

[DmytroPI-dev]

configKeyClusterREndpoint string = "CLUSTER_R_ENDPOINT" - in correspondence with this, IDE says, it is not used.

[mploski]

we should probably not override health otherwise we loose failure path from Actuate. This seems good on clusterModel, and override everywhere else. Shouldnt our tests catch it?

[M4KIF]

yup, a very serious error tbf. I'll write a test for such failure cases and align.