Skip to content

Commit b92c3d4

Browse files
rupesh-krupesh-k
committed
Addressed review comments.
1 parent ea9a7bf commit b92c3d4

8 files changed

Lines changed: 939 additions & 1384 deletions

File tree

‎orchagent/mirrororch.cpp‎

Lines changed: 100 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,76 @@ bool MirrorOrch::decreaseRefCount(const string& name)
265265
return true;
266266
}
267267

268+
bool MirrorOrch::validateDstPort(const string& dstPort)
269+
{
270+
Port port;
271+
if (!m_portsOrch->getPort(dstPort, port) && port.m_type != Port::PHY)
272+
{
273+
SWSS_LOG_ERROR("Failed to locate port %s", dstPort.c_str());
274+
return false;
275+
}
276+
return true;
277+
}
278+
279+
bool MirrorOrch::checkPortExistsInSrcPortList(const string& port, const string& srcPort)
280+
{
281+
auto ports = tokenize(srcPort, ',');
282+
if (ports.size() != 0)
283+
{
284+
for (auto alias : ports)
285+
{
286+
if(port == alias)
287+
{
288+
return true;
289+
}
290+
}
291+
}
292+
293+
return false;
294+
}
295+
296+
bool MirrorOrch::validateSrcPort(const string& srcPort)
297+
{
298+
auto ports = tokenize(srcPort, ',');
299+
300+
if (ports.size() != 0)
301+
{
302+
for (auto alias : ports)
303+
{
304+
Port port;
305+
if (!gPortsOrch->getPort(alias, port))
306+
{
307+
SWSS_LOG_ERROR("Failed to locate Port/LAG %s", alias.c_str());
308+
return false;
309+
}
310+
311+
if(!(port.m_type == Port::PHY || port.m_type == Port::LAG))
312+
{
313+
SWSS_LOG_ERROR("Not supported port %s", alias.c_str());
314+
return false;
315+
}
316+
317+
// Check if the ports in LAG are part of source port list
318+
if (port.m_type == Port::LAG)
319+
{
320+
vector<Port> portv;
321+
m_portsOrch->getLagMember(port, portv);
322+
for (const auto p : portv)
323+
{
324+
if (checkPortExistsInSrcPortList(p.m_alias, srcPort))
325+
{
326+
SWSS_LOG_ERROR("Port %s in LAG %s is also part of src_port config %s",
327+
p.m_alias.c_str(), port.m_alias.c_str(), srcPort.c_str());
328+
return false;
329+
}
330+
}
331+
}
332+
}
333+
}
334+
335+
return true;
336+
}
337+
268338
void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& data)
269339
{
270340
SWSS_LOG_ENTER();
@@ -331,36 +401,26 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
331401
}
332402
else if (fvField(i) == MIRROR_SESSION_SRC_PORT)
333403
{
334-
auto ports = tokenize(fvValue(i), ',');
335-
if (ports.size() != 0)
404+
if (!validateSrcPort(fvValue(i)))
336405
{
337-
for (auto alias : ports)
338-
{
339-
Port port;
340-
if (!gPortsOrch->getPort(alias, port))
341-
{
342-
SWSS_LOG_ERROR("Failed to locate port/LAG %s", alias.c_str());
343-
return;
344-
}
345-
}
406+
SWSS_LOG_ERROR("Failed to get valid source port list %s", fvValue(i).c_str());
407+
return;
346408
}
347409
entry.src_port = fvValue(i);
348410
}
349411
else if (fvField(i) == MIRROR_SESSION_DST_PORT)
350412
{
351-
Port dst_port;
352-
if (!m_portsOrch->getPort(fvValue(i), dst_port))
413+
if (!validateDstPort(fvValue(i)))
353414
{
354-
SWSS_LOG_ERROR("Failed to locate port %s", fvValue(i).c_str());
415+
SWSS_LOG_ERROR("Failed to get valid destination port %s", fvValue(i).c_str());
355416
return;
356417
}
357418
entry.dst_port = fvValue(i);
358-
359419
}
360420
else if (fvField(i) == MIRROR_SESSION_DIRECTION)
361421
{
362-
if (!(fvValue(i) == mirror_rx_direction || fvValue(i) == mirror_tx_direction
363-
|| fvValue(i) == mirror_both_direction))
422+
if (!(fvValue(i) == MIRROR_RX_DIRECTION || fvValue(i) == MIRROR_TX_DIRECTION
423+
|| fvValue(i) == MIRROR_BOTH_DIRECTION))
364424
{
365425
SWSS_LOG_ERROR("Failed to get valid direction %s", fvValue(i).c_str());
366426
return;
@@ -395,7 +455,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
395455

396456
setSessionState(key, entry);
397457

398-
if (entry.type == mirror_session_span && !entry.dst_port.empty())
458+
if (entry.type == MIRROR_SESSION_SPAN && !entry.dst_port.empty())
399459
{
400460
auto &session1 = m_syncdMirrors.find(key)->second;
401461
activateSession(key, session1);
@@ -430,7 +490,7 @@ task_process_status MirrorOrch::deleteEntry(const string& name)
430490

431491
if (session.status)
432492
{
433-
if (session.type != mirror_session_span)
493+
if (session.type != MIRROR_SESSION_SPAN)
434494
{
435495
m_routeOrch->detach(this, session.dstIp);
436496
}
@@ -700,8 +760,9 @@ bool MirrorOrch::setUnsetPortMirror(Port port,
700760
status = sai_port_api->set_port_attribute(p.m_port_id, &port_attr);
701761
if (status != SAI_STATUS_SUCCESS)
702762
{
703-
SWSS_LOG_ERROR("Failed to configure mirror session port %s: %s, status %d, sessionId %x\n",
704-
port.m_alias.c_str(), p.m_alias.c_str(), status, sessionId);
763+
SWSS_LOG_ERROR("Failed to configure %s session on port %s: %s, status %d, sessionId %x\n",
764+
ingress ? "RX" : "TX", port.m_alias.c_str(),
765+
p.m_alias.c_str(), status, sessionId);
705766
return false;
706767
}
707768
}
@@ -711,8 +772,8 @@ bool MirrorOrch::setUnsetPortMirror(Port port,
711772
status = sai_port_api->set_port_attribute(port.m_port_id, &port_attr);
712773
if (status != SAI_STATUS_SUCCESS)
713774
{
714-
SWSS_LOG_ERROR("Failed to configure mirror session to port %s, status %d, sessionId %x\n",
715-
port.m_alias.c_str(), status, sessionId);
775+
SWSS_LOG_ERROR("Failed to configure %s session on port %s, status %d, sessionId %x\n",
776+
ingress ? "RX" : "TX", port.m_alias.c_str(), status, sessionId);
716777
return false;
717778
}
718779
}
@@ -732,15 +793,23 @@ bool MirrorOrch::configurePortMirrorSession(const string& name, MirrorEntry& ses
732793
SWSS_LOG_ERROR("Failed to locate port/LAG %s", alias.c_str());
733794
return false;
734795
}
735-
if (session.direction == mirror_rx_direction || session.direction == mirror_both_direction)
796+
if (session.direction == MIRROR_RX_DIRECTION || session.direction == MIRROR_BOTH_DIRECTION)
736797
{
737798
if (!setUnsetPortMirror(port, true, set, session.sessionId))
799+
{
800+
SWSS_LOG_ERROR("Failed to configure mirror session %s port %s\n",
801+
name.c_str(), port.m_alias.c_str());
738802
return false;
803+
}
739804
}
740-
if (session.direction == mirror_tx_direction || session.direction == mirror_both_direction)
805+
if (session.direction == MIRROR_TX_DIRECTION || session.direction == MIRROR_BOTH_DIRECTION)
741806
{
742807
if (!setUnsetPortMirror(port, false, set, session.sessionId))
808+
{
809+
SWSS_LOG_ERROR("Failed to configure mirror session %s port %s \n",
810+
name.c_str(), port.m_alias.c_str());
743811
return false;
812+
}
744813
}
745814
}
746815
}
@@ -767,7 +836,7 @@ bool MirrorOrch::activateSession(const string& name, MirrorEntry& session)
767836
attrs.push_back(attr);
768837
}
769838

770-
if (session.type == mirror_session_span)
839+
if (session.type == MIRROR_SESSION_SPAN)
771840
{
772841
Port dst_port;
773842
if (!m_portsOrch->getPort(session.dst_port, dst_port))
@@ -1225,16 +1294,18 @@ void MirrorOrch::updateLagMember(const LagMemberUpdate& update)
12251294
// Check the following conditions:
12261295
// 1) Session is active
12271296
// 2) LAG is part of mirror session source ports.
1297+
// 3) Member port is not part of session source ports.
12281298
// if the above condition matches then set/unset mirror configuration to new member port.
12291299
if (session.status &&
12301300
!session.src_port.empty() &&
1231-
session.src_port.find(update.lag.m_alias.c_str()) != std::string::npos)
1301+
session.src_port.find(update.lag.m_alias.c_str()) != std::string::npos &&
1302+
!checkPortExistsInSrcPortList(update.member.m_alias, session.src_port))
12321303
{
1233-
if (session.direction == mirror_rx_direction || session.direction == mirror_both_direction)
1304+
if (session.direction == MIRROR_RX_DIRECTION || session.direction == MIRROR_BOTH_DIRECTION)
12341305
{
12351306
setUnsetPortMirror(update.member, true, update.add, session.sessionId);
12361307
}
1237-
if (session.direction == mirror_tx_direction || session.direction == mirror_both_direction)
1308+
if (session.direction == MIRROR_TX_DIRECTION || session.direction == MIRROR_BOTH_DIRECTION)
12381309
{
12391310
setUnsetPortMirror(update.member, false, update.add, session.sessionId);
12401311
}

‎orchagent/mirrororch.h‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,11 @@
1818
#include <map>
1919
#include <inttypes.h>
2020

21-
const string mirror_rx_direction = "RX";
22-
const string mirror_tx_direction = "TX";
23-
const string mirror_both_direction = "BOTH";
24-
const string mirror_session_span = "SPAN";
25-
const string mirror_session_erspan = "ERSPAN";
21+
#define MIRROR_RX_DIRECTION "RX"
22+
#define MIRROR_TX_DIRECTION "TX"
23+
#define MIRROR_BOTH_DIRECTION "BOTH"
24+
#define MIRROR_SESSION_SPAN "SPAN"
25+
#define MIRROR_SESSION_ERSPAN "ERSPAN"
2626

2727
/*
2828
* Contains session data specified by user in config file
@@ -128,6 +128,9 @@ class MirrorOrch : public Orch, public Observer, public Subject
128128
void updateLagMember(const LagMemberUpdate&);
129129
void updateVlanMember(const VlanMemberUpdate&);
130130

131+
bool checkPortExistsInSrcPortList(const string& port, const string& srcPort);
132+
bool validateSrcPort(const string& srcPort);
133+
bool validateDstPort(const string& dstPort);
131134
bool setUnsetPortMirror(Port port, bool ingress, bool set,
132135
sai_object_id_t sessionId);
133136
bool configurePortMirrorSession(const string&, MirrorEntry&, bool enable);

‎tests/conftest.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
from dvslib import dvs_acl
1818
from dvslib import dvs_vlan
1919
from dvslib import dvs_lag
20+
from dvslib import dvs_mirror
21+
from dvslib import dvs_policer
2022

2123
def ensure_system(cmd):
2224
(rc, output) = commands.getstatusoutput(cmd)
@@ -816,6 +818,26 @@ def remove_neighbor(self, interface, ip):
816818
tbl._del(interface + ":" + ip)
817819
time.sleep(1)
818820

821+
def add_route(self, prefix, nexthop):
822+
self.runcmd("ip route add " + prefix + " via " + nexthop)
823+
time.sleep(1)
824+
825+
def remove_route(self, prefix):
826+
self.runcmd("ip route del " + prefix)
827+
time.sleep(1)
828+
829+
def create_fdb(self, vlan, mac, interface):
830+
tbl = swsscommon.ProducerStateTable(self.pdb, "FDB_TABLE")
831+
fvs = swsscommon.FieldValuePairs([("port", interface),
832+
("type", "dynamic")])
833+
tbl.set("Vlan" + vlan + ":" + mac, fvs)
834+
time.sleep(1)
835+
836+
def remove_fdb(self, vlan, mac):
837+
tbl = swsscommon.ProducerStateTable(self.pdb, "FDB_TABLE")
838+
tbl._del("Vlan" + vlan + ":" + mac)
839+
time.sleep(1)
840+
819841
def setup_db(self):
820842
self.pdb = swsscommon.DBConnector(0, self.redis_sock, 0)
821843
self.adb = swsscommon.DBConnector(1, self.redis_sock, 0)
@@ -1023,6 +1045,17 @@ def dvs_vlan_manager(request, dvs):
10231045
dvs.get_state_db(),
10241046
dvs.get_counters_db(),
10251047
dvs.get_app_db())
1048+
@pytest.yield_fixture(scope="class")
1049+
def dvs_mirror_manager(request, dvs):
1050+
request.cls.dvs_mirror = dvs_mirror.DVSMirror(dvs.get_asic_db(),
1051+
dvs.get_config_db(),
1052+
dvs.get_state_db(),
1053+
dvs.get_counters_db(),
1054+
dvs.get_app_db())
1055+
@pytest.yield_fixture(scope="class")
1056+
def dvs_policer_manager(request, dvs):
1057+
request.cls.dvs_policer = dvs_policer.DVSPolicer(dvs.get_asic_db(),
1058+
dvs.get_config_db())
10261059
##################### DPB fixtures ###########################################
10271060
def create_dpb_config_file(dvs):
10281061
cmd = "sonic-cfggen -j /etc/sonic/init_cfg.json -j /tmp/ports.json --print-data > /tmp/dpb_config_db.json"

‎tests/dvslib/dvs_acl.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,16 @@ def create_acl_rule(self, table_name, rule_name, qualifiers, action="FORWARD", p
136136

137137
self.config_db.create_entry("ACL_RULE", "{}|{}".format(table_name, rule_name), fvs)
138138

139+
def create_mirror_acl_rule(self, table_name, rule_name, qualifiers, priority="2020"):
140+
fvs = {
141+
"priority": priority
142+
}
143+
144+
for k, v in qualifiers.items():
145+
fvs[k] = v
146+
147+
self.config_db.create_entry("ACL_RULE", "{}|{}".format(table_name, rule_name), fvs)
148+
139149
def remove_acl_rule(self, table_name, rule_name):
140150
self.config_db.delete_entry("ACL_RULE", "{}|{}".format(table_name, rule_name))
141151

0 commit comments

Comments
 (0)