fix(utils): floor a sub-core CPU limit at one core - #206
Draft
marceloneppel wants to merge 1 commit into
Draft
Conversation
Juju's Kubernetes provider maps a `cpu-power=N` application constraint to an `Nm` CPU limit on every container, so any sub-core constraint truncated to zero available cores. The worker-process sizing derives every `cpu_max_*` parameter from that number: zero cores collapses the automatic values to "0" and puts every explicit value above its `10 * vCores` ceiling of zero, so the unit can no longer be configured. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 14, 2026
The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 14, 2026
The behaviour it pins left this stack along with the floor itself, so the assertion now covers code that only exists in its own fix on 16/edge (#206), where the test belongs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 17, 2026
The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 17, 2026
The behaviour it pins left this stack along with the floor itself, so the assertion now covers code that only exists in its own fix on 16/edge (#206), where the test belongs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 18, 2026
…art engine, and bridges (4/6) (#180) * feat(config): complete update_config with API apply, restart engine, and bridges The library now owns the full config-update flow (steps 3-10) instead of the charm. update_config renders, applies Patroni-controlled parameters via the API, runs the TLS/pending-restart decision engine, honors the VM snap gate, and persists the config/user hashes -- all in the lib. The substrate-tangled pieces of the restart trigger, endpoint refresh, and monitoring/LDAP service restarts ride charm-side through three injected bridge callables (request_restart, refresh_endpoints, restart_services) until their own migration phases, so the two HIGH-risk substrate diffs in the restart trigger (VM pops postgresql_restarted; K8s updates the metrics scrape job) stay out of the library. is_tls_enabled and generate_config_hash internalize; user_hash stays injected. The config hash is byte-compatible with the charm's, so charm adoption does not force a spurious restart. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(utils): drop the sub-core CPU floor from this migration The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(workload): narrow the snap-revision gate instead of stubbing the base BaseWorkload gained a non-abstract get_snap_revision that only the VM workload can answer, purely so the type checker would accept a call the substrate guard already keeps off K8s. Reach it the way the event handlers reach the other VM-only workload capability, by narrowing with isinstance at the call site, so the shared interface stops advertising a snap the K8s workload has no access to. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): log the config update with the wording both charms know The ported line carried the VM charm's wording, which dropped the K8s charm's "Updating Patroni config file" from the logs entirely. Operators grepping either substrate's history for that phrase would have found nothing after the move. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): cache the config hash for the manager's lifetime Both charms compute this hash once per hook via cached_property; the port turned it into a plain property, so update_config recomputes it on each of its two accesses. The value cannot differ within a hook, so this only restores the charms' cost profile. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): keep the VM workload out of the K8s import graph Narrowing the snap gate with isinstance needed VMWorkload at runtime, and that module imports the snap charm lib, which ships only in the vm extra. The K8s charm installs postgresql and k8s, imports the config manager, and would have died at startup on `cannot import name 'snap' from 'charmlibs'`. The library's own test env installs every extra, so nothing here would have caught it. Narrow with a type-checking-only cast instead; the substrate guard is what keeps the call off K8s at runtime. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): announce the config update at info level The K8s charm logged this at info, so operators saw a config render in debug-log at default verbosity; the port left it at the VM charm's debug level and that visibility disappeared on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * docs(charms): drop the test-only claim from the bridge comments These base classes are the per-substrate charm definitions, not test scaffolding, and describing them as unit-test only reopens a settled question about whether they belong in the shipped wheel. What the comment needs to say is which side owns the bridges, which the preceding lines already do. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): gate TLS on the certs reaching disk, not just the databag Issued certs land in the relation databag before the push writes them to the workload, and the push can defer — on K8s whenever the container is not ready. Any of the other update_config call sites firing in that window rendered ssl on against files that were not there, which is the non-atomicity the TLS port left open; TLSManager.client_tls_files_on_disk was added for this and had no caller. This diverges from the charms, which check the databag alone. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): keep the parameter dump out of the default-verbosity log update_config runs on nearly every hook, and the ported line had merged the K8s charm's info level with the VM charm's parameter payload, so the full PostgreSQL parameter dict landed in every operator's log at default verbosity. Each substrate's original line goes back as it was: the info announcement operators grep for, and the parameter dump at debug. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 18, 2026
…te_config migration (5/6) (#188) * feat(config): complete update_config with API apply, restart engine, and bridges The library now owns the full config-update flow (steps 3-10) instead of the charm. update_config renders, applies Patroni-controlled parameters via the API, runs the TLS/pending-restart decision engine, honors the VM snap gate, and persists the config/user hashes -- all in the lib. The substrate-tangled pieces of the restart trigger, endpoint refresh, and monitoring/LDAP service restarts ride charm-side through three injected bridge callables (request_restart, refresh_endpoints, restart_services) until their own migration phases, so the two HIGH-risk substrate diffs in the restart trigger (VM pops postgresql_restarted; K8s updates the metrics scrape job) stay out of the library. is_tls_enabled and generate_config_hash internalize; user_hash stays injected. The config hash is byte-compatible with the charm's, so charm adoption does not force a spurious restart. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(utils): drop the sub-core CPU floor from this migration The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(workload): narrow the snap-revision gate instead of stubbing the base BaseWorkload gained a non-abstract get_snap_revision that only the VM workload can answer, purely so the type checker would accept a call the substrate guard already keeps off K8s. Reach it the way the event handlers reach the other VM-only workload capability, by narrowing with isinstance at the call site, so the shared interface stops advertising a snap the K8s workload has no access to. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): log the config update with the wording both charms know The ported line carried the VM charm's wording, which dropped the K8s charm's "Updating Patroni config file" from the logs entirely. Operators grepping either substrate's history for that phrase would have found nothing after the move. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): cache the config hash for the manager's lifetime Both charms compute this hash once per hook via cached_property; the port turned it into a plain property, so update_config recomputes it on each of its two accesses. The value cannot differ within a hook, so this only restores the charms' cost profile. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): keep the VM workload out of the K8s import graph Narrowing the snap gate with isinstance needed VMWorkload at runtime, and that module imports the snap charm lib, which ships only in the vm extra. The K8s charm installs postgresql and k8s, imports the config manager, and would have died at startup on `cannot import name 'snap' from 'charmlibs'`. The library's own test env installs every extra, so nothing here would have caught it. Narrow with a type-checking-only cast instead; the substrate guard is what keeps the call off K8s at runtime. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): announce the config update at info level The K8s charm logged this at info, so operators saw a config render in debug-log at default verbosity; the port left it at the VM charm's debug level and that visibility disappeared on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * docs(charms): drop the test-only claim from the bridge comments These base classes are the per-substrate charm definitions, not test scaffolding, and describing them as unit-test only reopens a settled question about whether they belong in the shipped wheel. What the comment needs to say is which side owns the bridges, which the preceding lines already do. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): gate TLS on the certs reaching disk, not just the databag Issued certs land in the relation databag before the push writes them to the workload, and the push can defer — on K8s whenever the container is not ready. Any of the other update_config call sites firing in that window rendered ssl on against files that were not there, which is the non-atomicity the TLS port left open; TLSManager.client_tls_files_on_disk was added for this and had no caller. This diverges from the charms, which check the databag alone. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): unit tests and golden fixtures for the update_config migration Split the migration's unit tests and patroni-template golden fixtures out of the code changes so each code change lands as a small, self-contained PR, with the tests landing together on top of the stack. Covers the peer-state config/user hash accessors, PostgreSQL parameter building and resource introspection, the substrate-conditional patroni template, and the update_config orchestration. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): compare the golden renders as configuration, not bytes The merged template now emits one field order for both substrates instead of reproducing each charm's, so a byte comparison fails on layout that carries no meaning to Patroni. Comparing the parsed documents keeps the guarantee that matters - same values, same keys, same rule order - and stays strict where order is semantic, since sequences still compare in order and pg_hba is first-match-wins. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): pin the K8s data paths to the workload version The charm template parameterised bin_dir but hardcoded the major version in the WAL, PostgreSQL-log and Patroni-log paths, so a 17 workload would have kept three directories on 16. The golden matrix cannot catch it: it pins version at 16, where both spellings render identically. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(utils): drop the sub-core CPU floor assertion The behaviour it pins left this stack along with the floor itself, so the assertion now covers code that only exists in its own fix on 16/edge (#206), where the test belongs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): cover the on-disk gate on is_tls_enabled Pins the window the gate closes: certs present in the databag but not yet written to the workload must not read as TLS enabled. The existing all-files-present test now states the disk input too, since presence in the databag alone no longer decides. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
added a commit
that referenced
this pull request
Aug 18, 2026
…push (6/6) (#189) * feat(config): complete update_config with API apply, restart engine, and bridges The library now owns the full config-update flow (steps 3-10) instead of the charm. update_config renders, applies Patroni-controlled parameters via the API, runs the TLS/pending-restart decision engine, honors the VM snap gate, and persists the config/user hashes -- all in the lib. The substrate-tangled pieces of the restart trigger, endpoint refresh, and monitoring/LDAP service restarts ride charm-side through three injected bridge callables (request_restart, refresh_endpoints, restart_services) until their own migration phases, so the two HIGH-risk substrate diffs in the restart trigger (VM pops postgresql_restarted; K8s updates the metrics scrape job) stay out of the library. is_tls_enabled and generate_config_hash internalize; user_hash stays injected. The config hash is byte-compatible with the charm's, so charm adoption does not force a spurious restart. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(utils): drop the sub-core CPU floor from this migration The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(workload): narrow the snap-revision gate instead of stubbing the base BaseWorkload gained a non-abstract get_snap_revision that only the VM workload can answer, purely so the type checker would accept a call the substrate guard already keeps off K8s. Reach it the way the event handlers reach the other VM-only workload capability, by narrowing with isinstance at the call site, so the shared interface stops advertising a snap the K8s workload has no access to. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): log the config update with the wording both charms know The ported line carried the VM charm's wording, which dropped the K8s charm's "Updating Patroni config file" from the logs entirely. Operators grepping either substrate's history for that phrase would have found nothing after the move. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): cache the config hash for the manager's lifetime Both charms compute this hash once per hook via cached_property; the port turned it into a plain property, so update_config recomputes it on each of its two accesses. The value cannot differ within a hook, so this only restores the charms' cost profile. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): keep the VM workload out of the K8s import graph Narrowing the snap gate with isinstance needed VMWorkload at runtime, and that module imports the snap charm lib, which ships only in the vm extra. The K8s charm installs postgresql and k8s, imports the config manager, and would have died at startup on `cannot import name 'snap' from 'charmlibs'`. The library's own test env installs every extra, so nothing here would have caught it. Narrow with a type-checking-only cast instead; the substrate guard is what keeps the call off K8s at runtime. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * refactor(config): announce the config update at info level The K8s charm logged this at info, so operators saw a config render in debug-log at default verbosity; the port left it at the VM charm's debug level and that visibility disappeared on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * docs(charms): drop the test-only claim from the bridge comments These base classes are the per-substrate charm definitions, not test scaffolding, and describing them as unit-test only reopens a settled question about whether they belong in the shipped wheel. What the comment needs to say is which side owns the bridges, which the preceding lines already do. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(config): gate TLS on the certs reaching disk, not just the databag Issued certs land in the relation databag before the push writes them to the workload, and the push can defer — on K8s whenever the container is not ready. Any of the other update_config call sites firing in that window rendered ssl on against files that were not there, which is the non-atomicity the TLS port left open; TLSManager.client_tls_files_on_disk was added for this and had no caller. This diverges from the charms, which check the databag alone. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): unit tests and golden fixtures for the update_config migration Split the migration's unit tests and patroni-template golden fixtures out of the code changes so each code change lands as a small, self-contained PR, with the tests landing together on top of the stack. Covers the peer-state config/user hash accessors, PostgreSQL parameter building and resource introspection, the substrate-conditional patroni template, and the update_config orchestration. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): compare the golden renders as configuration, not bytes The merged template now emits one field order for both substrates instead of reproducing each charm's, so a byte comparison fails on layout that carries no meaning to Patroni. Comparing the parsed documents keeps the guarantee that matters - same values, same keys, same rule order - and stays strict where order is semantic, since sequences still compare in order and pg_hba is first-match-wins. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): pin the K8s data paths to the workload version The charm template parameterised bin_dir but hardcoded the major version in the WAL, PostgreSQL-log and Patroni-log paths, so a 17 workload would have kept three directories on 16. The golden matrix cannot catch it: it pins version at 16, where both spellings render identically. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(utils): drop the sub-core CPU floor assertion The behaviour it pins left this stack along with the floor itself, so the assertion now covers code that only exists in its own fix on 16/edge (#206), where the test belongs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(config): cover the on-disk gate on is_tls_enabled Pins the window the gate closes: certs present in the databag but not yet written to the workload must not read as TLS enabled. The existing all-files-present test now states the disk input too, since presence in the databag alone no longer decides. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * feat(tls): emit tls_files_pushed after a successful cert push The TLS handler pushes cert files to the workload on certificate_available, and the charm reloads PostgreSQL in a separate observer of the same event. Because defer is per-observer, a push that defers (e.g. a Pebble write fails mid cert renewal) while the charm's reload observer completes leaves the retried push writing the new cert with nothing reloading. Emit a tls_files_pushed event only after push_tls_files() succeeds; a deferred push returns without emitting. The charm's reload bridge observes this instead of certificate_available, so the reload runs only once the files are on disk and a deferred push retries together with its reload. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * chore(release): bump version to 16.3.5 The update_config + tls_files_pushed stack branched at 16.3.3; 16/edge has since released 16.3.4. Set the version to the next patch so the library is release-ready as soon as this stack lands on 16/edge. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Juju's Kubernetes provider maps a
cpu-power=Napplication constraint straight to anNmmillicore CPU value (constraints.go#L38) and writes it into every container'sResources.Limits(#L339).any_cpu_to_corestruncates millicores with integer division, so any sub-core constraint —cpu-power=500→"500m"— reports zero available cores toK8sManager.get_available_resources.Zero cores then drives the whole worker-process sizing in
ConfigManager: theautovalues collapse to"0"(min(8, 2 * 0)), and every explicitly configuredcpu-max-*value is rejected against a10 * vCoresceiling of zero withvalue N exceeds maximum allowed of 0. The unit ends up impossible to configure.Solution
Floor the millicore conversion at a single core. A container allowed a fraction of a CPU still runs one PostgreSQL, so one core's worth of configuration is the smallest meaningful answer; whole-core values (
"12","1000m","1500m") are unaffected.Split out of #180, where it rode along with the
update_configmigration and left that PR reading as a behaviour change rather than a parity port.Checklist