* In case of ECMP resolve treat flow as update

1> In case of ECMP resolve, packet would be trapped for
   flow setup, in that scenario only component index should
   be changed.
2> When reverse flow gets reused due to flow movement treat
   the flow as update, instead of eviction.

(cherry picked from commit 58f71adb3b91d1efff3dec06ff72bc69c8a9d326)
Closes-bug:#1546876
Change-Id: I44fe8a66114c65442d630ef9dd79f2042c6586a7

src/vnsw/agent/pkt/flow_entry.cc

@@ -1719,7 +1719,7 @@ void FlowEntry::SetRemoteFlowEcmpIndex() {
     label = 0;
     boost::system::error_code ec;
     Ip4Address dest_ip = Ip4Address::from_string(peer_vrouter_, ec);
-    if (ec.value() == 0) {
+    if (ec.value() != 0) {
         return;
     }
 

src/vnsw/agent/pkt/flow_handler.cc

@@ -103,6 +103,7 @@ bool FlowHandler::Run() {
         pkt_info_->vrf = fe->data().vrf;
         pkt_info_->l3_forwarding = fe->l3_flow();
         info.l3_flow = fe->l3_flow();
+        info.out_component_nh_idx = fe->data().component_nh_idx;
     } else {
         info.l3_flow = pkt_info_->l3_forwarding = IsL3ModeFlow();
     }

src/vnsw/agent/pkt/flow_table.cc

@@ -157,28 +157,39 @@ void FlowTable::Add(FlowEntry *flow, FlowEntry *rflow) {
     uint64_t time = UTCTimestampUsec();
     FlowEntry *new_flow = Locate(flow, time);
     FlowEntry *new_rflow = (rflow != NULL) ? Locate(rflow, time) : NULL;
+
     FLOW_LOCK(new_flow, new_rflow);
-    AddInternal(flow, new_flow, rflow, new_rflow, false);
+    AddInternal(flow, new_flow, rflow, new_rflow, false, false);
 }
 
 void FlowTable::Update(FlowEntry *flow, FlowEntry *rflow) {
+    bool fwd_flow_update = true;
     FlowEntry *new_flow = Find(flow->key());
+
     FlowEntry *new_rflow = (rflow != NULL) ? Find(rflow->key()) : NULL;
+    bool rev_flow_update = true;
+    if (rflow && new_rflow == NULL) {
+        uint64_t time = UTCTimestampUsec();
+        new_rflow = Locate(rflow, time);
+        rev_flow_update = false;
+    }
+
     FLOW_LOCK(new_flow, new_rflow);
-    AddInternal(flow, new_flow, rflow, new_rflow, true);
+    AddInternal(flow, new_flow, rflow, new_rflow, fwd_flow_update,
+                rev_flow_update);
 }
 
 void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow,
                             FlowEntry *rflow_req, FlowEntry *rflow,
-                            bool update) {
+                            bool fwd_flow_update, bool rev_flow_update) {
     // The forward and reverse flow in request are linked. Unlink the flows
     // first. Flow table processing will link them if necessary
     flow_req->set_reverse_flow_entry(NULL);
     if (rflow_req)
         rflow_req->set_reverse_flow_entry(NULL);
 
     bool force_update_rflow = false;
-    if (update) {
+    if (fwd_flow_update) {
         if (flow == NULL)
             return;
 
@@ -195,13 +206,13 @@ void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow,
             // so trigger a force update instead of add for reverse flow
             force_update_rflow = true;
         }
-        Copy(flow, flow_req, update);
+        Copy(flow, flow_req, fwd_flow_update);
         flow->set_deleted(false);
     }
 
     if (rflow) {
         if (rflow_req != rflow) {
-            Copy(rflow, rflow_req, (update || force_update_rflow));
+            Copy(rflow, rflow_req, (rev_flow_update || force_update_rflow));
             // if the reverse flow was marked delete, reset its flow handle
             // to invalid index to assure it is attempted to reprogram using
             // kInvalidFlowHandle, this also ensures that flow entry wont
@@ -252,11 +263,11 @@ void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow,
     // While the scenario above cannot be totally avoided, programming reverse
     // flow first will reduce the probability
     if (rflow) {
-        UpdateKSync(rflow, (update || force_update_rflow));
+        UpdateKSync(rflow, (rev_flow_update || force_update_rflow));
         AddFlowInfo(rflow);
     }
 
-    UpdateKSync(flow, update);
+    UpdateKSync(flow, fwd_flow_update);
     AddFlowInfo(flow);
 }
 

src/vnsw/agent/pkt/flow_table.h

@@ -254,7 +254,10 @@ class FlowTable {
 
     void UpdateUnLocked(FlowEntry *flow, FlowEntry *rflow);
     void AddInternal(FlowEntry *flow, FlowEntry *new_flow, FlowEntry *rflow,
-                     FlowEntry *new_rflow, bool update);
+                     FlowEntry *new_rflow, bool fwd_flow_update,
+                     bool rev_flow_update);
+    void Add(FlowEntry *flow, FlowEntry *new_flow, FlowEntry *rflow,
+             FlowEntry *new_rflow, bool fwd_flow_update, bool rev_flow_update);
     void GetMutexSeq(tbb::mutex &mutex1, tbb::mutex &mutex2,
                      tbb::mutex **mutex_ptr_1, tbb::mutex **mutex_ptr_2);
     void EvictFlow(FlowEntry *flow, FlowEntry *rflow);

src/vnsw/agent/pkt/pkt_flow_info.cc

@@ -212,6 +212,12 @@ static bool NhDecode(const NextHop *nh, const PktInfo *pkt, PktFlowInfo *info,
             info->ecmp = true;
             const CompositeNH *comp_nh = static_cast<const CompositeNH *>(nh);
 
+            if (pkt->type == PktType::MESSAGE &&
+                info->out_component_nh_idx ==
+                CompositeNH::kInvalidComponentNHIdx) {
+                info->out_component_nh_idx = 0;
+            }
+
             // Compute out_component_nh_idx if,
             // 1. out_compoenent_nh_idx was set, but points to a deleted NH
             //    This can happen if flow is trapped for ECMP resolution from
@@ -1594,8 +1600,9 @@ void PktFlowInfo::Add(const PktInfo *pkt, PktControlInfo *in,
         UpdateEvictedFlowStats(pkt);
     }
 
-    if (pkt->type == PktType::MESSAGE &&
-        pkt->agent_hdr.cmd == AgentHdr::TRAP_FLOW_MISS) {
+    if ((pkt->type == PktType::MESSAGE &&
+        pkt->agent_hdr.cmd == AgentHdr::TRAP_FLOW_MISS) ||
+        pkt->agent_hdr.cmd == AgentHdr::TRAP_ECMP_RESOLVE) {
         update = true;
     }
 
@@ -1647,7 +1654,7 @@ void PktFlowInfo::Add(const PktInfo *pkt, PktControlInfo *in,
 
     bool swap_flows = false;
     // If this is message processing, then retain forward and reverse flows
-    if (pkt->type == PktType::MESSAGE &&
+    if (pkt->type == PktType::MESSAGE && !short_flow &&
         flow_entry->is_flags_set(FlowEntry::ReverseFlow)) {
         // for cases where we need to swap flows rflow should always
         // be Non-NULL

src/vnsw/agent/pkt/pkt_flow_info.h

@@ -79,8 +79,6 @@ class PktFlowInfo {
     void Add(const PktInfo *pkt, PktControlInfo *in,
              PktControlInfo *out);
     bool Process(const PktInfo *pkt, PktControlInfo *in, PktControlInfo *out);
-    void SetEcmpFlowInfo(const PktInfo *pkt, const PktControlInfo *in,
-                         const PktControlInfo *out);
     static bool GetIngressNwPolicyAclList(const Interface *intf,
                                           const VnEntry *vn,
                                           MatchPolicy *m_policy);

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

@@ -102,19 +102,19 @@ class EcmpTest : public ::testing::Test {
 
         //Add couple of remote VM routes for generating packet
         Inet4TunnelRouteAdd(bgp_peer, "vrf2", remote_vm_ip1_, 32,
-                            remote_server_ip_, TunnelType::AllType(),
+                            remote_server_ip_, TunnelType::GREType(),
                             30, "vn2", SecurityGroupList(),
                             PathPreference());
 
         Inet4TunnelRouteAdd(bgp_peer, "default-project:vn3:vn3",
                             remote_vm_ip2_, 32,
-                            remote_server_ip_, TunnelType::AllType(),
+                            remote_server_ip_, TunnelType::GREType(),
                             30, "default-project:vn3", SecurityGroupList(),
                             PathPreference());
 
         Inet4TunnelRouteAdd(bgp_peer, "default-project:vn4:vn4",
                             remote_vm_ip3_, 32,
-                            remote_server_ip_, TunnelType::AllType(),
+                            remote_server_ip_, TunnelType::GREType(),
                             30, "default-project:vn4", SecurityGroupList(),
                             PathPreference());
         client->WaitForIdle();
@@ -184,7 +184,7 @@ class EcmpTest : public ::testing::Test {
                         label, Agent::GetInstance()->fabric_vrf_name(),
                         Agent::GetInstance()->router_id(),
                         Ip4Address(remote_server_ip++),
-                        false, TunnelType::AllType()));
+                        false, TunnelType::GREType()));
             comp_nh_list.push_back(comp_nh);
             if (!same_label) {
                 label++;
@@ -207,6 +207,7 @@ class EcmpTest : public ::testing::Test {
                     VxLanTable::kInvalidvxlan_id, false, vn_list,
                     InterfaceNHFlags::INET4, SecurityGroupList(),
                     PathPreference(), ControllerPeerPath::kInvalidPeerIdentifier,
+                    EcmpLoadBalance(),
                     NULL);
         InetUnicastAgentRouteTable *rt_table =
             agent_->vrf_table()->GetInet4UnicastRouteTable(vrf_name);
@@ -224,7 +225,7 @@ class EcmpTest : public ::testing::Test {
         ControllerVmRoute *data =
             ControllerVmRoute::MakeControllerVmRoute(bgp_peer,
                                agent_->fabric_vrf_name(), agent_->router_id(),
-                               vrf_name, addr, TunnelType::AllType(), 16,
+                               vrf_name, addr, TunnelType::GREType(), 16,
                                vn_list, SecurityGroupList(),
                                PathPreference(), false, EcmpLoadBalance());
         InetUnicastAgentRouteTable::AddRemoteVmRouteReq(bgp_peer,
@@ -431,19 +432,19 @@ TEST_F(EcmpTest, EcmpTest_8) {
     ComponentNHKeyPtr comp_nh_data1(new ComponentNHKey(
         16, Agent::GetInstance()->fabric_vrf_name(),
         Agent::GetInstance()->router_id(), server_ip1, false,
-        TunnelType::AllType()));
+        TunnelType::GREType()));
     comp_nh.push_back(comp_nh_data1);
 
     ComponentNHKeyPtr comp_nh_data2(new ComponentNHKey(
         17, Agent::GetInstance()->fabric_vrf_name(),
         Agent::GetInstance()->router_id(),
-        server_ip2, false, TunnelType::AllType()));
+        server_ip2, false, TunnelType::GREType()));
     comp_nh.push_back(comp_nh_data2);
 
     ComponentNHKeyPtr comp_nh_data3(new ComponentNHKey(
         18, Agent::GetInstance()->fabric_vrf_name(),
         Agent::GetInstance()->router_id(),
-        server_ip3, false, TunnelType::AllType()));
+        server_ip3, false, TunnelType::GREType()));
     comp_nh.push_back(comp_nh_data3);
 
     SecurityGroupList sg_list;
@@ -549,7 +550,6 @@ TEST_F(EcmpTest, EcmpTest_10) {
 
     FlowEntry *entry = FlowGet(VrfGet("vrf2")->vrf_id(),
             "2.1.1.1", "9.1.1.1", 1, 0, 0, GetFlowKeyNH(2));
-    FlowEntry *old_entry = entry;
     EXPECT_TRUE(entry != NULL);
     EXPECT_TRUE(entry->data().component_nh_idx ==
             CompositeNH::kInvalidComponentNHIdx);
@@ -573,10 +573,6 @@ TEST_F(EcmpTest, EcmpTest_10) {
     EXPECT_TRUE(entry->data().component_nh_idx ==
                 CompositeNH::kInvalidComponentNHIdx);
     EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false);
-    //Old flow and new flow have same key, hence old flow should become
-    //short flow
-    EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true);
-    old_entry = entry;
 
     //Reverse flow is no ECMP
     rev_entry = entry->reverse_flow_entry();
@@ -596,9 +592,6 @@ TEST_F(EcmpTest, EcmpTest_10) {
     EXPECT_TRUE(entry->data().component_nh_idx ==
                 CompositeNH::kInvalidComponentNHIdx);
     EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false);
-    //Old flow and new flow have same key, hence old flow should become
-    //short flow
-    EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true);
 
     //Reverse flow is no ECMP
     rev_entry = entry->reverse_flow_entry();
@@ -667,7 +660,6 @@ TEST_F(EcmpTest, EcmpTest_11) {
     EXPECT_TRUE(nh->GetInterface()->name() == "vnet2");
     EXPECT_TRUE(rev_entry->is_flags_set(FlowEntry::ShortFlow) == false);
 
-    FlowEntry *old_entry = entry;
     TxIpPacket(VmPortGetId(3), "2.1.1.1", "9.1.1.1", 1);
     client->WaitForIdle();
     entry = FlowGet(VrfGet("vrf2")->vrf_id(),
@@ -676,9 +668,6 @@ TEST_F(EcmpTest, EcmpTest_11) {
     EXPECT_TRUE(entry->data().component_nh_idx ==
                 CompositeNH::kInvalidComponentNHIdx);
     EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false);
-    //Old entry becomes short flow since key are same
-    EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true);
-    old_entry = entry;
 
     //Reverse flow is no ECMP
     rev_entry = entry->reverse_flow_entry();
@@ -698,8 +687,6 @@ TEST_F(EcmpTest, EcmpTest_11) {
     EXPECT_TRUE(entry->data().component_nh_idx ==
                 CompositeNH::kInvalidComponentNHIdx);
     EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false);
-    //Old entry becomes short flow since key are same
-    EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true);
 
     //Reverse flow is no ECMP
     rev_entry = entry->reverse_flow_entry();
@@ -1201,7 +1188,7 @@ TEST_F(EcmpTest, EcmpReEval_2) {
     Ip4Address remote_vm_ip = Ip4Address::from_string("3.1.1.10");
     Ip4Address remote_server_ip = Ip4Address::from_string("10.10.10.10");
     Inet4TunnelRouteAdd(bgp_peer, "vrf2",remote_vm_ip, 32, 
-                        remote_server_ip, TunnelType::AllType(), 16, "vn2",
+                        remote_server_ip, TunnelType::GREType(), 16, "vn2",
                         SecurityGroupList(), PathPreference());
     client->WaitForIdle();
     TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1);
@@ -1219,7 +1206,7 @@ TEST_F(EcmpTest, EcmpReEval_2) {
     client->WaitForIdle();
 
     //Enqueue a re-evaluate request
-    TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1);
+    TxIpPacketEcmp(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1);
     client->WaitForIdle();
     entry = FlowGet(VrfGet("vrf2")->vrf_id(),
             "1.1.1.1", "3.1.1.10", 1, 0, 0, GetFlowKeyNH(1));
@@ -1269,7 +1256,7 @@ TEST_F(EcmpTest, EcmpReEval_3) {
     client->WaitForIdle();
 
     //Enqueue a re-evaluate request
-    TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1);
+    TxIpPacketEcmp(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1);
     client->WaitForIdle();
     entry = FlowGet(VrfGet("vrf2")->vrf_id(), "1.1.1.1", "3.1.1.10", 1, 0, 0,
                     GetFlowKeyNH(1));
@@ -1476,7 +1463,7 @@ TEST_F(EcmpTest, ServiceVlanTest_3) {
         std::string vn_name_10("vn10");
         std::string vn_name_11("vn11");
         EXPECT_TRUE(VnMatch(entry->data().source_vn_list, vn_name_10));
-        EXPECT_TRUE(VnMatch(entry->data().dest_vn _list, vn_name_11));
+        EXPECT_TRUE(VnMatch(entry->data().dest_vn_list, vn_name_11));
 
         //Reverse flow is no ECMP
         FlowEntry *rev_entry = entry->reverse_flow_entry();
@@ -1493,7 +1480,7 @@ TEST_F(EcmpTest, ServiceVlanTest_3) {
         }
         EXPECT_TRUE(rev_entry->data().dest_vrf == vrf_id);
         EXPECT_TRUE(VnMatch(rev_entry->data().source_vn_list, vn_name_11));
-        EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn _list, vn_name_10));
+        EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn_list, vn_name_10));
         sport++;
         dport++;
     }
@@ -1617,7 +1604,7 @@ TEST_F(EcmpTest, ServiceVlanTest_4) {
         std::string vn_name_10("vn10");
         std::string vn_name_11("vn11");
         EXPECT_TRUE(VnMatch(entry->data().source_vn_list, vn_name_10));
-        EXPECT_TRUE(VnMatch(entry->data().dest_vn _list, vn_name_11));
+        EXPECT_TRUE(VnMatch(entry->data().dest_vn_list, vn_name_11));
 
         //Reverse flow is no ECMP
         FlowEntry *rev_entry = entry->reverse_flow_entry();
@@ -1628,7 +1615,7 @@ TEST_F(EcmpTest, ServiceVlanTest_4) {
         EXPECT_TRUE(rev_entry->data().vrf == service_vrf_id);
         EXPECT_TRUE(rev_entry->data().dest_vrf == vrf_id);
         EXPECT_TRUE(VnMatch(rev_entry->data().source_vn_list, vn_name_11));
-        EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn _list, vn_name_10));
+        EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn_list, vn_name_10));
         sport++;
         dport++;
     }