Sync shared cache-config port across content-cache units - #149
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ead on followers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix pruning when the last relation is removed: reconcile early-returns before _ensure_ports when no valid config remains, so free the port explicitly in the relation-broken handler (leader-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Also extract _resolve_ported_config to keep _load_nginx_config within the complexity limit, and assert peer relation presence in tests for mypy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…che units Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
relation_broken handling can still treat the departing relation as “valid” during that hook execution, causing port re-allocation and cache-backend re-publication for a relation that is being removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR moves per-cache-config nginx port allocation from per-unit local state to a leader-coordinated, peer-relation-backed shared map so that all content-cache units expose the same port (with different IPs) for a given backend relation.
Changes:
- Introduces a
content-cache-peerspeer relation and stores a sharedport_map/next_offsetin the peer app databag; leader allocates/prunes ports, followers consume them. - Updates charm reconcile flow to derive nginx config ports from the shared map (including port release on relation-broken).
- Extends unit + integration tests and updates charm design documentation to reflect cross-unit port synchronization.
File summaries
| File | Description |
|---|---|
| docs/explanation/charm-design.md | Documents the new leader-coordinated, peer-backed port allocation and uniform per-relation ports across units. |
| content-cache/src/charm.py | Implements peer-relation-backed shared port allocation, follower behavior, and port pruning on relation removal. |
| content-cache/metadata.yaml | Adds the content-cache-peers peer relation definition required by the charm logic. |
| content-cache/tests/unit/conftest.py | Adjusts harness leadership defaults and adds a follower harness fixture. |
| content-cache/tests/unit/test_charm.py | Updates and adds tests for shared port allocation, follower waiting behavior, and port-map pruning. |
| content-cache/tests/integration/test_basic.py | Scales to 2 units and asserts same port/distinct IPs for a relation, validating cross-unit synchronization. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eer event before nginx install Address Copilot review: _resolve_ported_config now filters broken_relation_id out of nginx_config itself (not just the pruning existing_ids set), so a relation-broken hook can never re-allocate a port or republish backends for the relation that is departing, even if remote data for it is still visible. Also guard _on_peer_relation_changed to skip reconciling when nginx has not been installed yet: on unit add, the peers relation-created event can fire before the start hook runs, and calling _load_nginx_config() that early tried to restart a not-yet-installed nginx service (observed as an integration test failure: cache/1 unit in error state, 'Failed to restart nginx.service: Unit nginx.service not found'). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ration tests Application.related_applications() unconditionally unpacks Relation.endpoints as a 2-tuple, but a peer relation only ever has a single endpoint entry. Now that content-cache declares the content-cache-peers peer relation, any call to app.related_applications() on the content-cache app raises 'ValueError: not enough values to unpack (expected 2, got 1)', which broke test_tls_cert.py's certificate_transfer and certificates cleanup checks in CI. Add has_related_application(), a peer-relation-safe replacement, and use it in place of the two affected app.related_applications() calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reword to 'data bag' and 'IP addresses' to satisfy the rtd-docs-checks spell check, which flagged 'databag' and 'IPs' as misspelled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot please re-review |
Re-checked the fix from commit 0ada417: |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core runtime port-allocation behavior and adds peer-coordination logic affecting multi-unit operation, which warrants final human review alongside the suggested fixes.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
content-cache/src/charm.py:475
- _ensure_ports iterates
valid_relation_idsas aset, which makes port assignment order non-deterministic when multiple new relations are allocated in one reconcile. This can lead to confusing/unstable port-to-relation mappings (e.g., after leader restart or when several relations become valid at once). Iterating in a deterministic order (e.g., sorted relation ids) makes allocation reproducible without changing correctness.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
… endpoint lookup
Address second-pass Copilot review and weiiwang01's review comments:
- _ensure_ports now allocates ports for newly-valid relations in sorted
(ascending) relation-id order instead of iterating a set, so allocation is
reproducible when several relations become valid in the same reconcile
(e.g. after a leader restart).
- json.dumps(..., sort_keys=True) for the peer port_map field, for stable
encoding.
- Rename PORT_MAP_FIELD/NEXT_OFFSET_FIELD to kebab-case ('port-map',
'next-offset') for consistency with other relation data field naming.
- has_related_application() no longer raises StopIteration when a relation
has no endpoint matching the local app name; it now safely returns False
for that relation instead of blowing up test cleanup (finally blocks) and
leaking relations.
- Add test_port_allocation_order_is_deterministic to cover the ordering fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The charm no longer clears previously opened ports when config/peer prerequisites are missing, which can leave stale ports exposed after relations are removed or before ports are assigned.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
content-cache/src/charm.py:335
- When there is no valid nginx config (e.g., after the last cache-config relation is removed),
_load_nginx_configreturns without callingunit.set_ports(). Since this is now the only call site, previously opened ports may remain exposed indefinitely even though the charm is Blocked/Waiting.
This issue also appears in the following locations of the same file:
- line 337
- line 352
docs/explanation/charm-design.md:76
- The sentence "To keep the backend list consumers see uniform" is grammatically awkward; it reads like a missing word and can be misinterpreted.
content-cache/src/charm.py:339
- If the peer relation is not established yet,
_resolve_ported_configreturns None and_load_nginx_configexits without clearing previously opened ports. That can leave stale ports open while the unit is in WaitingStatus for port assignment.
resolved = self._resolve_ported_config(nginx_config, broken_relation_id)
if resolved is None:
return
content-cache/src/charm.py:355
- When
ported_configis empty (e.g., follower hasn't observed any leader-assigned ports yet), the charm clears relation data and returns but does not clear any previously opened ports. Clearing ports here avoids exposing stale listeners while waiting for assignment.
if not ported_config:
self.unit.status = ops.WaitingStatus(WAIT_FOR_PORT_MESSAGE)
self._clear_cache_backend()
return
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Picks up canonical/content-cache-operator#149, which makes all content-cache units serve a given cache-config relation on the same nginx port (coordinated via a leader-allocated peer relation), instead of each unit independently allocating its own port. This is a step toward resolving the review discussion on src/charm.py:394 about backend-addresses/backend-ports combinatorics when units use different ports.
Summary
content-cache-peerspeer relation; the leader unit allocates one nginx port percache-configrelation and stores it in the peer app databag (port_map/next_offset), replacing per-unitops.StoredStateallocation.cache-configrelation for that ID is removed (_release_port), fixing a bug where the port map was never cleaned up in that case.content-cacheto 2 units and assert they share the same port with distinct IPs for the same backend.docs/explanation/charm-design.mdto describe the new leader-coordinated port allocation.Test Plan
tox -e unit(76 passed)tox -e statictox -e linttox -e integration(relies on CI / LXD+Juju; not run locally in this session)