Repository navigation
[HFT][countersyncd]: Support SAI_TAM_TEL_TYPE_MODE_MIXED_TYPE - #4732
DavidZagury wants to merge 37 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
c2b0ef5 to
f89f6c9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
f89f6c9 to
ab1377d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
7eb50a5 to
2d21a64
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
5e1a9aa to
f43e15e
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
24 similar comments
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
| // Leaving this item in orchagent's own retry queue is what lets it | ||
| // get applied automatically once the profile is disabled. | ||
| const string blocked_key = profile_name + "|" + group_name; | ||
| if (profile->isMixedTypeMode() && |
There was a problem hiding this comment.
Could we simplify this by making canBeUpdated() allow configuration changes only in STOP_STREAM, rejecting both START_STREAM and CREATE_CONFIG? Then remove this object_names-specific gate and its extra blocked-key tracking. The common check should cover group additions, object/stat updates, and deletions consistently; currently stats-only updates and group deletion bypass this new gate.
Please also adjust profileTableSet(): it currently calls canBeUpdated() before processing disable, so tightening the helper alone would prevent a running profile from being stopped. Handle enable/disable through their state-transition checks, while keeping configuration fields such as poll_interval subject to the stopped-only rule. This centralizes the stop-modify-start contract instead of adding per-field checks.
d629733 to
6b5bba3
Compare
* MIXED mode doesn't support live reconfiguration: every group shares one stream and one combined IPFIX template, so an object-list change - and the label allocation it may need - is only safe to apply while the whole profile is stopped. Previously a group update was accepted at any time and simply stopped the stream as a side effect, with no guaranteed quiescent period beforehand * HFTelProfile::canBeUpdated(object_type) now centralizes this: in MIXED mode it requires the shared tel_type to be SAI_TAM_TEL_TYPE_STATE_STOP_STREAM; in SINGLE mode it keeps its prior behavior (blocks only CREATE_CONFIG), since SINGLE's own setters already stop just the type being changed as an existing, intentional side effect. HFTelOrch::groupTableSet's single canBeUpdated(type) check at its top now covers object_names, object_counters, and - via groupTableDel's own existing check - group deletion, instead of a bespoke check that only covered object_names and left object_counters updates and group deletion unguarded * task_need_retry (not task_failed) because this is transient: the GCU-based CLI apply path only writes CONFIG_DB deltas, so there is no way for an operator to "retry" by resubmitting an identical config once they disable the profile - nothing would change, so no write, so no new notification. Leaving the item in orchagent's own retry queue is what lets it get applied automatically, with the original desired configuration, once the profile is disabled * Log once (not on every retry poll, since task_need_retry items are re-attempted on every doTask pass) when a group first becomes blocked this way, so the rare case where the CLI's own pre-flight guard doesn't catch this first is still debuggable instead of silent. Renamed the tracking set from m_mixed_live_reconfig_blocked to m_group_update_blocked since it's no longer MIXED-specific * HFTelOrch::profileTableSet previously called the blanket canBeUpdated() before handling stream_state, so tightening canBeUpdated() alone would have blocked disabling a profile that's actively streaming - the only way to reach STOP_STREAM from there. stream_state (enable/disable) is now applied unconditionally via setStreamState()'s own transition checks, which are always safe regardless of current state (the only unimplemented transition, START_STREAM -> CREATE_CONFIG, is never requested by stream_state). poll_interval remains subject to the stopped-only rule via canBeUpdated() * poll_interval's gate also accepts a stream_state=disabled present in that same update, even though canBeUpdated() still reflects the pre-call state at that point: setStreamState(STOP_STREAM) always succeeds regardless of current state, so without this exception, disabling a running profile together with a poll_interval change in one CONFIG_DB write would retry forever - the gate would never pass, because the disable that would satisfy it is never reached (it's never applied, since the gate rejects the whole task first). stream_state=enabled does not grant this exception, since it doesn't stop the profile; poll_interval correctly keeps retrying in that case until a separate disable is issued * HFTelProfile::setObjectNames now returns bool instead of void, and groupTableSet fails the task if it returns false (the label allocator would exceed the 15-bit IPFIX IE range - a permanent condition for that exact request, so task_failed instead of task_need_retry). This also fixes a latent gap where that rejection was previously silently treated as applied * Add tests covering the new gate: rejecting object_names and object_counters updates and group deletion with task_need_retry while the profile is streaming, tracking the blocked key exactly once across repeated retries, rejecting a poll_interval change alone or together with a redundant enable while streaming, and applying poll_interval together with an actual disable in one pass instead of retrying forever Signed-off-by: david.zagury <davidza@nvidia.com>
* Both lines are hit only when HFT ends up disabled entirely: no SAI_TAM_TEL_TYPE_MODE is advertised at all, or SAI_TAM_TEL_TYPE_ATTR_MODE isn't an enum attribute as expected. A NOTICE is easy to miss when a platform silently loses the feature; WARN matches the operational impact Signed-off-by: david.zagury <davidza@nvidia.com>
6b5bba3 to
cd4a821
Compare
5 participants
What I did
Added support for
SAI_TAM_TEL_TYPE_MODE_MIXED_TYPEtoHFTelOrchHFTelProfile, plus the matching label-resolution path in CounterSyncd:SAI_TAM_TEL_TYPE_ATTR_MODEcapability at orchagent init and pickthe mode for the orchagent lifetime.
HFTelProfile. In MIXED mode theprofile creates exactly one
sai_tam_tel_typeand onesai_tam_reportper profile, with all three
SWITCH_ENABLE_*_STATSflags set. SAI-objectfan-out collapses from up-to-N (per object type) to 1 per profile.
HFTelProfilethrough amapKey()helper that returns the singletonSAI_OBJECT_TYPE_NULLin MIXED so SINGLE-mode behavior is preserved without per-call branching.
ready before transitioning the single tel_type into
CREATE_CONFIG.(
m_next_label), guaranteeing label uniqueness across all per-groupsessions of a profile. Required for the countersyncd aggregation below.
into every per-group
HIGH_FREQUENCY_TELEMETRY_SESSION_TABLEentry.object_id_name_mapentries across all sessionsthat registered the same
template_id. Necessary because MIXED hasmultiple per-group sessions sharing one template_id.
both, neither, query-not-supported) and DVS regression for per-group
session_configand the MIXED placeholder path.Why I did it
Some vendor SAI implementations advertise only
MIXED_TYPEforSAI_TAM_TEL_TYPE_ATTR_MODE. On those platforms today HFT cannot be enabled because the orchagent hardcodesSINGLE_TYPE. MIXED also reduces SAI-object count per profile (1 tel_type + 1 report vs up to N + N) on platforms that support both modes.How I verified it
make tests && ./testsintests/mock_tests/— all HFT mock UTs pass,including the new
hftelorch_*andhftelprofile_utcases for thecapability matrix and MIXED tel_type / report cardinality.
pytest tests/test_hft.pyagainst the DVS — the per-groupsession_configreplication path is exercised end-to-end.Details if related