perf(network): skip empty egress programming on clean slots - #110
Open
cleverhu wants to merge 1 commit into
Open
perf(network): skip empty egress programming on clean slots#110cleverhu wants to merge 1 commit into
cleverhu wants to merge 1 commit into
Conversation
Programming an empty policy into an already empty chain is a no-op, and the iptables-restore it spawns is the single most expensive step of an otherwise pooled sandbox start. Track whether the namespace's user egress chain holds rules so recycled slots still get cleared, and treat a failed apply as leaving rules behind.
Contributor
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
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.
What
Skip the egress-policy apply when the requested policy is empty and the namespace's user egress chain is already empty. Slots track whether their chain currently holds rules so recycled slots are still cleared.
Why
set_egress_policyunconditionally spawns a thread, enters the sandbox network namespace, and runsiptables-restore. When a sandbox is started with no egress policy — the common case — that entire sequence programs an empty policy into an already empty chain: a no-op that still pays for a process spawn and a netns switch.On a sandbox start that otherwise hits every warm pool (network slot, block device, Firecracker), this is the single most expensive remaining step. Removing it takes the whole call to a couple of atomic loads.
Related issue
None. The change is local to one function and comes with unit tests.
Scope and non-goals
Included: the fast path plus the state tracking that makes it safe.
Excluded:
Design and behavior changes
Slotgainsuser_egress_rules_present: AtomicBool. A freshly created namespace starts with an empty chain, so it initializes tofalse.set_egress_policyreturns early only when both the requested policy has no runtime egress rules and the flag isfalse. Otherwise it takes the existing path and then storesresult.is_err() || wants_rules.The
is_err()term is deliberate: a failed apply may have flushed the chain and stopped partway, so the slot is assumed to hold rules. That is the conservative direction — the next tenant does a redundant clear rather than inheriting the previous tenant's rules.This matters because slots are recycled through the warm pool without their rules being cleared at release time. A recycled slot that held rules still gets cleared by the next tenant's empty policy.
Compatibility and operations
falseon restart, which matches a fresh namespace.Validation
make fmtmake clippymake test-unit(targeted; see below)make -C services test(required whenservices/changes)maketargetCommands and results:
Two tests are added:
empty_egress_policy_on_a_clean_slot_is_a_no_op— no namespace is ever created for the slot, so any path other than the fast one would fail entering it. Passing proves the fast path was taken.empty_egress_policy_still_clears_rules_a_previous_tenant_left— with the flag forced totrue, the call must be attempted (and therefore fail, since there is no namespace) rather than skipped.Skipped checks and reasons:
make test-unitnot run; only the two relevant tests plus a full--all-targetsbuild. The remaining phases need capabilities and a provisioned host.scripts/bench.py createon a warmed pool is the way to quantify it.Risks and reviewer notes
The correctness question is the state machine on
user_egress_rules_present, so please review that closely:falseis correct only if a new namespace really starts with an empty user egress chain.result.is_err() || wants_ruleserrs toward assuming rules exist. The cost of being wrong in that direction is a redundant clear; the cost in the other direction would be a leaked policy across tenants, which is a security issue.Slot, not being reset on release.The security-relevant scenario is a slot that held a restrictive policy being reused by a tenant with no policy. That is exactly the second test.
Most important file:
src/sandbox/network/slot.rs.Checklist