canonical / canonical/postgresql-single-kernel-library

Config render performs a redundant goal-state round-trip per update_config; dedupe with the caller's read or memoize per hook invocation

Open
#294 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
4
Forks
3
Avg merge
3d 17h
Merged PRs (30d)
26

Description

## Steps to reproduce

1. Deploy a PostgreSQL charm (VM or K8s) whose charm code calls `ConfigManager.update_config()` — this runs on every `update-status` and on every config-affecting hook.
2. Observe the hook-tool traffic of a single hook invocation (DEBUG log or `juju debug-log --replay`): within one `update_config()` call, the `goal-state` hook command is executed **twice** —
- once by the charm itself (membership/lag checks such as `get_running_cluster_members() < planned_units`),
- once by the lib's render path: `managers/config.py update_config` → `render_patroni_yml_file` → `core/state.py synchronous_node_count` (cached_property per state object) → `core/peer_relation.py planned_units` (cached_property per state object) → `ops Application.planned_units()` — an **uncached** `goal-state` hook command (verified in ops 3.8.1 and 3.8.2, `ops/model.py _canonicalize_id`-adjacent `planned_units` implementation).

The two `cached_property`s dedupe only *within* a single render; the charm's own read is a separate, second round-trip in the same hook.

## Expected behavior

One `goal-state` read per hook invocation, serving both the charm's own reads and the lib's config render. ops' documented contract states the data is "up-to-date as of the start of the current hook invocation", so reads are dedupable within a hook.

Suggested options (either):
- accept the already-read value from the caller: `update_config(..., planned_units: int | None = None)` (or an equivalent state-input parameter), so the charm shares its single read with the render; or
- memoize `planned_units` per hook invocation inside the lib (safe within a hook by the snapshot semantics above).

## Actual behavior

Two `goal-state` executions per `update_config()` call — on every `update-status` and every config-affecting hook, for every unit of every charm built on this library (VM and K8s). The charm cannot avoid its half: `update_config` accepts no `planned_units` parameter (verified in 16.3.8), and a charm-side memoization cannot reach the lib's render.

Constraint for any fix: **do not cache across hook invocations.** `planned_units` includes pending units and changes on scale events, and freshness per hook is a relied-upon contract (e.g. the restore pre-checks read it fresh per action, codified in the charm's unit tests). On K8s the charm process is long-lived, so any cache must be keyed to the render/hook, not to a long-lived object.

## Versions

Operating system: Ubuntu 24.04 (observed on an LXD VM backend; the code path is substrate-independent)

Juju CLI: 3.6.28

Juju agent: 3.6.28

Charm revision: canonical/postgresql-operator `dpe-10203-async-replication-recovery` / `dpe-10203-dead-dc-recovery-test` (local builds, revision 0) and the `16/edge` published base charm; library verified at `postgresql-charms-single-kernel` 16.3.8 with ops 3.8.1/3.8.2 (same behavior)

LXD: 5.21

microk8s: not used (VM substrate observed; the path is shared with the K8s charm)

## Log output

Juju debug log:

The read chain as observed in a live reproduction (DPE-10203 dead-DC recovery work):

```
charm.update_config (charm.py)
-> lib config.py update_config
-> config.py render_patroni_yml_file
-> state.py synchronous_node_count [cached_property per state object]
-> peer_relation.py planned_units [cached_property per state object]
-> ops Application.planned_units() -> goal-state [uncached]
```

plus the charm's own separate `planned_units()` read in the same hook.

## Additional context

- Companion issue #285 covers the resilience side of the same read (guard `planned_units()` against `ModelError` when a force-removed cross-model SAAS lingers in goal-state). This issue is the efficiency side: don't perform the read twice per hook. Cross-referencing both.
- Observed during review of canonical/postgresql-operator#1913 (comment thread on `_planned_units`/goal-state cost).
- Note for any fix: a charm-side cache alone does not help (the charm reads once per hook), and caching across hooks would be incorrect — hence the ask landing here.

Contributor guide

No contributing guide indexed for this repository

Research direction

Trace managers/config.py update_config through render_patroni_yml_file, core/state.py synchronous_node_count, and core/peer_relation.py planned_units, then compare it with the charm's own planned_units read in charm.py. Reproduce the hook-tool traffic with debug logging and verify that the fix performs one goal-state read per hook invocation, while preserving fresh values across hook invocations and scale events.

Written by the indexing model from the issue text.

Assessment

Tech stack
postgresql, python
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.