-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SAI Proposal TAM stream telemetry #2089
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3084,6 +3084,46 @@ typedef enum _sai_switch_attr_t | |
*/ | ||
SAI_SWITCH_ATTR_EXTENDED_PORT_STATE_CHANGE_NOTIFY, | ||
|
||
/** | ||
* @brief Tam telemetry reporting byte size of chunk under the stream telemetry | ||
* | ||
* Defines the maximum number of bytes in a single report. | ||
* The total number of bytes in a report should be as close as possible to this value. | ||
* We can increase this value to reduce the number of sys calls. | ||
* If the type of message is IPFIX, this value should not be less than 65535. | ||
Pterosaur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Because we don't expect the IPFIX record to be fragmented. | ||
* | ||
* @type sai_uint32_t | ||
* @flags CREATE_ONLY | ||
* @default 65535 | ||
*/ | ||
SAI_SWITCH_ATTR_TAM_ST_REPORT_CHUNK_SIZE, | ||
|
||
/** | ||
* @brief Tam telemetry chunk count under the stream telemetry | ||
* | ||
* This value indicates how many chunks of reports that can be restored in the buffer. | ||
* If the data structure is a ring buffer, the byte size of ring buffer is chunk count * chunk size. | ||
* The default value, 0, means that this value was determined by the vendor. | ||
* If the buffer is full, new incoming data will be dropped. | ||
* | ||
* @type sai_uint32_t | ||
* @flags CREATE_ONLY | ||
* @default 0 | ||
*/ | ||
SAI_SWITCH_ATTR_TAM_ST_CHUNK_COUNT, | ||
|
||
/** | ||
* @brief Set TAM telemetry type config change event notification callback function passed to the adapter. | ||
* | ||
* Use sai_tam_tel_type_config_change_notification_fn as notification function. | ||
* | ||
* @type sai_pointer_t sai_tam_tel_type_config_change_notification_fn | ||
* @flags CREATE_AND_SET | ||
* @default NULL | ||
*/ | ||
SAI_SWITCH_ATTR_TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be moved to TAM object ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like all notify handlers have to be bound on the switch object. It's the limitation of SAI design. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if its SAI design limitation, or just that all callback handlers are at switch level currently. @kcudnik please comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Bandaru, yes, in my understanding, this is basically how the notify handler works. The handler will be used by all objects in that type, hence it is added on upper layers, and the best place is the switch, as it will be setup during the initialization. For example, fob handler will be used by all fdb entries, hence it is not tied to any fdb entry. This is why the notifications are added on the switch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Currently it's forced that all notifications be in switch, this will unify entire design, and it will help update pointers when switch is put into warm boot mode, currently we don't have specific notifications per object and we are not planning to do so, more explanation is like Riff said in above comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can return object_id of that tam in notification, and act accordingly, we currently are not planning to decouple pointers from switch to single objects, we generate a lot of metadat which help us organize everygnig, including notification pointers assignments, which are coupled to the switch, at this point it will be up to user to decide differnt action based on notificaion data do you have any concearns about that? like performance reasons ? will there be expected a lot of tam notifications per second ? internally in vendor implementation you can still have "differnet" pointers on each created internally tam objects, but assign them the same pointer that was given by switch object There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a restriction in the metadata and other aspects, I think it is OK. I will wait for the name edits that I suggested above, before approving this PR. This callback item is closed from my side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those are not hard restrictions, but we're made for convince, and drifting away from current schema would cause a lot of refactoring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats OK, I am not talking about hard restrictions alone. No need to change all the infra for this for now. However, I request you to consider the possibility that the callbacks may - at sometime in future - may be per-object and plan any changes for this. Thanks @kcudnik There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is also another side why all pointers are in switch, during warm boot of the platform, user is calling create_switch, and it can pass new pointers to the switch to receive notifications, this is importnt, since after platform warm boot, new notifications wii most likely have different memory address than previous boot, so notifications would need to be updated. and this could be done in a single call during create switch, if you would have pointers in objects , you would need to go and find that object and assign new notification on it or you would have invalid pointer, or null, depends how platform handles pointers in sdk |
||
|
||
/** | ||
* @brief End of attributes | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -910,6 +910,51 @@ typedef enum _sai_tam_telemetry_type_t | |||||||||||||
|
||||||||||||||
} sai_tam_telemetry_type_t; | ||||||||||||||
|
||||||||||||||
typedef enum _sai_tam_tel_type_mode_t | ||||||||||||||
{ | ||||||||||||||
/** | ||||||||||||||
* @brief This TAM telemetry type supports to bound only one counter type | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_MODE_SINGLE_TYPE, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief This TAM telemetry type supports to bound multiple counter types | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_MODE_MIXED_TYPE, | ||||||||||||||
|
||||||||||||||
} sai_tam_tel_type_mode_t; | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief TAM telemetry type state of state machine | ||||||||||||||
*/ | ||||||||||||||
typedef enum _sai_tam_tel_type_state_t | ||||||||||||||
{ | ||||||||||||||
/** | ||||||||||||||
* @brief Telemetry type is stopped | ||||||||||||||
* | ||||||||||||||
* In this stage, the recording stream should be stopped, | ||||||||||||||
* and the configuration should be cleared. | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_STATE_STOP_STREAM, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief Telemetry type is started | ||||||||||||||
* | ||||||||||||||
* In this stage, the recording stream should be started, | ||||||||||||||
* and the latest configuration should be applied. | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_STATE_START_STREAM, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief Telemetry type configuration is prepared, | ||||||||||||||
* | ||||||||||||||
* We expect the configuration to be generated in the feature, | ||||||||||||||
* And notify the user by sai_tam_tel_type_config_change_notification_fn | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_STATE_CREATE_CONFIG, | ||||||||||||||
|
||||||||||||||
} sai_tam_tel_type_state_t; | ||||||||||||||
|
||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpik, names of the attributes are not clear about the intent of the state machine. FSM will look like this |
||||||||||||||
/** | ||||||||||||||
* @brief Telemetry type attributes | ||||||||||||||
*/ | ||||||||||||||
|
@@ -1076,6 +1121,35 @@ typedef enum _sai_tam_tel_type_attr_t | |||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_ATTR_COUNTER_SUBSCRIPTION_LIST, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief The mode of TAM telemetry type | ||||||||||||||
* | ||||||||||||||
* @type sai_tam_tel_type_mode_t | ||||||||||||||
* @flags CREATE_ONLY | ||||||||||||||
* @default SAI_TAM_TEL_TYPE_MODE_SINGLE_TYPE | ||||||||||||||
* @validonly SAI_TAM_TEL_TYPE_ATTR_TAM_TELEMETRY_TYPE == SAI_TAM_TELEMETRY_TYPE_COUNTER_SUBSCRIPTION | ||||||||||||||
*/ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity, it should be marked as: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea,thanks for your suggestion. |
||||||||||||||
SAI_TAM_TEL_TYPE_ATTR_MODE, | ||||||||||||||
Pterosaur marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief TAM telemetry type state | ||||||||||||||
* | ||||||||||||||
* @type sai_tam_tel_type_state_t | ||||||||||||||
* @flags CREATE_AND_SET | ||||||||||||||
* @default SAI_TAM_TEL_TYPE_STATE_STOP_STREAM | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_ATTR_STATE, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief Query IPFIX template | ||||||||||||||
* SAI adapter will return error if COUNTER_SUBSCRIPTION_LIST and REPORT_ID is not configured. | ||||||||||||||
* Return the IPFIX template for this telemetry type object. | ||||||||||||||
* | ||||||||||||||
* @type sai_u8_list_t | ||||||||||||||
* @flags READ_ONLY | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_TEL_TYPE_ATTR_IPFIX_TEMPLATES, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief End of Attributes | ||||||||||||||
*/ | ||||||||||||||
|
@@ -1088,6 +1162,14 @@ typedef enum _sai_tam_tel_type_attr_t | |||||||||||||
SAI_TAM_TEL_TYPE_ATTR_CUSTOM_RANGE_END | ||||||||||||||
} sai_tam_tel_type_attr_t; | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief TAM telemetry state change callback | ||||||||||||||
* | ||||||||||||||
* @param[in] tam_tel_id Create Telemetry Object ID | ||||||||||||||
*/ | ||||||||||||||
typedef void (*sai_tam_tel_type_config_change_notification_fn)( | ||||||||||||||
_In_ sai_object_id_t tam_tel_id); | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief Create and return a telemetry type object | ||||||||||||||
* | ||||||||||||||
|
@@ -2192,13 +2274,23 @@ typedef enum _sai_tam_counter_subscription_attr_t | |||||||||||||
* @brief Telemetry label | ||||||||||||||
* | ||||||||||||||
* Label to identify this counter in telemetry reports. | ||||||||||||||
* If the report type is IPFIX, this label will be used as the element ID in the IPFIX template. | ||||||||||||||
* | ||||||||||||||
* @type sai_uint64_t | ||||||||||||||
* @flags CREATE_ONLY | ||||||||||||||
* @default 0 | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_LABEL, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief Setting of read-clear or read-only for statistics read. | ||||||||||||||
* | ||||||||||||||
* @type sai_stats_mode_t | ||||||||||||||
* @flags CREATE_ONLY | ||||||||||||||
* @default SAI_STATS_MODE_READ | ||||||||||||||
*/ | ||||||||||||||
SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STATS_MODE, | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* @brief End of Attributes | ||||||||||||||
*/ | ||||||||||||||
|
@@ -2370,6 +2462,8 @@ typedef struct _sai_tam_api_t | |||||||||||||
sai_remove_tam_counter_subscription_fn remove_tam_counter_subscription; | ||||||||||||||
sai_set_tam_counter_subscription_attribute_fn set_tam_counter_subscription_attribute; | ||||||||||||||
sai_get_tam_counter_subscription_attribute_fn get_tam_counter_subscription_attribute; | ||||||||||||||
sai_bulk_object_create_fn create_tam_counter_subscriptions; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use a tam string as part of the type for consistency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's a convention of SAI to use sai_bulk_object_xxxx for bulk operation. Lines 395 to 396 in ad20062
Lines 335 to 338 in ad20062
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||||||||||||||
sai_bulk_object_remove_fn remove_tam_counter_subscriptions; | ||||||||||||||
} sai_tam_api_t; | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -222,3 +222,5 @@ Wildcard | |
www | ||
xconnect | ||
TWAMP | ||
config | ||
sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should make this more generic ? since very similar function already exists and this is adding only 1 field to structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi Kamil, we discussed internally using the sai_query_stats_capability directly and add the polling interval in it, but the reason we are not using that API is because of 2 reasons:
So after discussing it, we ended up deciding creating a new API for it.