{"action":"created","issue":{"url":"https://api.github.com/repos/opencomputeproject/SAI/issues/874","repository_url":"https://api.github.com/repos/opencomputeproject/SAI","labels_url":"https://api.github.com/repos/opencomputeproject/SAI/issues/874/labels{/name}","comments_url":"https://api.github.com/repos/opencomputeproject/SAI/issues/874/comments","events_url":"https://api.github.com/repos/opencomputeproject/SAI/issues/874/events","html_url":"https://github.com/opencomputeproject/SAI/pull/874","id":364966618,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE5MDI3OTQ0","number":874,"title":"Update saitam.h","user":{"login":"JaiOCP","id":42279853,"node_id":"MDQ6VXNlcjQyMjc5ODUz","avatar_url":"https://avatars1.githubusercontent.com/u/42279853?v=4","gravatar_id":"","url":"https://api.github.com/users/JaiOCP","html_url":"https://github.com/JaiOCP","followers_url":"https://api.github.com/users/JaiOCP/followers","following_url":"https://api.github.com/users/JaiOCP/following{/other_user}","gists_url":"https://api.github.com/users/JaiOCP/gists{/gist_id}","starred_url":"https://api.github.com/users/JaiOCP/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JaiOCP/subscriptions","organizations_url":"https://api.github.com/users/JaiOCP/orgs","repos_url":"https://api.github.com/users/JaiOCP/repos","events_url":"https://api.github.com/users/JaiOCP/events{/privacy}","received_events_url":"https://api.github.com/users/JaiOCP/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2018-09-28T16:53:38Z","updated_at":"2018-11-13T18:03:39Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/opencomputeproject/SAI/pulls/874","html_url":"https://github.com/opencomputeproject/SAI/pull/874","diff_url":"https://github.com/opencomputeproject/SAI/pull/874.diff","patch_url":"https://github.com/opencomputeproject/SAI/pull/874.patch"},"body":"TAM 2.0"},"comment":{"url":"https://api.github.com/repos/opencomputeproject/SAI/issues/comments/438375382","html_url":"https://github.com/opencomputeproject/SAI/pull/874#issuecomment-438375382","issue_url":"https://api.github.com/repos/opencomputeproject/SAI/issues/874","id":438375382,"node_id":"MDEyOklzc3VlQ29tbWVudDQzODM3NTM4Mg==","user":{"login":"JaiOCP","id":42279853,"node_id":"MDQ6VXNlcjQyMjc5ODUz","avatar_url":"https://avatars1.githubusercontent.com/u/42279853?v=4","gravatar_id":"","url":"https://api.github.com/users/JaiOCP","html_url":"https://github.com/JaiOCP","followers_url":"https://api.github.com/users/JaiOCP/followers","following_url":"https://api.github.com/users/JaiOCP/following{/other_user}","gists_url":"https://api.github.com/users/JaiOCP/gists{/gist_id}","starred_url":"https://api.github.com/users/JaiOCP/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/JaiOCP/subscriptions","organizations_url":"https://api.github.com/users/JaiOCP/orgs","repos_url":"https://api.github.com/users/JaiOCP/repos","events_url":"https://api.github.com/users/JaiOCP/events{/privacy}","received_events_url":"https://api.github.com/users/JaiOCP/received_events","type":"User","site_admin":false},"created_at":"2018-11-13T18:03:39Z","updated_at":"2018-11-13T18:03:39Z","author_association":"NONE","body":"Hi Mickey,\n\n \n\nThanks for reviewing the diffs.\n\nMost of your comments are about IFA/OAM API and attributes. Here is what we discussed about it and this spec before\nInitial thinking was to remove the IFA/OAM API and attributes. The changes are too intrusive and need to fixed in a followup PR anyways so the suggestion was to capture in the document that what APIs are not complete and in what priority they will be addressed in the followup commits. This has been done and document is checked in with updates. (Please see last meeting minutes)\nRegarding your comment about experimental. Origin of this spec is in to come up with a single consistent generic framework in lieu of chip specific (TAM 1.0 and DTEl) implementations. Team has spent considerable  amount of time iterating over it, refining it and plan is to push it as a single standard method of doing telemetry in SAI.\n \n\nRegards,\n\n-Jai\n\n \n\nFrom: Mickey Spiegel <notifications@github.com>\nReply-To: opencomputeproject/SAI <reply@reply.github.com>\nDate: Wednesday, November 7, 2018 at 9:01 PM\nTo: opencomputeproject/SAI <SAI@noreply.github.com>\nCc: Jai Kumar <jai.kumar@broadcom.com>, Author <author@noreply.github.com>\nSubject: Re: [opencomputeproject/SAI] Update saitam.h (#874)\n\n \n\n@mickeyspiegel requested changes on this pull request.\n\nThere has been a lot of good progress made, particularly around port stats, queue stats, and get APIs. However, the proposed scope of the TAM 2 project is much wider than that. A large part of this functionality has either not been delivered or has only been partially developed.\n\nI see two paths forward:\nIf it is important to get some functionality approved sooner rather than later, then incomplete functionality such as probes / OAM and flows should be removed entirely from this proposal. They can always be added in again when they are more complete.\nIf it is desired to put a stake in the ground with the understanding that further development is required and things may change, then at most a status of experimental can be agreed to at this time.\nIn inc/saitam.h:\n>       *\n-     * Example: SAI_INGRESS_PRIORITY_GROUP_STAT_CURR_OCCUPANCY_BYTES.\n+     * @type sai_object_list_t\n+     * @flags CREATE_AND_SET\n+     * @objects SAI_OBJECT_TYPE_TAM_PROBE\n+     * @default empty\n+     */\n+    SAI_TAM_ATTR_PROBE_OBJECTS_LIST,\nThe TAM probe / OAM functionality is incomplete, needs to be reworked. This should be removed until a more complete proposal is presented.\n\nIn inc/saitam.h:\n>          _In_ uint32_t attr_count,\n         _Inout_ sai_attribute_t *attr_list);\n \n /**\n- * @brief TAM Statistic\n- *\n- * Identifies a specific counter within the SAI object hierarchy\n- * and provides the current value of the counter.\n+ * @brief IFA header Object Attributes\nWhy are there attributes and APIs defined for a proprietary header format?\n\nDefining dataplane telemetry contents by header seems overly specific to synthetic probes, and does not address how dataplane telemetry can be applied to in-situ / inband traffic.\n\nThis should be removed until a more complete proposal is presented.\n\nIn inc/saitam.h:\n>  \n /**\n- * @brief TAM Threshold Breach Event notification\n+ * @brief IFA Probe Object\nWhy are probes specific to the proprietary IFA format?\n\nThere does not seem to be anything addressing functionality beyond header contents?\nWhen are probes sent?\nDue to what triggers?\nHow often?\n\nWhat about OAM functionality beyond probes?\n\nThis should be removed until a more complete proposal is presented.\n\nIn inc/saitam.h:\n> +     */\n+    SAI_TAM_TELEMETRY_TYPE_FABRIC,\n+\n+    /**\n+     * @brief Flow TAM\n+     * All the data relevant to a given flow\n+     */\n+    SAI_TAM_TELEMETRY_TYPE_FLOW,\n+\n+    /**\n+     * @brief INT TAM\n+     * All the data relevant on a per packet basis\n+     */\n+    SAI_TAM_TELEMETRY_TYPE_INT\n+\n+} sai_tam_telemetry_type_t;\nWhen a TAM object is bound to a port and attributes are set to enable collection of port or queue stats, what should the telemetry type value be set to?\n\nIn inc/saitam.h:\n> +\n+/**\n+ * @brief TAM telemetry types supported\n+ */\n+typedef enum _sai_tam_telemetry_type_t\n+{\n+    /**\n+     * @brief Networking element TAM\n+     */\n+    SAI_TAM_TELEMETRY_TYPE_NE,\n+\n+    /**\n+     * @brief Switch silicon TAM\n+     * All the data relevant to switch\n+     */\n+    SAI_TAM_TELEMETRY_TYPE_SWITCH,\nThese could use further elaboration.\nWhat is the difference between NE and SWITCH?\n\nIn inc/saitam.h:\n> +     * Switch Identifier can be an encoded number or an IP address\n+     *\n+     * @type sai_uint32_t\n+     * @flags CREATE_AND_SET\n+     * @default 0\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_INT_SWITCH_IDENTIFIER,\n+\n+    /**\n+     * @brief Flow - Flow ID\n+     *\n+     * @type sai_uint32_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_AND_SET\n+     * @condition SAI_TAM_TEL_TYPE_ATTR_TAM_TELEMETRY_TYPE == SAI_TAM_TELEMETRY_TYPE_FLOW\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_FLOW_ID,\nWhat does a flow ID represent?\n\nThere does not seem to be any mechanism to identify a subset of traffic for which flow reports are to be generated.\nThere is no binding to ACLs or filters, which are very commonly used to scope flow reports.\n\nThere does not seem to be a differentiation between the granularity of traffic to be captured in flow reports, e.g. IP source address 10.10.0.0/16, and the granularity at which flow statistics should be captured, for example every 5-tuple.\n\nUntil there is a proposal that addresses mainstream IPFIX use cases, all of the flow functionality should be removed.\n\nIn inc/saitam.h:\n> +    SAI_TAM_REPORT_TYPE_PROTO,\n+\n+    /**\n+     * @brief Report using THRIFT\n+     */\n+    SAI_TAM_REPORT_TYPE_THRIFT,\n+\n+    /**\n+     * @brief Report using JSON\n+     */\n+    SAI_TAM_REPORT_TYPE_JSON,\n+\n+    /**\n+     * @brief Report using INT\n+     */\n+    SAI_TAM_REPORT_TYPE_INT,\nThis should really be SAI_TAM_REPORT_TYPE_DTEL, referring to the p4.org Telemetry Report specification which can apply to data plane telemetry using postcards as well as INT.\n\nIn inc/saitam.h:\n> +    /**\n+     * @brief Transport attributes object\n+     *\n+     * @type sai_object_id_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_AND_SET\n+     * @objects SAI_OBJECT_TYPE_TAM_TRANSPORT\n+     */\n+    SAI_TAM_COLLECTOR_ATTR_TRANSPORT,\n+\n+    /**\n+     * @brief DSCP value\n+     *\n+     * @type sai_uint8_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_AND_SET\n+     */\n+    SAI_TAM_COLLECTOR_ATTR_DSCP_VALUE,\nTelemetry can generate significant amounts of traffic. Differentiating between telemetry traffic using DSCP is needed so that some telemetry traffic can be prioritized over others. In other words, DSCP should move out of collector attributes, and instead move into TAM event attributes and TAM telemetry type attributes.\n\nIn inc/saitam.h:\n> +    /**\n+     * @brief INT - Switch Identifier\n+     *\n+     * Switch Identifier can be an encoded number or an IP address\n+     *\n+     * @type sai_uint32_t\n+     * @flags CREATE_AND_SET\n+     * @default 0\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_INT_SWITCH_IDENTIFIER,\n+\n+    /**\n+     * @brief Flow - Flow ID\n+     *\n+     * @type sai_uint32_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_AND_SET\nWhat does mandatory mean here?\nIf all that is desired is collecting port and queue stats, then there would be no flow ID.\n\nIn inc/saitam.h:\n> +\n+    /**\n+     * @brief Flow TCP FLAGS event\n+     * All TCP Flags are monitored for change\n+     */\n+    SAI_TAM_EVENT_TYPE_FLOW_TCPFLAG,\n+\n+    /**\n+     * @brief Queue depth or latency threshold event\n+     * Queue occupancy threshold\n+     */\n+    SAI_TAM_EVENT_TYPE_QUEUE_THRESHOLD,\n+\n+    /**\n+     * @brief Queue tail drop event\n+     * Number of packets dropped as tail drops \nWhat does mandatory mean here?\nDepending on the event type, not every event would need a threshold to be defined.\n\nIn data/TAM/sai_tam_event.proto:\n> + * to Broadcom Limited and/or its subsidiaries.\n+ \n+ * Licensed under the Apache License, Version 2.0 (the \"License\");\n+ * you may not use this file except in compliance with the License.\n+ \n+ * You may obtain a copy of the License at\n+ * http://www.apache.org/licenses/LICENSE-2.0\n+ \n+ * Unless required by applicable law or agreed to in writing, software\n+ * distributed under the License is distributed on an \"AS IS\" BASIS,\n+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n+ * See the License for the specific language governing permissions and\n+ * limitations under the License.\n+ */\n+ \n+syntax = \"proto2\";\nWhy are we using proto2 when most new proposals have moved on to proto3?\n\nIn data/TAM/sai_tam_port.proto:\n> +    optional string if_admin_status        = 10;\n+\n+    // Interface operational status\n+    optional string if_op_status           = 11;\n+\n+    // number of carrier transitions on this interface\n+    optional uint64 if_transitions         = 12 [(telemetry_options).is_counter = true]; \n+\n+    // This corresponds to the ifLastChange object in the standard interface MIB\n+    optional uint32 ifLastChange           = 13;\n+\n+    // This corresponds to the ifHighSpeed object in the standard interface MIB\n+    optional uint32 ifHighSpeed            = 14;\n+\n+    repeated QueueStats egress_queue_info  = 15;    // egress queue information\n+    repeated QueueStats ingress_queue_info = 16;    // ingress queue information\nWhen we have messages like this with many attributes, it is likely that the list will be incomplete. Moreover different implementations may want to extend this list in different ways. There should be a more flexible way to extend the contents.\n\nIn proto2, extensions can be used to achieve this.\n\nIn proto3, extensions have been replaced by the Any type, which is more flexible.\n\nIn inc/saitam.h:\n> +     * @type sai_tam_telemetry_type_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_ONLY\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_TAM_TELEMETRY_TYPE = SAI_TAM_TEL_TYPE_ATTR_START,\n+\n+    /**\n+     * @brief INT - Template Id\n+     *\n+     * Template id indicates the supported metadata header.\n+     * Device can support one or more template IDs\n+     *\n+     * @type sai_uint8_t\n+     * @flags CREATE_AND_SET\n+     * @default 0\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_INT_TEMPLATE_ID,\nWhat is this? Needs more description.\n\nIn inc/saitam.h:\n> +     * @brief Flow - Duration in Milliseconds\n+     *\n+     * @type sai_int32_t\n+     * @flags CREATE_AND_SET\n+     * @default 0\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_FLOW_DURATION,\n+\n+    /**\n+     * @brief Flow - Size in Kilo Bytes\n+     *\n+     * @type sai_int32_t\n+     * @flags CREATE_AND_SET\n+     * @default 0\n+     */\n+    SAI_TAM_TEL_TYPE_ATTR_FLOW_SIZE,\nWhat does this do? Needs more description.\n\nIn inc/saitam.h:\n> +    /**\n+     * @brief Transport type\n+     *\n+     * @type sai_tam_transport_type_t\n+     * @flags MANDATORY_ON_CREATE | CREATE_ONLY\n+     */\n+    SAI_TAM_TRANSPORT_ATTR_TRANSPORT_TYPE = SAI_TAM_TRANSPORT_ATTR_START,\n+\n+    /**\n+     * @brief Transport src port\n+     *\n+     * @type sai_uint32_t\n+     * @flags CREATE_AND_SET\n+     * @default 31337\n+     */\n+    SAI_TAM_TRANSPORT_ATTR_SRC_PORT,\nThere can be significant amounts of telemetry traffic generated. The source port should be used for entropy rather than being statically configured, allowing for telemetry traffic to be delivered over multiple paths.\n\n—\nYou are receiving this because you authored the thread.\nReply to this email directly, view it on GitHub, or mute the thread.\n\n"}}
{"actor":{"display_login":"JaiOCP"}}