Fix issue of high stats value being sent from VN UVE.

Issue
VN stats are picked from flow statistics.

Statistics for VN are collected from flow statistics. Agent periodically collects flow stats from vrouter and updates its statistics for flow.
During such collection agent also detects whether 6 byte statistics provided by vrouter has resulted in overflow. If it detects overflow, it will increment stats by value of 0x0001000000000000 (first bit of 7th byte) to account for overflow. Because of bug in overflow detection, the stats were getting incremented by 0x0001000000000000, even when overflow has not really occurred. This is resulting in high VN stats.

During periodic stats collection from vrouter, agent detects overflow by checking if the value of agent copy of stats is bigger than the value of vrouter copy of stats. The agent copy of stats was higher than vrouter copy due to the following reasons
1.  When gen-id/flow-handle of a flow changes, statistics for the flow were not reset by stats collection module.
2.  During periodic scanning of flows, the stats collection module used current/updated flow-handle and gen-id from the flow.
3.  When vrouter sends message for stats collection, agent was not using the gen-id in the message.

Fix
1.When stats collection module receives add for a flow which is already present, reset stats for the flow if its gen-id/flow-handle (identified by change of flow uuid) has changed.
2. During periodic scanning of flows, use gen-id/flow-handle with which the flow was added to stats collection module, instead of using the latest gen-id/flow-handle of flow.
3. The Evicted flows stats update message sent to stats collection module should carry UUID of the flow. In Stats collection module ignore the stats update message if the Flow UUID in the message does not match with its Flow UUID.
4. Read all the stats, flags, udp source port for flows in one shot and validate the read values. Use these read values for further processing instead of reading these fields from vrouter separately during processing.
5. When stats collection module is processing delete message for a flow, we read and export stats for the flow before it is deleted. To read stats here we should use gen-id/flow-handle from flow stats module instead of picking latest value from FlowEntry.

Change-Id: I1a852816dd890d49ddee8ff7fcafd4100c5212e4
Closes-Bug: #1654557

src/vnsw/agent/pkt/flow_entry.cc

@@ -470,9 +470,8 @@ void FlowEntry::Copy(FlowEntry *rhs, bool update) {
         gen_id_ = rhs->gen_id_;
         flow_handle_ = rhs->flow_handle_;
         /* Flow Entry is being re-used. Generate a new UUID for it. */
-        // results is delete miss for previous uuid to stats collector
-        // with eviction disabled following is not required
-        // uuid_ = flow_table_->rand_gen();
+        uuid_ = flow_table_->rand_gen();
+        egress_uuid_ = flow_table_->rand_gen();
     }
 }
 

src/vnsw/agent/pkt/flow_mgmt.cc

@@ -140,7 +140,8 @@ void FlowMgmtManager::DeleteEvent(FlowEntry *flow,
 
 void FlowMgmtManager::FlowStatsUpdateEvent(FlowEntry *flow, uint32_t bytes,
                                            uint32_t packets,
-                                           uint32_t oflow_bytes) {
+                                           uint32_t oflow_bytes,
+                                           const boost::uuids::uuid &u) {
     if (bytes == 0 && packets == 0 && oflow_bytes == 0) {
         return;
     }
@@ -151,7 +152,7 @@ void FlowMgmtManager::FlowStatsUpdateEvent(FlowEntry *flow, uint32_t bytes,
     }
     FlowMgmtRequestPtr req(new FlowMgmtRequest
                            (FlowMgmtRequest::UPDATE_FLOW_STATS, flow,
-                            bytes, packets, oflow_bytes));
+                            bytes, packets, oflow_bytes, u));
     request_queue_.Enqueue(req);
 }
 
@@ -430,13 +431,27 @@ bool FlowMgmtManager::RequestHandler(FlowMgmtRequestPtr req) {
                                        (FlowMgmtRequest::ADD_FLOW,
                                         req->flow().get()));
             log_queue_.Enqueue(log_req);
+
+            //Enqueue Add request to flow-stats-collector
+            agent_->flow_stats_manager()->AddEvent(req->flow());
+
+            //Enqueue Add request to UVE module for ACE stats
+            EnqueueUveAddEvent(flow);
+
             AddFlow(req->flow());
 
         } else {
             FlowMgmtRequestPtr log_req(new FlowMgmtRequest
                                        (FlowMgmtRequest::DELETE_FLOW,
                                         req->flow().get(), req->params()));
             log_queue_.Enqueue(log_req);
+
+            //Enqueue Delete request to flow-stats-collector
+            agent_->flow_stats_manager()->DeleteEvent(flow, req->params());
+
+            //Enqueue Delete request to UVE module for ACE stats
+            EnqueueUveDeleteEvent(flow);
+
             DeleteFlow(req->flow(), req->params());
         }
         break;
@@ -445,7 +460,7 @@ bool FlowMgmtManager::RequestHandler(FlowMgmtRequestPtr req) {
     case FlowMgmtRequest::UPDATE_FLOW_STATS: {
         //Handle Flow stats update for flow-mgmt
         UpdateFlowStats(req->flow(), req->bytes(), req->packets(),
-                        req->oflow_bytes());
+                        req->oflow_bytes(), req->flow_uuid());
         break;
     }
 
@@ -493,25 +508,11 @@ bool FlowMgmtManager::LogHandler(FlowMgmtRequestPtr req) {
     switch (req->event()) {
     case FlowMgmtRequest::ADD_FLOW: {
         LogFlowUnlocked(flow, "ADD");
-
-        //Enqueue Add request to flow-stats-collector
-        agent_->flow_stats_manager()->AddEvent(req->flow());
-
-        //Enqueue Add request to UVE module for ACE stats
-        EnqueueUveAddEvent(flow);
-
         break;
     }
 
     case FlowMgmtRequest::DELETE_FLOW: {
         LogFlowUnlocked(flow, "DEL");
-
-        //Enqueue Delete request to flow-stats-collector
-        agent_->flow_stats_manager()->DeleteEvent(flow, req->params());
-
-        //Enqueue Delete request to UVE module for ACE stats
-        EnqueueUveDeleteEvent(flow);
-
         break;
     }
 
@@ -674,10 +675,11 @@ void FlowMgmtManager::DeleteFlow(FlowEntryPtr &flow,
 }
 
 void FlowMgmtManager::UpdateFlowStats(FlowEntryPtr &flow, uint32_t bytes,
-                                      uint32_t packets, uint32_t oflow_bytes) {
+                                      uint32_t packets, uint32_t oflow_bytes,
+                                      const boost::uuids::uuid &u) {
     //Enqueue Flow Index Update Event request to flow-stats-collector
     agent_->flow_stats_manager()->UpdateStatsEvent(flow, bytes, packets,
-                                                   oflow_bytes);
+                                                   oflow_bytes, u);
 }
 
 bool FlowMgmtManager::HasVrfFlows(uint32_t vrf_id) {

src/vnsw/agent/pkt/flow_mgmt.h

@@ -1108,7 +1108,8 @@ class FlowMgmtManager {
     void AddEvent(FlowEntry *low);
     void DeleteEvent(FlowEntry *flow, const RevFlowDepParams &params);
     void FlowStatsUpdateEvent(FlowEntry *flow, uint32_t bytes, uint32_t packets,
-                              uint32_t oflow_bytes);
+                              uint32_t oflow_bytes,
+                              const boost::uuids::uuid &u);
     void AddDBEntryEvent(const DBEntry *entry, uint32_t gen_id);
     void ChangeDBEntryEvent(const DBEntry *entry, uint32_t gen_id);
     void DeleteDBEntryEvent(const DBEntry *entry, uint32_t gen_id);
@@ -1145,7 +1146,7 @@ class FlowMgmtManager {
     // Handle Delete of a flow. Updates FlowMgmtKeyTree for all objects
     void DeleteFlow(FlowEntryPtr &flow, const RevFlowDepParams &p);
     void UpdateFlowStats(FlowEntryPtr &flow, uint32_t bytes, uint32_t packets,
-                         uint32_t oflow_bytes);
+                         uint32_t oflow_bytes, const boost::uuids::uuid &u);
 
     // Add a FlowMgmtKey into the FlowMgmtKeyTree for an object
     // The FlowMgmtKeyTree for object is passed as argument

src/vnsw/agent/pkt/flow_mgmt_request.h

@@ -41,9 +41,11 @@ class FlowMgmtRequest {
     }
 
     FlowMgmtRequest(Event event, FlowEntry *flow, uint32_t bytes,
-                    uint32_t packets, uint32_t oflow_bytes) :
+                    uint32_t packets, uint32_t oflow_bytes,
+                    const boost::uuids::uuid &u) :
         event_(event), flow_(flow), db_entry_(NULL), vrf_id_(0), gen_id_(),
-        bytes_(bytes), packets_(packets), oflow_bytes_(oflow_bytes), params_() {
+        bytes_(bytes), packets_(packets), oflow_bytes_(oflow_bytes), params_(),
+        flow_uuid_(u) {
             if (event == RETRY_DELETE_VRF)
                 assert(vrf_id_);
     }
@@ -109,6 +111,7 @@ class FlowMgmtRequest {
     void set_params(const RevFlowDepParams &params) {
         params_ = params;
     }
+    boost::uuids::uuid flow_uuid() const { return flow_uuid_; }
 
 private:
     Event event_;
@@ -123,6 +126,7 @@ class FlowMgmtRequest {
     uint32_t packets_;
     uint32_t oflow_bytes_;
     RevFlowDepParams params_;
+    boost::uuids::uuid flow_uuid_;
 
     DISALLOW_COPY_AND_ASSIGN(FlowMgmtRequest);
 };

src/vnsw/agent/pkt/flow_table.cc

@@ -754,7 +754,8 @@ void FlowTable::ProcessKSyncFlowEvent(const FlowEventKSync *req,
             mgr->FlowStatsUpdateEvent(evicted_flow.get(),
                                       req->evict_flow_bytes(),
                                       req->evict_flow_packets(),
-                                      req->evict_flow_oflow());
+                                      req->evict_flow_oflow(),
+                                      evicted_flow->uuid());
         }
     }
 

src/vnsw/agent/pkt/pkt_flow_info.cc

@@ -1674,10 +1674,11 @@ void PktFlowInfo::UpdateEvictedFlowStats(const PktInfo *pkt) {
     FlowMgmtManager *mgr = agent->pkt()->flow_mgmt_manager(
                                flow_table->table_index());
 
+    /* Enqueue stats update request with UUID of the flow */
     if (flow.get() && flow->deleted() == false) {
         mgr->FlowStatsUpdateEvent(flow.get(), pkt->agent_hdr.cmd_param_2,
                                   pkt->agent_hdr.cmd_param_3,
-                                  pkt->agent_hdr.cmd_param_4);
+                                  pkt->agent_hdr.cmd_param_4, flow->uuid());
     }
 }
 

src/vnsw/agent/pkt/test/test_flow_table.cc

@@ -288,6 +288,65 @@ TEST_F(TestFlowTable, EvictPktTrapBeforeReverseFlowResp) {
     client->WaitForIdle();
 }
 
+TEST_F(TestFlowTable, ResetFlowStats) {
+    TxTcpMplsPacket(eth->id(), remote_compute, router_id_, vif0->label(),
+                    remote_vm1_ip, vm1_ip, 1000, 200, 1, 1);
+    client->WaitForIdle();
+
+    FlowEntry *flow = FlowGet(remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                              200, vif0->flow_key_nh()->id(), 1);
+    EXPECT_TRUE(flow != NULL);
+    FlowEntry *rflow = flow->reverse_flow_entry();
+    EXPECT_TRUE(rflow != NULL);
+
+    //Invoke FlowStatsCollector to check whether flow gets evicted
+    util_.EnqueueFlowStatsCollectorTask();
+    client->WaitForIdle();
+
+    //Verify the stats of flow
+    EXPECT_TRUE(FlowStatsMatch("vrf1", remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                               200, 1, 30, vif0->flow_key_nh()->id(), 1));
+
+    //Change the stats
+    KSyncSockTypeMap::IncrFlowStats(1, 1, 30);
+
+    //Invoke FlowStatsCollector to enqueue delete for evicted flow.
+    util_.EnqueueFlowStatsCollectorTask();
+    client->WaitForIdle();
+
+    //Verify the stats of flow
+    EXPECT_TRUE(FlowStatsMatch("vrf1", remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                               200, 2, 60, vif0->flow_key_nh()->id(), 1));
+
+    TxTcpMplsPacket(eth->id(), remote_compute, router_id_, vif0->label(),
+                    remote_vm1_ip, vm1_ip, 1000, 200, 1, 10);
+    client->WaitForIdle();
+
+    flow = FlowGet(remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                   200, vif0->flow_key_nh()->id(), 10);
+    EXPECT_TRUE(flow != NULL);
+
+    //Verify that flow-handle is updated in flow-stats module
+    FlowStatsCollector *fsc = flow->fsc();
+    EXPECT_TRUE(fsc != NULL);
+    FlowExportInfo *info = fsc->FindFlowExportInfo(flow);
+    EXPECT_TRUE(info != NULL);
+    EXPECT_TRUE(info->flow_handle() == 10);
+
+    //Verify the stats of flow are reset after change of flow-handle
+    EXPECT_TRUE(FlowStatsMatch("vrf1", remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                               200, 0, 0, vif0->flow_key_nh()->id(), 10));
+
+    //Invoke FlowStatsCollector to enqueue delete for reverse flow which is
+    //marked as short flow
+    util_.EnqueueFlowStatsCollectorTask();
+    client->WaitForIdle();
+
+    //Verify the stats of flow
+    EXPECT_TRUE(FlowStatsMatch("vrf1", remote_vm1_ip, vm1_ip, IPPROTO_TCP, 1000,
+                               200, 1, 30, vif0->flow_key_nh()->id(), 10));
+}
+
 int main(int argc, char *argv[]) {
     int ret = 0;
     GETUSERARGS();

src/vnsw/agent/test-xml/test_xml.cc

@@ -35,7 +35,7 @@ class FlowExportTask : public Task {
             return true;
         }
 
-        fec->ExportFlow(info, bytes_, pkts_, NULL);
+        fec->ExportFlow(info, bytes_, pkts_, NULL, true);
         return true;
     }
     std::string Description() const { return "FlowExportTask"; }

src/vnsw/agent/test/test_cmn_util.h

@@ -369,7 +369,8 @@ FlowEntry* FlowGet(int vrf_id, std::string sip, std::string dip, uint8_t proto,
                    uint16_t sport, uint16_t dport, int nh_id);
 bool FlowStatsMatch(const string &vrf_name, const char *sip, const char *dip,
                     uint8_t proto, uint16_t sport, uint16_t dport,
-                    uint64_t pkts, uint64_t bytes, int nh_id);
+                    uint64_t pkts, uint64_t bytes, int nh_id,
+                    uint32_t flow_handle = FlowEntry::kInvalidFlowHandle);
 bool FindFlow(const string &vrf_name, const char *sip, const char *dip,
               uint8_t proto, uint16_t sport, uint16_t dport, bool nat,
               const string &nat_vrf_name, const char *nat_sip,

src/vnsw/agent/test/test_util.cc

@@ -3452,7 +3452,8 @@ bool FlowGet(const string &vrf_name, const char *sip, const char *dip,
 
 bool FlowStatsMatch(const string &vrf_name, const char *sip,
                     const char *dip, uint8_t proto, uint16_t sport,
-                    uint16_t dport, uint64_t pkts, uint64_t bytes, int nh_id) {
+                    uint16_t dport, uint64_t pkts, uint64_t bytes, int nh_id,
+                    uint32_t flow_handle) {
     Agent *agent = Agent::GetInstance();
     VrfEntry *vrf = agent->vrf_table()->FindVrfFromName(vrf_name);
     EXPECT_TRUE(vrf != NULL);
@@ -3468,7 +3469,17 @@ bool FlowStatsMatch(const string &vrf_name, const char *sip,
     key.protocol = proto;
     key.family = key.src_addr.is_v4() ? Address::INET : Address::INET6;
 
-    FlowEntry *fe = agent->pkt()->get_flow_proto()->Find(key, 0);
+    FlowEntry *fe = NULL;
+    if (flow_handle != FlowEntry::kInvalidFlowHandle) {
+        FlowTable *table = agent->pkt()->get_flow_proto()->GetFlowTable(key,
+                                                                        flow_handle);
+        if (table == NULL) {
+            return NULL;
+        }
+        fe = table->Find(key);
+    } else {
+        fe = agent->pkt()->get_flow_proto()->Find(key, 0);
+    }
     EXPECT_TRUE(fe != NULL);
     if (fe == NULL) {
         return false;

src/vnsw/agent/vrouter/flow_stats/flow_export_info.cc

@@ -5,22 +5,28 @@ FlowExportInfo::FlowExportInfo() :
     flow_(), setup_time_(0), teardown_time_(0), last_modified_time_(0),
     bytes_(0), packets_(0), underlay_source_port_(0), changed_(false),
     tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0),
-    exported_atleast_once_(false) {
+    exported_atleast_once_(false), gen_id_(0),
+    flow_handle_(FlowEntry::kInvalidFlowHandle),
+    rev_flow_egress_uuid_(nil_uuid()) {
 }
 
 FlowExportInfo::FlowExportInfo(const FlowEntryPtr &fe) :
     flow_(fe), setup_time_(0), teardown_time_(0), last_modified_time_(0),
     bytes_(0), packets_(0), underlay_source_port_(0), changed_(true),
     tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0),
-    exported_atleast_once_(false) {
+    exported_atleast_once_(false), gen_id_(0),
+    flow_handle_(FlowEntry::kInvalidFlowHandle),
+    rev_flow_egress_uuid_(nil_uuid()) {
 }
 
 FlowExportInfo::FlowExportInfo(const FlowEntryPtr &fe, uint64_t setup_time) :
     flow_(fe), setup_time_(setup_time),
     teardown_time_(0), last_modified_time_(setup_time),
     bytes_(0), packets_(0), underlay_source_port_(0), changed_(true),
     tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0),
-    exported_atleast_once_(false) {
+    exported_atleast_once_(false), gen_id_(0),
+    flow_handle_(FlowEntry::kInvalidFlowHandle),
+    rev_flow_egress_uuid_(nil_uuid()) {
 }
 
 FlowEntry* FlowExportInfo::reverse_flow() const {
@@ -39,6 +45,22 @@ bool FlowExportInfo::IsActionLog() const {
     return false;
 }
 
+void FlowExportInfo::CopyFlowInfo(FlowEntry *fe) {
+    gen_id_ = fe->gen_id();
+    flow_handle_ = fe->flow_handle();
+    uuid_ = fe->uuid();
+    flags_ = fe->flags();
+    FlowEntry *rflow = reverse_flow();
+    if (rflow) {
+        rev_flow_egress_uuid_ = rflow->egress_uuid();
+    }
+}
+
+void FlowExportInfo::ResetStats() {
+    bytes_ = packets_ = 0;
+    tcp_flags_ = 0;
+    underlay_source_port_ = 0;
+}
 ///////////////////////////////////////////////////////////////////
 // APIs used only by UT
 //////////////////////////////////////////////////////////////////

src/vnsw/agent/vrouter/flow_stats/flow_export_info.h

@@ -50,6 +50,20 @@ class FlowExportInfo {
     uint64_t delete_enqueue_time() const { return delete_enqueue_time_; }
     void set_evict_enqueue_time(uint64_t value) { evict_enqueue_time_ = value; }
     uint64_t evict_enqueue_time() const { return evict_enqueue_time_; }
+    uint8_t gen_id() const { return gen_id_; }
+    void set_gen_id(uint8_t value) { gen_id_ = value; }
+    uint32_t flow_handle() const { return flow_handle_; }
+    void set_flow_handle(uint32_t value) { flow_handle_ = value; }
+    const boost::uuids::uuid &uuid() const { return uuid_; }
+    const boost::uuids::uuid &rev_flow_egress_uuid() const {
+        return rev_flow_egress_uuid_;
+    }
+    uint32_t flags() const { return flags_; }
+    bool is_flags_set(const FlowEntry::FlowEntryFlags &flags) const {
+        return (flags_ & flags);
+    }
+    void CopyFlowInfo(FlowEntry *fe);
+    void ResetStats();
 private:
     FlowEntryPtr flow_;
     uint64_t setup_time_;
@@ -67,6 +81,11 @@ class FlowExportInfo {
     uint64_t delete_enqueue_time_;
     uint64_t evict_enqueue_time_;
     bool exported_atleast_once_;
+    uint8_t gen_id_;
+    uint32_t flow_handle_;
+    boost::uuids::uuid uuid_;
+    boost::uuids::uuid rev_flow_egress_uuid_;
+    uint32_t flags_;
 };
 
 #endif //  __AGENT_FLOW_EXPORT_INFO_H__