cos: enforce the inet-precedence classifier end to end (#6847) - #6877
Open
psaab wants to merge 6 commits into
Open
cos: enforce the inet-precedence classifier end to end (#6847)#6877psaab wants to merge 6 commits into
psaab wants to merge 6 commits into
Conversation
DO NOT MERGE THIS COMMIT ALONE. It adds the unit-level binding site
without the enforcement behind it, so an operator can now bind an
inet-precedence classifier and it will silently do nothing. That is
WORSE than master, where the bind line is rejected outright. The Rust
classify arm and the wire fields must land before this is mergeable.
Scope finding that changed the plan. The issue described inet-precedence
as accepted-but-inert, implying only a dataplane arm was missing. In
fact the unit-level `classifiers` schema node had no `inet-precedence`
child at all: the classifier was definable at the top level and NOT
bindable, so `set class-of-service interfaces <if> unit <n> classifiers
inet-precedence <name>` was rejected by the schema. An imported vSRX
config fails at the bind line, not silently at runtime.
Landed in this commit (Go config layer):
- CoSINetPrecedenceClassifier / ...Entry types and the keyed
INetPrecedenceClassifierDefs map. #4316 recorded only classifier
NAMES because nothing consumed them; the entries are now compiled.
- The unit-level schema binding site (new) and the matching
CoSInterfaceUnit.INetPrecedenceClassifier field.
- collectCoSINetPrecedenceCodePoints: the 0..7 domain with its own
message wording rather than borrowing the 802.1p collector's, since
it is a different field of the same width.
- The classifier compile, mirroring dscp including the
lenientCoSNumericCodePoint downgrade.
- interface -> unit inheritance, mirroring the dscp / 802.1p fallbacks.
- validateCoSUnitClassifierConflict: a unit binding BOTH dscp and
inet-precedence is rejected at commit. The two read the same IPv4 TOS
byte -- precedence is its top 3 bits -- so binding both is a
contradiction, not a composition, and the BA chain consults DSCP
first, which would leave the inet-precedence binding silently dead.
Paired with lenientCoSUnitClassifierConflict so the tolerant
Load/SyncApply path downgrades to a warning and an
already-persisted config still boots (#1960); DSCP wins on that boot.
The conflict rejection rests on our own semantics (one TOS byte, two
classifiers, no defined composition), NOT on a claim about Junos
behaviour, which I have no way to verify here. If someone can check a
real vSRX and Junos defines a precedence order, that should override
this choice.
Golden 4406 regenerated. Verified the diff is EXCLUSIVELY the new struct
field before accepting it: 18 keys added, 0 removed, 0 changed, every
added key `INetPrecedenceClassifierDefs`. A golden regen that masks a
real behavior change is its own defect, so this was checked rather than
assumed.
Remaining before this is mergeable:
1. Wire: `cos_inet_precedence_classifier` on the interface snapshot and
an `inet_precedence_classifiers` list (protocol/snapshot.rs,
protocol/cos.rs) plus the Go publish side.
2. Rust: an 8-entry precedence_queue_by_prec table beside
dscp_queue_by_dscp (forwarding_build/cos.rs) and a
resolve_cos_inet_precedence_classifier_queue_id arm reading
(dscp >> 3) & 0x7.
3. Retract the classifier half of the accepted-but-inert advisory
(compiler_validate_warn.go) once the arm lands. The rewrite-rules
inet-precedence advisory STAYS -- that direction remains inert.
4. A RUNTIME assertion per the #6850 doctrine: a packet whose
precedence bits map to a forwarding class must land in that class's
queue. A compile-and-store test would pass identically with no
dataplane consumer, which is the state this knob is in today.
5. Conflict-gate tests, including the lenient-path downgrade.
Advances #6847.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The `inet-precedence` behavior-aggregate classifier was accepted at commit and did nothing. #4316 recorded only the classifier NAMES, and the unit-level `classifiers` schema node had no `inet-precedence` child at all -- so the classifier was definable at the top level and NOT bindable, and `set class-of-service interfaces <if> unit <n> classifiers inet-precedence <name>` was rejected outright by the schema. An imported vSRX config failed at the bind line rather than silently misclassifying. The preceding WIP commit added the Go config layer, including the unit binding site. That left the tree in a state strictly WORSE than before: an operator could bind the classifier and it silently did nothing, which converts a loud failure into a quiet one. This commit lands the enforcement behind it, so the branch is coherent as a whole. Wire, additive on both sides. `inet_precedence_classifiers` on the CoS snapshot and `cos_inet_precedence_classifier` on the interface snapshot. An older helper ignores both keys; a newer helper reading an older snapshot gets the empty default, which is "no inet-precedence binding" -- the pre-change behaviour exactly. `omitempty` keeps the wire byte-identical for every config that does not use one. Dataplane. An 8-entry `inet_precedence_queue_by_prec` table is built beside `dscp_queue_by_dscp`, and `resolve_cos_inet_precedence_classifier_queue_id` reads `(dscp >> 3) & 0x7`. That shift is load-bearing and was verified rather than assumed: `frame/generated.rs:78` parses `packet[1] >> 2`, so `meta.dscp` is already the 6-bit DS field and IP precedence is its top 3 bits. The arm is family-agnostic exactly like the existing dscp arm -- this codebase has never split the DS-field classifier by address family, and splitting it for inet-precedence alone would be a new inconsistency. Ordering. The arm is spliced in after DSCP and before 802.1p at all five call sites, so both L3 arms precede the L2 one. DSCP-before-inet- precedence is not incidental: a unit binding both is rejected at commit, but the tolerant Load / SyncApply path downgrades that to a warning so an already-persisted or peer-synced config still boots (#1960), and this order is what makes the warning's claim -- "DSCP wins on that boot" -- true. Three gaps in the Go half that would have left the feature dead even with the classify arm present. Each was found by a test, not by reading: 1. `coSInterfaceUnitHasBinding` did not list `INetPrecedenceClassifier`, so a unit binding ONLY that classifier was parsed and then DISCARDED, taking the whole CoS interface with it. Nothing reached the snapshot. 2. The #1183 `useful_cos_state` gate had no inet-precedence arm, so an interface with no shaping-rate and no scheduler-map was skipped even once the binding survived -- i.e. exactly the plain `unit 0 classifiers inet-precedence <name>` config this work is about. 3. The `ba_reclassify` gate had no inet-precedence arm, so a flow's queue would freeze on its seed packet's marking. Classifying once and then ignoring every later marking change is the same wrong-queue outcome as not classifying, with a harder-to-see cause. The entry's `loss-priority` is wired into `resolve_cos_loss_priority` alongside the queue arm. Without it `loss-priority high` would compile, cross the wire, and then silently apply the LOW egress rewrite -- an accepted-but-inert sub-knob inside the fix for accepted-but-inert. The classifier half of the #4316 advisory is retracted; keeping an "inert" warning on a working classifier would be actively wrong. The `rewrite-rules inet-precedence` advisory STAYS -- that direction is still inert. The advisory test asserts the retracted string is ABSENT rather than merely dropping it from the expected list, so reinstating it fails. Validation. Fourteen guards were mutation-proved: each reverted individually, observed RED with an ASSERTION rather than a build break, then restored from a byte snapshot and the restore verified. Two harness errors of my own surfaced that way and were fixed -- a build-break marker that matched cargo's own `error: test failed` line and reported seven false negatives, and one test that failed via a nil-pointer panic instead of an assertion (now it steps through the lookup and names what broke). MEASURED negative result, recorded in the test comment rather than papered over: the out-of-range fail-closed check exists at two sites whose key sets are always identical, because `queue_by_prec` and `lp_by_prec` are filled from the same entries in the same loop. No snapshot can reach one without the other, so mutating either alone leaves the test GREEN -- the property binds only when both are mutated. The comment states that scope instead of claiming each site is independently load-bearing. Both are kept because each is the natural bounds check for its own table. Suites: full `go test ./...` exit 0; `go test ./pkg/refactoraudit/` exit 0. The Rust `--bin` suite is 4241 passed / 1 failed against 4233 passed / 1 failed at the branch base `ad9591177` measured in a detached worktree -- the same single test (`current_generation_install_and_delete_still_apply_on_poisoned_shared_mutex`, a known process-global HA counter flake whose fix is not yet merged), and the +8 delta is exactly the eight new Rust tests. `protocol_wire_v1.json` regenerated; the diff is exactly three additive keys, 0 removed, 0 changed. No Rust formatter was run; gofmt was applied only to the three Go files edited. Advances #6847. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
added 4 commits
August 6, 2026 00:28
# Conflicts: # _Log.md
Resolve the sole conflict, _Log.md, by union: every entry from both sides is retained and none is rewritten. The file is no longer append-ordered, so line counts and prefix checks say nothing useful about the result; the resolution was verified structurally instead, by confirming that each pre-merge side diffs into the merged file with add-hunks only and no changed-or-deleted hunk on either side. Every other path merged without conflict. Because a clean textual auto-merge can still break compilation when a signature moves on one side, that was confirmed by building rather than by inspection: go build ./... clean on the merged tree, and cargo check --all-targets clean for the Rust crate. Advances #6877.
# Conflicts: # _Log.md
# Conflicts: # _Log.md
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.
Summary
class-of-service classifiers inet-precedencewas accepted by the configcompiler and then did nothing: the classifier was parsed, validated and stored,
but no dataplane path ever consulted it, so every packet kept its default
forwarding class. This closes that gap end to end — config half plus the Rust
dataplane enforcement.
Why the scope is what it is
Three discarded-binding gaps had to be closed together or the feature stays
inert:
coSInterfaceUnitHasBinding, the #1183useful_cos_stategate, and theba_reclassifygate each independently dropped the classifier before it couldtake effect. A PR containing only the classifier plumbing would ship exactly
the accepted-but-does-nothing state this issue exists to close.
loss-priorityis included for the same reason one level down: without it,loss-priority highcompiles and silently applies the LOW rewrite — anaccepted-but-inert sub-knob inside the fix for accepted-but-inert.
Validation
go test ./...— exit 0.go test ./pkg/refactoraudit/— exit 0; no file crossed a heatmap tier.known process-global HA counter flake could not be mistaken for a regression:
base
ad95911774233 passed / 1 failed, this branch 4241 passed / 1failed — the same single test, and the +8 are exactly this branch's eight
new tests. The diff touches zero bytes under
afxdp/ha. That flake is fixedseparately by test(ha): scope sync-import refusal counters per Coordinator #6862.
restore.
protocol_wire_v1.jsonregenerated in its own commit: 3 additive keys, 0removed, 0 changed — checked on both the Rust and Go sides, since a
one-sided field is a no-transit risk.
Measured negative result, recorded so it is not mistaken for redundancy
The out-of-range check exists at two sites whose key sets are always identical,
so mutating either one alone leaves the suite green. Both are kept
deliberately; do not delete the second as dead.
Outstanding
Loss-cluster smoke is owed and is the maintainer's to run — this touches the
CoS classifier path.
Closes #6847