[Nexthop][fboss2-dev] Add fboss2-dev config/delete qos default-policy commands#1366
Open
vybhav-nexthop wants to merge 1 commit into
Open
[Nexthop][fboss2-dev] Add fboss2-dev config/delete qos default-policy commands#1366vybhav-nexthop wants to merge 1 commit into
vybhav-nexthop wants to merge 1 commit into
Conversation
config qos default-policy <name> sets sw.dataPlaneTrafficPolicy.defaultQosPolicy after validating the policy name exists in qosPolicies. Commits at HITLESS (applied via updatePort() in ApplyThriftConfig, no ChangeProhibited in SAI). delete qos default-policy clears that field, under the delete subtree consistent with delete interface / delete protocol. The delete commits at AGENT_COLDBOOT rather than HITLESS to avoid a hw agent qos-map handle desync on hitless unset (details in PR description). Unit tests: 14 passing across set and delete fixtures. Integration test: covers set and delete round-trip, restores prior state.
92bbf16 to
b5eefe4
Compare
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.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
Adds two CLI commands for the switch-wide default QoS policy (the policy applied to ports that don't have a per-port override):
fboss2-dev config qos default-policy <name>sw.dataPlaneTrafficPolicy.defaultQosPolicy. Validates the name exists inqosPolicies.fboss2-dev delete qos default-policydelete qos, consistent withdelete interface,delete protocol, etc.Usage:
This PR only adds CLI and tests. It does not fix the underlying hw-agent bug.
Known issue: hitless unset corrupts hw-agent state (pre-existing on main)
Found on the device while testing these commands.
Repro (deterministic)
Crash in the agent log:
The same crash can happen on any later change to that policy in the map (edit maps, remove from
qosPolicies) — not just re-setting it as default. The hitless delete creates the bug; later commits cause the crash.Why unsetting the default is special
A QoS policy can live in one of two places in agent state:
QosPolicyMap— named policies used by per-port overridesdefaultDataPlaneQosPolicy— the switch-wide defaultA policy is in exactly one place. When
defaultQosPolicyis set in config,ThriftConfigApplier::updateQosPolicies()skips that policy when building the map (it lives in the default slot instead):Clearing
defaultQosPolicydoes not remove the policy fromqosPoliciesin config — it only clears one string. On the next commit, the applier stops skipping it, so one config change produces two agent deltas:qosPolicies[{"name": "default"}][{"name": "default"}](unchanged)defaultQosPolicy"default"QosPolicyMap{}{"default": ...}← added"default"In short: picking a policy as default doesn't remove it from
qosPolicies— CLI requires it stay there. Deleting the default just clears thedefaultQosPolicyfield. With that field now empty, the applier stops skipping the policy, so it gets added back intoQosPolicyMapon the same commit that removes it as default.What goes wrong in the hw agent
The hw agent tracks programmed QoS maps in
SaiQosMapManager::handles_, whose key is the policy name only (one slot per name). It applies the two deltas in order:Step (2) should tear down the old default-slot programming. But
removeQosMapignores theisDefaultflag and always erases by name:The commit succeeds, but hw and sw agent don't have the same state: SwitchState says the policy is in the map, while
handles_has no entry for it.isDefaultis stored on each handle and used for default-policy lookup, but remove never checks it — so the flag doesn't protect this handoff. Unit tests that unset the default only clear the default slot from the defaultQosPolicy (one delta); they don't also add the policy toQosPolicyMap, so they never exercise this path.Why delete uses AGENT_COLDBOOT
handles_table — that's the failure mode.handles_from scratch. Config and SwitchState are already correct; only the in-memory table is wrong.Set stays HITLESS — setting the default is safe on its own. It only crashes after a prior hitless unset left the agent in a bad state, which the delete command avoids by forcing COLDBOOT.
Test Plan
Unit tests, rerun fresh (14/14):
Integration on NH-4010-F, rerun fresh (2/2):
Integration tests use COLDBOOT on delete, so they do not exercise the hitless-unset bug path. If tests pass but you see
Channel got EOForNRestartsincrementing onfboss_hw_agent@0/fboss_sw_agent, the harness retried until agents recovered after a crash.Review Findings
Pre-publication automated review flagged 4 potential missing null checks on
thrift field accessors (
agentConfig.sw(),switchConfig.qosPolicies());all verified false positives — both are unqualified thrift fields
(
qosPolicies = []default), sofield_refdereference is always valid.No findings above threshold.