Make host ports node-exclusive placement claims - #47
Merged
Conversation
Two services claiming the same host_port could be colocated, producing DNAT rules with identical match criteria and different guest destinations. The first rule wins, so traffic reaches the wrong microVM while both microVMs report healthy. - model a claim as (tcp, host_port), matching the iptables rules, with the protocol carried explicitly so UDP forwards can widen the key - track claims per node in ScheduleWithStorage: a service is placed only where every claim is free, checked before storage fitting so nothing is committed on a rejected node; otherwise it stays pending with host_port_conflict, or duplicate_host_port_claims when it repeats its own port - re-evaluate existing placement so a newly conflicting assignment relocates instead of being preserved - reject a conflicting rendered node config in the agent before any networking or service state changes - reject a service repeating a host port in the enricher, and warn on repeats across services of one node type, which only placement can resolve Closes #40. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two 8-vCPU nodes let plain bin-packing spread the services on capacity alone, so the test passed with the host-port check disabled. Sizing one node to hold both services makes the claim the only reason they end up apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # internal/agent/agent_test.go
artemnikitin
marked this pull request as ready for review
August 25, 2026 11:51
artemnikitin
added a commit
that referenced
this pull request
Aug 25, 2026
Resolves the conflict with PR #47 (node-exclusive host-port claims), which this branch's description flagged as the coordination point: both changes add a per-candidate rejection tracker to the same loop in ScheduleWithStorage. Textual resolution: keep both trackers. The port check stays ahead of fitStorage, as #47 placed it, so a port-rejected node still commits no storage usage, and the pending reason keeps #47's precedence — a host-port conflict outranks a storage reason, because the port check runs first and a node rejected on ports is never evaluated for storage, so a storage reason recorded elsewhere describes a different node than the one the operator has to fix. That precedence is now stated where the constant is defined rather than left implicit in statement order, and the literal is named ReasonHostPortConflict alongside the other reason codes. Semantic resolution: a held service — one re-rendered outside the scheduler because its own volume record could not be read — still holds its host ports, and the scheduler could not see them. Neither branch had this hole; only the combination does. The scheduler now takes the pinned claims explicitly, and the controller supplies them from the held set, the same way reserveHeldCapacity already reserves their compute. Without it the scheduler would hand a held service's port to a newcomer on the same node, the agent would reject the whole rendered node config, and an unreadable record for one service would take down every service on that node — strictly worse than the hold it came from. The two docs that both branches touch are reconciled rather than concatenated: the pending-reason example list now carries the storage vocabulary as well as host_port_conflict, and the two reason vocabularies cross-link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
Closes #40.
Summary
A
host_portis a node-scoped exclusive resource, but nothing enforced that.The scheduler could colocate two services claiming the same port, and the agent
then installed DNAT rules with identical match criteria (
-i <iface> -d <host_ip>/32 -p tcp --dport <port>) and different--to-destinationguests.The first rule wins, so traffic reaches the wrong microVM while both microVMs
report healthy — the AWS demo failure of 2026-07-25, where tenant-2 Kibana 9.3.0
was silently routed to tenant-1 Elasticsearch 8.19.11 and looped at 503.
This makes the claim explicit at three boundaries: placement, agent admission,
and configuration validation.
The claim model
internal/config/portclaim.gointroducesPortClaim{Protocol, HostPort}.Uniqueness key:
(tcp, host_port). That matches the iptables implementationexactly —
scopedPortForwardSpec,hairpinPortForwardSpec, andlegacyPortForwardSpecall hardcode-p tcp, and all three scope to the singlehost IP resolved from the default route, so there is no per-service bind address
to include today.
Protocolis carried in the struct rather than implied, soadding UDP forwards later widens the key without touching any caller.
host_port: 0/absent claims nothing — the agent installs no rule for it.Scheduler (
ScheduleWithStorage)Each node carries a
claim → holding servicemap. A service is placed only on anode where every claim is free; the check runs before
fitStorage, so aport-rejected node commits no storage usage and multi-claim services move as a
unit. Claims are recorded on commit.
preferredmerely biases the candidate sort, so a conflicted preferred node is skipped and
the service relocates.
pendingwith reasonhost_port_conflictand a message naming the contested port, the servicealready holding it, and the node.
the distinct reason
duplicate_host_port_claims.Pending reasons already flow through
PlacementRevisioninto the status API andfireworkctl, so no plumbing was needed for invariant 6.Legacy
Scheduleis unchanged (no non-test callers); its doc comment now says itdoes not enforce claims.
Agent admission (defense in depth)
config.ValidateNodePortClaimsrejects a rendered node config whose servicesconflict, before
assignNetworkingand before any reconcile — so a stale,hand-written, or older-controller config installs no partial DNAT changes. The
revision is not recorded as applied, the agent reports
host_port_conflictonNetworkReady, and the next poll retries.Configuration validation
configcheck/enricher reject a service that repeats a host port(
ValidateInputandValidateOutput): always invalid, on any node.anti-affine services legitimately reuse a port on different nodes — so they
stay valid and surface as a new
repeated_host_portwarning, scoped to onenode type (services of different node types never share a node). That warning
alone would have flagged the tenant-1/tenant-2 Elasticsearch pair in CI.
Invariant 5 — release without collateral teardown
Verified as already correct and now regression-tested rather than changed: the
update path tears down using
Action.PreviousService(
internal/reconciler/reconciler.go:151-155,:222-226), and every rule speccarries
--to-destination <guest>:<port>, so removing one service's rule cannotmatch a peer's rule on the same host port.
TestPortForwardSpecsAreScopedToGuestDestinationpins that property.
Operational consequence before merging
On a deployment that currently has two colliding services on one node, the first
reconcile after this ships relocates one of them — a VM restart for that
service. The choice is deterministic (services ordered by vCPU descending, then
name; the first to take the port keeps it). If no other node can host the moved
service, it becomes
pendingwithhost_port_conflictinstead of running in theambiguous state it has today. On the AWS demo that is tenant-1/tenant-2
Elasticsearch on
9200; giving tenant-2 a unique port (and matching its Kibanacross_node_links) remains the right configuration fix independent of thischange.
Design questions from the issue
(
PortClaim) rather than a scalar resource, so the same tracking generalizesto other node-exclusive claims without another scheduler concept.
(protocol, host_port), with protocol pinned totcpbythe current iptables rules. See above.
collision? — Re-evaluated; the loser relocates, or goes pending.
configcheckreject statically? — Only intra-service duplicates;cross-service repeats are a warning.
added: the scheduler is now the only producer and it cannot emit a conflicting
node config, and the agent validates on the consuming side. A third check would
duplicate the invariant without a distinct failure mode to catch.
Testing
gofmt,go vet,staticcheck, andgo test -race ./...all clean.New coverage: claim extraction and node validation (
internal/config); placementseparation, preserved repeats across nodes, pending on conflict, relocation on a
new conflict, multi-claim atomicity, and self-conflict rejection
(
internal/scheduler); agent rejection before reconcile plus the non-conflictingcontrol (
internal/agent); duplicate rejection and the repeated-port warning(
internal/enricher); destination-scoped rule specs (internal/network).