feat: define Docker network capacity policy - #82
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a7898c292
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "network": { | ||
| key: snapshot.get("docker_network_headroom", {}).get(key, 0) | ||
| for key in ("configured", "used", "free", "legacy") | ||
| }, |
There was a problem hiding this comment.
When status reporting is enabled, every report now contains docker.network, but StatusReceiver._validate_minimum still requires the Docker object to contain exactly healthy and oom (scripts/status_receiver.py:242). Consequently the receiver rejects every report from this engine as invalid_report, leaving external controller status stale; update the receiver validation and its compatibility tests alongside the sender.
Useful? React with 👍 / 👎.
| for subnet in subnets: | ||
| if any(subnet.subnet_of(pool) for pool in pools): | ||
| used_subnets.add(str(subnet)) |
There was a problem hiding this comment.
When an inspected subnet is inside a configured base but has a broader prefix than the policy's allocation size, this adds only one set entry. For example, a /24 network inside a /24 pool configured with size: 28 occupies all 16 allocatable /28 slots, yet the report says used=1, free=15, and healthy. Count the allocation-size subnets covered by each network, or classify mismatched prefixes as nonconforming, so exhaustion is not hidden.
Useful? React with 👍 / 👎.
| if network.version != 4: | ||
| return empty |
There was a problem hiding this comment.
When a Docker network is dual-stack, its valid IPv6 IPAM entry reaches this condition and causes the entire IPv4 headroom inspection to return unavailable, which evaluate reports as critical. Since the configured policy explicitly measures IPv4 pools, IPv6 entries should be skipped while retaining and evaluating the network's IPv4 subnets.
Useful? React with 👍 / 👎.
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["pool", "location", "state", "scale_set_name", "lifecycle", "engine_ref", "min_runners", "max_runners", "runner_resources"], | ||
| "required": ["pool", "location", "state", "scale_set_name", "lifecycle", "engine_ref", "min_runners", "max_runners", "runner_resources", "docker_network_policy"], |
There was a problem hiding this comment.
Making docker_network_policy immediately required leaves no configuration that both the previous and new installed managers accept: the previous exact-key validator rejects the new field as unknown, while this validator rejects a configuration without it. The installed manager's remote-reconcile.sh validates desired state with its current engine before fetching the newly pinned engine, so existing managed controllers cannot converge to this revision through the authoritative desired-state lifecycle; stage the field as optional/capability-gated before making it mandatory.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbe3be5c49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match = next(((pool, size) for pool, size in pools if subnet.subnet_of(pool)), None) | ||
| if match is None: | ||
| network_legacy = True | ||
| continue |
There was a problem hiding this comment.
When an existing Docker subnet contains a configured pool, subnet.subnet_of(pool) is false, so the subnet is only marked legacy and none of the overlapping allocation slots are counted. For example, a 198.51.100.0/23 network covering a configured 198.51.100.0/24 pool with /28 allocations is reported as used=0, free=16, warning, although every configured slot is occupied and exhaustion should be critical. Fresh evidence in the reviewed final tree is that the new containment check still excludes this inverse-overlap case; count the configured slots in every intersection.
Useful? React with 👍 / 👎.
| if subnet.prefixlen <= size: | ||
| used_subnets.update(str(slot) for slot in subnet.subnets(new_prefix=size)) |
There was a problem hiding this comment.
For accepted policies with a large prefix gap, this expands every covered allocation into a string and retains it in a set. A valid /8 pool with size: 32 and an inspected /8 network attempts to create roughly 16.8 million strings during each health check, consuming gigabytes of memory and potentially killing or indefinitely delaying monitoring. Compute occupied ranges or counts without enumerating every possible subnet.
Useful? React with 👍 / 👎.
| inspected = run(["docker", "network", "inspect", name]) | ||
| if inspected.returncode != 0: | ||
| return empty |
There was a problem hiding this comment.
When a workflow finishes and removes its per-job network after docker network ls but before this separate inspect call, the normal concurrent deletion produces a nonzero result and the entire headroom check becomes unavailable, which evaluate reports as critical. On an active fleet this race can therefore generate false unhealthy reports during ordinary job teardown; handle a confirmed not-found by refreshing or skipping that vanished network while still failing closed for genuine inspection errors.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ae88b83b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "network": { | ||
| key: snapshot.get("docker_network_headroom", {}).get(key, 0) | ||
| for key in ("configured", "used", "free", "legacy") | ||
| }, |
There was a problem hiding this comment.
When status reporting is enabled but the independently deployed receiver has not yet been upgraded, every controller report now unconditionally includes docker.network, even when docker_network_policy is absent. The previous v1 receiver accepts exactly healthy and oom, so upgrading or rolling back these separately managed components in the wrong order makes all reports fail as invalid_report and leaves external monitoring stale. Fresh evidence in the final tree is that the sender emits the field without a configured policy while the receiver retains a separate upgrade/rollback lifecycle; omit the field until the policy is configured or document and test a receiver-first compatibility sequence.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
| "docker_network_policy": { | ||
| "reserve_subnets": 1, | ||
| "default_address_pools": [ | ||
| {"base": "198.51.100.0/24", "size": 24 + args.max_runners.bit_length()}, |
There was a problem hiding this comment.
For accepted values starting at --max-runners 32, this formula emits /30 subnets (/31 at 64 and /32 at 128). A /30 bridge has only two usable IPv4 addresses, one consumed by Docker's gateway, so a contract-compliant Compose job with a test container and a database cannot attach both services; the smaller prefixes are worse. Expand the base or reject maxima that cannot retain a practical subnet size rather than satisfying runner capacity by shrinking each job network.
AGENTS.md reference: AGENTS.md:L13-L21
Useful? React with 👍 / 👎.
| if configured < max_runners + reserve: | ||
| raise DesiredStateError(f"{path}: policy cannot satisfy max_runners plus reserve") |
There was a problem hiding this comment.
When a policy provides exactly max_runners + reserve_subnets slots, this check approves it without accounting for the persistent ci-fleet_default network created by deploy/compose.yaml. Once that network is allocated from the configured pools, peak usage consumes max_runners + 1 slots, so only reserve_subnets - 1 remain; with the common reserve of one, full runner capacity is immediately reported as exhausted rather than retaining the reviewed reserve. Include the controller network in the required capacity calculation.
AGENTS.md reference: AGENTS.md:L44-L45
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5247252b50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| network = docker.get("network") | ||
| if network is not None: |
There was a problem hiding this comment.
When an authenticated report includes the optional key as "network": null, the Docker key-set check accepts it and this guard skips all network validation, so the receiver stores a report that violates the published v1 schema's object requirement. Consumers that treat the presence of network as a validated aggregate object can then fail or misclassify the controller; distinguish an absent key from a present null value and reject the latter as invalid_report.
Useful? React with 👍 / 👎.
| validation.require(type(memory) is int and memory >= 512, f"{path}.runner_resources.memory_mib", "must be at least 512 MiB") | ||
| network_policy = controller.get("docker_network_policy") | ||
| capacity_maximum = maximum if state != "disabled" and type(maximum) is int and maximum > 0 else 0 | ||
| if network_policy is not None: |
There was a problem hiding this comment.
When a controller contains "docker_network_policy": null, exact_keys permits the optional key and this condition skips semantic validation, so both normal and strict validation accept a value that the updated JSON schema requires to be an object. The installer does not execute JSON Schema validation separately, meaning this malformed desired state is deployable and silently disables the promised network-capacity inspection; validate whenever the key is present rather than only when its value is non-null.
AGENTS.md reference: templates/config-repository/AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80da5050b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if name == "bridge": | ||
| continue |
There was a problem hiding this comment.
When the daemon's built-in bridge is deliberately assigned a subnet inside a configured address pool (for example via bip), this unconditional skip omits the allocation even though Docker cannot reuse an overlapping pool slot. The report can therefore overstate free capacity and remain healthy at exhaustion; inspect bridge to count any overlap while exempting it only from the legacy-network warning.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
| if type(size) is not int or size < 0 or size > 32: | ||
| validation.errors.append(f"{pool_path}.size: must be an IPv4 prefix length between 0 and 32") |
There was a problem hiding this comment.
For a hand-edited private policy, strict validation still accepts size values up to /32, despite the initializer enforcing /29 or larger for usable Compose networks. A /31 or /32 allocation cannot provide the gateway and container addresses required by ordinary jobs, so a policy can pass schema, semantic validation, and rendering while Docker network creation fails; enforce the practical prefix ceiling in the schema and both validators.
AGENTS.md reference: templates/config-repository/AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| configured = sum(1 << (item["size"] - item["network"].prefixlen) for item in parsed) | ||
| if configured < max_runners + reserve + 1: | ||
| validation.errors.append( | ||
| f"{path}: network capacity cannot satisfy max_runners + reserve_subnets + one controller Compose network" |
There was a problem hiding this comment.
For a compliant project whose Compose file defines two internal networks, this minimum assumes only one subnet per concurrent runner. For example, a two-runner controller with one reserved subnet passes with four slots, but the controller network plus both jobs' two networks require five, so the final network creation fails at otherwise valid maximum capacity. Either model a reviewed networks-per-runner bound or enforce a single-network project contract before claiming this capacity is sufficient.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9679136e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce621fb68f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "docker_network_policy" in new: | ||
| validation.require( | ||
| current_evidence.get("engine_ref") == new.get("engine_ref") | ||
| and current_evidence.get("docker_network_policy_config") is True, | ||
| f"$.controllers.{name}.docker_network_policy", |
There was a problem hiding this comment.
When an installed controller retains this policy while selecting an incompatible engine, the evidence check runs only inside validate_transition, but the production validation calls in scripts/install-worker-controller.sh:191 and scripts/remote-reconcile.sh never provide --previous-config, so this block is skipped. The desired state therefore passes strict validation without matching docker_network_policy_config evidence and is rejected only later when rendering against the selected engine manifest, blocking reconciliation. Fresh evidence in the final tree is that the new retained-policy check remains reachable only through this optional transition path; invoke transition validation from the upgrade/reconcile paths or enforce current evidence during ordinary config validation.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
* feat: define Docker network capacity policy * fix: address Docker network policy review findings * fix: harden Docker network inspection * test: use reserved network capacity range * fix: harden Docker network policy validation * fix: harden network policy rollout validation * fix: enforce Docker network capacity policy * fix: harden network policy rollout reporting --------- Co-authored-by: Nick's Hermes <1572453+Nickfost@users.noreply.github.com>
…/controller/routine-go-9b6dc1ae62 * origin/main: feat: define Docker network capacity policy (#82)
…d config templates/config-repository/scripts/init.py hardcoded an RFC 5737 documentation address (198.51.100.0/24) as the default Docker default_address_pools base. PR #82's schema validation rejects RFC 5737 ranges under strict mode, so any config init.py generates now fails 'validate.py --strict'. The bootstrap test builds its fixture via init.py and the bootstrap strict-validates that pinned config, so the mocked bootstrap test could not bring up its routing callback. Emit 10.64.0.0/24, the operational CIDR the schema's own test (test_policy.py) uses as the valid example. The hand-written template fleet.json examples remain RFC 5737 placeholders because CI validates them non-strict (intended 'replace before use' examples).
1 participant
Summary
This is phase 1 of #81. It detects and reports risk only.
Validation
python3 scripts/test_desired_state.py— 17 passedpython3 scripts/test_health.py— 25 passedpython3 templates/config-repository/scripts/test_policy.py— 64 passedpython3 scripts/test_status_receiver.py— 28 passedscripts/validate.sh— all non-Docker gates passed, including installer and status receiver installation tests; stopped at the final Docker command because the local environment has no Docker CLIExcluded later #81 slices
Merging this PR does not authorize deployment, host access, Docker cleanup or restart, desired-state rollout, VM changes, or any other infrastructure action.