[countersyncd]: Harden and optimize HFT IPFIX processing - #4859
Closed
Pterosaur wants to merge 2 commits into
Closed
[countersyncd]: Harden and optimize HFT IPFIX processing#4859Pterosaur wants to merge 2 commits into
Pterosaur wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
Author
|
Superseded by #4860, which contains the same final diff as one DCO-signed commit. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a large behavioral refactor across decoding, actor messaging, backpressure, and lifecycle management where correctness depends on subtle ordering/capacity invariants that warrant final human review.
Pull request overview
This PR hardens and substantially optimizes countersyncd’s SONiC HFT IPFIX processing path by moving to fixed-width decoding, tightening template lifecycle rules, and batching/deduplicating message flow between actors to reduce allocations and improve backpressure behavior.
Changes:
- Introduces/propagates flat
SAIStatsBatchmessages (record-preserving, multi-record per channel item) and switches object names toArc<str>to reduce per-sample allocation/cloning. - Adds stronger input validation and bounded buffering/capacity controls (including CLI parsing bounds) and improves comm-stats hot-path behavior by removing a shared mutex.
- Updates integration tests and benchmarks to use readiness barriers/probes and the new batching semantics.
File summaries
| File | Description |
|---|---|
| crates/countersyncd/tests/ipfix_test_helpers.rs | Adds object-metadata generation and strengthens IPFIX length handling to prevent stalling in record generation. |
| crates/countersyncd/tests/ipfix_helpers_integration.rs | Refactors integration test to consume SAIStatsBatch records, adds readiness and delete-barrier probes, and updates schema-change expectations. |
| crates/countersyncd/tests/integration_test.rs | Updates end-to-end tests for batched SAI stats delivery and revised IPFIX record layout expectations. |
| crates/countersyncd/src/utilities/mod.rs | Reworks comm-stats tracking from mutex+map to fixed atomic slots and optimizes hex formatting. |
| crates/countersyncd/src/message/saistats.rs | Replaces per-record message type patterns with SAIStatsBatch, introduces SAIStatsRef, and centralizes enterprise-number ID decoding. |
| crates/countersyncd/src/message/otel.rs | Updates OTel conversion to consume borrowed SAIStatsRef/Arc<str> object names rather than owned strings. |
| crates/countersyncd/src/main.rs | Adds IPFIX join classification, bounds channel capacities via clap value parsers, and updates default capacities for batch channels. |
| crates/countersyncd/src/actor/swss.rs | Tightens HFT metadata validation and ensures disabled/non-IPFIX sessions actively deactivate prior IPFIX state via deletes. |
| crates/countersyncd/src/actor/stats_reporter.rs | Switches stats input to SAIStatsBatchMessage and updates aggregation paths to iterate per record. |
| crates/countersyncd/src/actor/otel.rs | Switches to SAIStatsBatchMessage and processes records in-order while preserving flush/backoff behavior. |
| crates/countersyncd/src/actor/counter_db.rs | Switches to batched stats input and updates caching/write pipeline to iterate per record. |
| crates/countersyncd/Cargo.toml | Removes unused dependencies (ipfixrw, binrw, rand) as the strict decoder path replaces the generic parser approach. |
| crates/countersyncd/benches/otel_actor_perf.rs | Updates benchmark message generation to produce batched stats and Arc<str> object names. |
| crates/countersyncd/benches/ipfix_bench_data.rs | Reworks dataset/template preparation, adds readiness probe template/record, and includes object metadata. |
| crates/countersyncd/benches/ipfix_actor_perf.rs | Updates perf benchmark to use readiness probe and batched stats accounting. |
| crates/countersyncd/benches/end_to_end.rs | Updates end-to-end bench to use readiness drain/verification and batched channels for all sinks. |
| crates/countersyncd/benches/counter_db_actor_perf.rs | Updates CounterDB benchmark to send multi-record batches and validates actor completion. |
| Cargo.toml | Removes workspace-level unused deps corresponding to dropped generic IPFIX parser tooling. |
| Cargo.lock | Drops transitive deps tied to removed crates and normalizes syn/owo-colors entries accordingly. |
Review details
- Files reviewed: 18/20 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+51
to
+56
| const CHANNEL_LABEL_COUNT: usize = 6; | ||
|
|
||
| impl ChannelLabel { | ||
| const fn index(self) -> usize { | ||
| self as usize | ||
| } |
Comment on lines
+214
to
+218
| debug!( | ||
| "Received SAI stats message with {} counters at time {}", | ||
| msg.stats.len(), | ||
| msg.observation_time | ||
| ); |
Comment on lines
+293
to
+299
| self.total_messages_received += 1; | ||
|
|
||
| debug!( | ||
| "Received SAI stats with {} entries, observation_time: {}", | ||
| stats.stats.len(), | ||
| stats.observation_time | ||
| ); |
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 I did
This change hardens and optimizes the
countersyncdHFT IPFIX processing path.HFT decoder
ipfixrwobject-graph parser with a strict, fixed-width SONiC HFT decoder.(observation_domain_id, template_id)instead of a process-global template ID.IpfixActorrather than thread-local storage.Template lifecycle
Safety and backpressure
observationTimeNanosecondsfield and at least one enterprise counter per HFT template.SAIStatsBatchmessages and share object names withArc<str>.Why I did it
The previous hot path built a generic IPFIX object graph for every message. It cloned the expanded template per record, allocated a
HashMapper record and aVec<u8>per field, scanned fields repeatedly, cloned every object name, and sent every record as a separate channel message.Malformed length zero could also spin forever, template deletion did not remove parser state, template identity ignored observation domains, one unknown template could discard unrelated sets, and a slow sink serialized delivery to every other sink.
Performance
Criterion workload: approximately four million counters per case, release build,
swss-bookworm-master, one pinned CPU for the optimized run.These numbers are not a pure parser-only A/B. The benchmark was also corrected to prebuild inputs, use a causal readiness probe, and exclude setup from the measured interval; the old benchmark generated and copied random records inside its timed path. The source-derived result that does not depend on timing is that the new decoder removes per-record template clones and field maps, per-field byte-vector allocations, and per-counter object-name allocation.
How I verified it
CI-equivalent environment:
sonicdev-microsoft.azurecr.io:443/sonic-slave-bookworm:master-amd64sha256:073697e9f029974f00dc9eb8f11c81d664082e2f4b802ec77f2408eb30577aa9latestFromBranch(master)common-lib, swss-common, and sairedis artifacts.azure-pipelines/build-template.ymlCommands:
Final result: 242 tests passed in both parallel and serial runs. Focused IPFIX tests cover malformed framing, random-input panic safety, multi-domain IDs, transactional active/pending lifecycle, supersession/rollback, delete tombstones, ordered deferred replay, malformed deferred data, multiple recipients, closed/full sinks, and deterministic field order.
HFT profile limitations
This is intentionally a SONiC HFT decoder, not a general RFC 7011 collector. It currently requires:
observationTimeNanosecondsencoded as the raw SONiC HFTu64timestamp;Options Templates, on-wire withdrawals, variable-length fields, generic IANA information elements, and RFC NTP timestamp decoding are rejected.
Follow-up issues not solved here
u64counters toi64and does not enforce an encoded-byte request limit.stat << 16 | type) conflicts with the existing countersyncd implementation (type << 16 | stat). This change preserves the existing Rust behavior and adds distinct type/stat tests; the HLD should be corrected separately.