First update after init review

doc/dhcp_relay/DHCPv4-per-interface-counter.md

@@ -35,11 +35,9 @@ This document describes the high level design details about how **DHCPv4 Relay p
 
 ## Problem Statement
 
-Currently, DHCPv4 counter in dhcpmon mainly focus on Vlan / PortChannel interface packet statistics and it store count statics in memory. Also dhcpmon only reports issue in syslog for DHCPv4 packets relayed disparity. User cannot actively query packets count. When issue like DHCP client cannot get ip happens, we need to do some packets capture and analyze to fingure out whether the packets received in switch and whether the packets has been relayed.
-
-Below picture describe the workflow of current dhcpmon.
-
-<div align="center"> <img src=images/old_dhcpmon.png width=480 /> </div>
+Currently, DHCPv4 counter in dhcpmon mainly focus on Vlan / PortChannel interface packet statistics and it store count statics in memory. Also dhcpmon only reports issue in syslog for DHCPv4 packets relayed disparity. It has below disadvantages:
+1. The counteng granularity is Vlan/PortChannel, the data for physical interface is missing.
+2. There is no API for User to actively query packets count. When issue like DHCP client cannot get ip happens, we need to do some packets capture and analyze to fingure out whether the packets received in switch and whether the packets has been relayed.
 
 ## Functional Requirements
 
@@ -50,29 +48,40 @@ Below picture describe the workflow of current dhcpmon.
 
 ## Design Overview
 
-Below picture describe the new counter work flow. The highlighting part is what we change for per-interface counter.
+To address above 2 issues, we propose below design, it could be devided into 3 parts: `Init`, `Per-interface counting` and `Persist`
+
+### Init
+
+<div align="center"> <img src=images/init.png width=530 /> </div>
+
+1. There are 2 sockets for all interfaces to capture DHCPv4 packets. We bind callback to the network events to process packets.
+2. Read from `PORTCHANNEL_MEMBER` and `VLAN_MEMBER` table CONFIG_DB to construct mapping between physical interface and PortChannel/Vlan. It's used to query context interface by physical interface.
+3. We have 2 kind of counters, one is persisted in STATE_DB, another is stored in process memory, they are initialized when process startup.
+4. Initialize a timer to periodically sync counter data from cache counter to DB counter.
 
-<div align="center"> <img src=images/new_dhcpmon_block_incre.png width=630 /> </div>
+### Per-interface counting
 
-1. Initialize rx/tx socket.
-2. Update Vlan-physical interface and PortChannel-physical interface map.
-3. Add timer events.
-4. Initialize cache counter.
-5. Different event callbacks handle packet event and timer event.
+<div align="center"> <img src=images/per_intf_counting.png width=600 /> </div>
 
-Below is sequence picture for it.
-- Startup process is similar as previous. The different is that cache counter contains more interfaces' data and it's incremental data rather than full data.
-- In libevent timer, dhcpmon would update STATE_DB by incremental cache counter and clear counter after healthy check.
+* From socket, we can fingure out which physical is the packet came from.
+* Context interface name could be obtained by querying context interface counter.
+* Then cache counter for corresponding physical interface and context would increase **immediately**.
 
-<div align="center"> <img src=images/new_dhcpmon_seq.png width=600 /> </div>
+### Persist
 
-### Counter Logic
+<div align="center"> <img src=images/persist.png width=450 /> </div>
+
+DB update timer would be invoked periodically (**every 20s**), it will obtain the counter data increased during the statistical period from the cache counter
+
+## Counter Logic
+
+### Overview
 
 Below pictures are samples for expected counter increasing for both directions.
 
-<div align="center"> <img src=images/counter_sample.png width=800 /> </div>
+<div align="center"> <img src=images/counter_sample.png width=600 /> </div>
 
-dhcpmon would main focus on comparing below 3 fields in packet when counting packets. Point 1 and point 2 is used for counting context interface. And point 3 is used to get packet type.
+dhcpmon would main focus on comparing below 3 fields in packet when counting packets. Point 1 and point 2 is used for counting context interface, **we only count client-sent packets which come from downlink Vlan and server-sent packets which come from uplink interfaces**. And point 3 is used to get packet type.
 1. `Destination ip Address` in **IP header**
 2. `Gateway IP Address` in **DHCPv4 header**
 3. `Option 53` in **DHCPv4 header**
@@ -84,29 +93,13 @@ Counter for context interface should be increased in below scenarios, and they c
 | RX packet | **A**: If context interface is not uplink (Vlan) | **B**: If dst ip in ip header equals to context gateway and context interface is uplink (PortChannel) |
 | TX packet | **C**: If gateway ip in dhcp header equals to context gateway ip and context interface is uplink | **D**: If context interface is not uplink (Vlan) |
 
-Below picture shows work flow for counting.
-
-<div align="center"> <img src=images/counter_logic.png width=800 /> </div>
-
-1. Get physical interface name
-2. Get context interface name by physical interface
-3. If physical interface is not uplink context interface, update physical interface counter (This logic is for the scenario that uplink interface is single physical interface rather than PortChannel)
-4. Update context interface counter with above logic
+### Dual-ToR Specified
 
-### Alert Logic
+In Dual-ToR there are some behaviors different with single ToR:
+1. We wouldn't count packets come from standby interfaces (Refer to `HW_MUX_CABLE_TABLE` in STATE_DB).
+2. In above counting logic, we would compare `gateway` in DHCP packets. In single ToR, the gateway is Vlan ip, but in Dual-ToR, it's device's Loopback ip.
 
-Libevent timer would check whehther all packets are relayed expected periodicly when there are packets received. When the unhealthy status is found after ten consecutive times checks, dhcpmon should report an alarm in syslog. Below is how dhcpmon determine unhealthy situation:
-
-1. For packets sent by client (Discover / Request / Decline / Release / Inform), the expected TX count depends on the number of DHCP server configured. Take below picture as example, there are 2 DHCP servers configured. If there is 1 RX Discover packet in Vlan1000, then there should total 2 TX Discover packets be found in PortChannel1 and Port Channel2. Hence when **`[RX number of downlink context intf] * [DHCP server number] > [TX number of uplink context interface]`**, dhcpmon would treat it as unhealthy.
-
-<div align="center"> <img src=images/client_to_server.png width=800 /> </div>
-
-2. For packets sent by server (Offer / Ack / Nak), the expected TX count in context intf should be equal to RX count in context intf. Hence when **`[RX number of uplink context intf] > [TX number of downlink context intf]`**, dhcpmon would treat it as unhealthy.
-<div align="center"> <img src=images/server_to_client.png width=800 /> </div>
-
-3. TX packets are expected to go through downlink or uplink route, but if there is issue with default route, TX packets maybe go through management port. Hence when **`TX number of management port is increasing`**, dhcpmon would treat it as unhealthy.
-
-## Counter aging
+## Counter Reset
 
 * Container restart
   * One dhcpmon process would only listen on one downlink Vlan interface, hence dhcpmon process restart will initialize (counter set to zero) for interface in below list:
@@ -119,7 +112,6 @@ Libevent timer would check whehther all packets are relayed expected periodicly
   * Member add: It's expected to set add entry and set counter to zero for member interface.
   * Member del: It's expected to delete related counter entry.
   * **Note: This requires db change subscription support. In early stage, we will mainly focus on key functionality. This feature maybe be supported in future.**
-* Vlan member interface flapping: keep counting.
 
 ## DB Change
 
@@ -130,7 +122,13 @@ Following table changes would be added in State DB, including **DHCP_COUNTER_TAB
 ```
 {
   'DHCP_COUNTER_TABLE': {
-    'Ethernet4': {
+    'Vlan1000|Ethernet4': {
+      'RX': '{"Ack":"0","Decline":"0","Discover":"1","Inform":"0","Nak":"0","Offer":"0","Release":"0","Request":"0","Unknown":"0"}',
+      'TX': '{"Ack":"0","Decline":"0","Discover":"1","Inform":"0","Nak":"0","Offer":"0","Release":"0","Request":"0","Unknown":"0"}'
+    }
+  },
+  'DHCP_COUNTER_TABLE': {
+    'Vlan1000|PortChannel1': {
       'RX': '{"Ack":"0","Decline":"0","Discover":"1","Inform":"0","Nak":"0","Offer":"0","Release":"0","Request":"0","Unknown":"0"}',
       'TX': '{"Ack":"0","Decline":"0","Discover":"1","Inform":"0","Nak":"0","Offer":"0","Release":"0","Request":"0","Unknown":"0"}'
     }
@@ -148,35 +146,55 @@ This command is used to show dhcp_relay counter.
 
 - Usage
     ```
-    show dhcp_relay ipv4 counter [--dir (TX|RX)] [<interface>]
+    show dhcp_relay ipv4 counter [--dir (TX|RX)] [--type <type>] <vlan_interface>
+
+    Options:
+        dir: Specify egress or ingress
+        type: Specify DHCP packet type
+    
+    Note: At least one of dir and type must be specified
     ```
 
 - Example
     ```
-    show dhcp_relay ipv4 counter Vlan1000
-        Message Type       Vlan1000(RX)
-    -------------------  -----------------
-                Unknown                  0
-            Discover                  0
-                Offer                  0
-                Request                  0
-                    Ack                  0
-                Release                  0
-                Inform                  0
-                Decline                  0
-                    Nak                  0
-
-            Message Type       Vlan1000(TX)
-    -------------------  -----------------
-                Unknown                  0
-            Discover                  0
-                Offer                  0
-                Request                  0
-                    Ack                  0
-                Release                  0
-                Inform                  0
-                Decline                  0
-                    Nak                  0
+    show dhcp_relay ipv4 counter Vlan1000 --dir TX
+    Packet type Abbr: Un - Unknown, Dis - Discover, Off - Offer, Req - Request,
+                      Ack - Acknowledge, Rel - Release, Inf - Inform,
+                      Dec - Decline
+    +-----------------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
+    | Vlan1000 (TX)         | Un  | Dis | Off | Req | Ack | Rel | Inf | Dec | Nak |
+    ------------------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
+    | Downlink - Ethernet1  | 0   | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   |
+    | Downlink - Ethernet2  | 0   | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   |
+    | Uplink - Ethernet46   | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   | 0   |
+    | Uplink - Ethernet47   | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   | 0   |
+    | Uplink - PortChannel1 | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   | 0   |
+    | Uplink - PortChannel2 | 0   | 2   | 0   | 2   | 0   | 0   | 0   | 0   | 0   |
+    +-----------------------+-----+-----+-----+-----+-----+-----+-----+-----+-----+
+
+    show dhcp_relay ipv4 counter Vlan1000 --type Discover
+    +----------------------+----+----+
+    |Vlan1000 (Discover)   | TX | RX |
+    +----------------------+----+----+
+    |Downlink - Ethernet1  | 0  | 2  |
+    |Downlink - Ethernet2  | 0  | 0  |
+    |Uplink - Ethernet46   | 2  | 0  |
+    |Uplink - Ethernet47   | 0  | 0  |
+    |Uplink - PortChannel1 | 2  | 0  |
+    |Uplink - PortChannel2 | 0  | 0  |
+    +----------------------+----+----+
+
+    show dhcp_relay ipv4 counter Vlan1000 --type Discover --dir TX
+    +----------------------+----+
+    |Vlan1000 (Discover)   | TX |
+    +----------------------+----+
+    |Downlink - Ethernet1  | 0  |
+    |Downlink - Ethernet2  | 0  |
+    |Uplink - Ethernet46   | 2  |
+    |Uplink - Ethernet47   | 0  |
+    |Uplink - PortChannel1 | 2  |
+    |Uplink - PortChannel2 | 0  |
+    +----------------------+----+
     ```
 
 ## Clear Cli
@@ -187,7 +205,7 @@ This command is used to clear DHCPv4 counter
 
 - Usage
     ```
-    sonic-clear dhcp_relay ipv4 counter [--dir (TX|RX)] [<interface>]
+    sonic-clear dhcp_relay ipv4 counter [--dir (TX|RX)] [<vlan_interface>]
     ```
 
 - Example