VOL-2494: Fix UT on BAL3.2.3.2 integrated agent
Fix for FlowAdd, FlowRemove and RemoveTrafficScheduler UT's
Fixed data lock issue caused while installing and removing ACL
Bumped version to 2.1.5
Change-Id: Ie8526220de820df42b1ac9c936b0c0f6a6eb7d4f
diff --git a/VERSION b/VERSION
index 7d2ed7c..cd57a8b 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.1.4
+2.1.5
diff --git a/agent/src/core_utils.cc b/agent/src/core_utils.cc
index 9bcee03..32de09d 100644
--- a/agent/src/core_utils.cc
+++ b/agent/src/core_utils.cc
@@ -360,10 +360,11 @@
return result;
}
-// Gets free ACL ID if available, else -1
+/* ACL ID is a shared resource, caller of this function has to ensure atomicity using locks
+ Gets free ACL ID if available, else -1. */
int get_acl_id() {
int acl_id;
- bcmos_fastlock_lock(&data_lock);
+
/* Complexity of O(n). Is there better way that can avoid linear search? */
for (acl_id = 0; acl_id < MAX_ACL_ID; acl_id++) {
if (acl_id_bitset[acl_id] == 0) {
@@ -371,7 +372,6 @@
break;
}
}
- bcmos_fastlock_unlock(&data_lock, 0);
if (acl_id < MAX_ACL_ID) {
return acl_id ;
} else {
@@ -379,12 +379,11 @@
}
}
-// Frees up the ACL ID.
+/* ACL ID is a shared resource, caller of this function has to ensure atomicity using locks
+ Frees up the ACL ID. */
void free_acl_id (int acl_id) {
if (acl_id < MAX_ACL_ID) {
- bcmos_fastlock_lock(&data_lock);
acl_id_bitset[acl_id] = 0;
- bcmos_fastlock_unlock(&data_lock, 0);
}
}
@@ -922,7 +921,6 @@
}
Status install_acl(const acl_classifier_key acl_key) {
-
bcmos_errno err;
bcmolt_access_control_cfg cfg;
bcmolt_access_control_key key = { };
@@ -934,14 +932,12 @@
if (acl_id < 0) {
OPENOLT_LOG(ERROR, openolt_log_id, "exhausted acl_id for eth_type = %d, ip_proto = %d, src_port = %d, dst_port = %d\n",
acl_key.ether_type, acl_key.ip_proto, acl_key.src_port, acl_key.dst_port);
- bcmos_fastlock_unlock(&data_lock, 0);
return bcm_to_grpc_err(BCM_ERR_INTERNAL, "exhausted acl id");
}
key.id = acl_id;
/* config access control instance */
BCMOLT_CFG_INIT(&cfg, access_control, key);
-
if (acl_key.ether_type > 0) {
OPENOLT_LOG(DEBUG, openolt_log_id, "Access_Control classify ether_type 0x%04x\n", acl_key.ether_type);
BCMOLT_FIELD_SET(&c_val, classifier, ether_type, acl_key.ether_type);
diff --git a/agent/test/src/test_core.cc b/agent/test/src/test_core.cc
index 5082fac..b08b92e 100644
--- a/agent/test/src/test_core.cc
+++ b/agent/test/src/test_core.cc
@@ -1160,8 +1160,6 @@
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
-// TODO: VOL-2494
-#if 0
////////////////////////////////////////////////////////////////////////////
// For testing FlowAdd functionality
////////////////////////////////////////////////////////////////////////////
@@ -1179,6 +1177,7 @@
int32_t gemport_id = 1024;
int32_t priority_value = 0;
uint64_t cookie = 0;
+ int32_t group_id = -1;
NiceMock<BalMocker> balMock;
openolt::Flow* flow;
@@ -1304,19 +1303,23 @@
bcmos_errno olt_cfg_set_res = BCM_ERR_OK;
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type,
+ alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
+#ifdef FLOW_CHECKER
// Test 2 - FlowAdd - Duplicate Flow case
TEST_F(TestFlowAdd, FlowAddHsiaFixedQueueUpstreamDuplicate) {
bcmos_errno flow_cfg_get_stub_res = BCM_ERR_OK;
EXPECT_GLOBAL_CALL(bcmolt_cfg_get__flow_stub, bcmolt_cfg_get__flow_stub(_, _))
.WillRepeatedly(DoAll(SetArg1ToBcmOltFlowCfg(flow_cfg), Return(flow_cfg_get_stub_res)));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
+#endif
// Test 3 - FlowAdd - Failure case(bcmolt_cfg_set returns error)
TEST_F(TestFlowAdd, FlowAddHsiaFixedQueueUpstreamFailure) {
@@ -1329,7 +1332,8 @@
.WillRepeatedly(DoAll(SetArg1ToBcmOltFlowCfg(flow_cfg), Return(flow_cfg_get_stub_res)));
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
@@ -1337,7 +1341,8 @@
TEST_F(TestFlowAdd, FlowAddFailureInvalidFlowDirection) {
flow_type = "bidirectional";
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
@@ -1345,7 +1350,8 @@
TEST_F(TestFlowAdd, FlowAddFailureInvalidNWCfg) {
network_intf_id = -1;
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
@@ -1354,7 +1360,8 @@
flow_id = 2;
classifier->set_eth_type(34958);
- cmd->set_add_outer_tag(false);
+ action = new openolt::Action;
+ cmd = new openolt::ActionCmd;
cmd->set_trap_to_host(true);
action->set_allocated_cmd(cmd);
@@ -1364,7 +1371,8 @@
.WillRepeatedly(DoAll(SetArg1ToBcmOltFlowCfg(flow_cfg), Return(flow_cfg_get_stub_res)));
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id,
+ network_intf_id, gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
@@ -1376,7 +1384,8 @@
classifier->set_ip_proto(17);
classifier->set_src_port(68);
classifier->set_dst_port(67);
- cmd->set_add_outer_tag(false);
+ action = new openolt::Action;
+ cmd = new openolt::ActionCmd;
cmd->set_trap_to_host(true);
action->set_allocated_cmd(cmd);
@@ -1386,7 +1395,8 @@
.WillRepeatedly(DoAll(SetArg1ToBcmOltFlowCfg(flow_cfg), Return(flow_cfg_get_stub_res)));
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
@@ -1398,8 +1408,9 @@
classifier->set_o_vid(12);
classifier->set_i_vid(7);
classifier->set_pkt_tag_type("double_tag");
+ action = new openolt::Action;
+ cmd = new openolt::ActionCmd;
action->set_o_vid(0);
- cmd->set_add_outer_tag(false);
cmd->set_remove_outer_tag(true);
action->set_allocated_cmd(cmd);
@@ -1409,7 +1420,8 @@
.WillRepeatedly(DoAll(SetArg1ToBcmOltFlowCfg(flow_cfg), Return(flow_cfg_get_stub_res)));
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
@@ -1429,7 +1441,8 @@
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
CreateTrafficQueues_(traffic_queues);
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
@@ -1443,8 +1456,9 @@
classifier->set_o_vid(12);
classifier->set_i_vid(7);
classifier->set_pkt_tag_type("double_tag");
+ action = new openolt::Action;
+ cmd = new openolt::ActionCmd;
action->set_o_vid(0);
- cmd->set_add_outer_tag(false);
cmd->set_remove_outer_tag(true);
action->set_allocated_cmd(cmd);
@@ -1458,10 +1472,11 @@
ON_CALL(balMock, bcmolt_cfg_set(_, _)).WillByDefault(Return(olt_cfg_set_res));
CreateTrafficQueues_(traffic_queues);
- Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id, gemport_id, *classifier, *action, priority_value, cookie);
+ Status status = FlowAdd_(access_intf_id, onu_id, uni_id, port_no, flow_id, flow_type, alloc_id, network_intf_id,
+ gemport_id, *classifier, *action, priority_value, cookie, group_id);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
-#endif
+
////////////////////////////////////////////////////////////////////////////
// For testing OnuPacketOut functionality
@@ -1502,8 +1517,6 @@
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
-// TODO: VOL-2494
-#if 0
// Test 3 - OnuPacketOut success, Finding Flow ID from port no and Gem from Flow ID case
TEST_F(TestOnuPacketOut, OnuPacketOutFindGemFromFlowSuccess) {
uint32_t port_no = 16;
@@ -1515,7 +1528,6 @@
Status status = OnuPacketOut_(pon_id, onu_id, port_no, gemport_id, pkt);
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
-#endif
// Test 4 - OnuPacketOut success, Failure in finding Gem port case
TEST_F(TestOnuPacketOut, OnuPacketOutFindGemFromFlowFailure) {
@@ -1529,8 +1541,6 @@
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
-// TODO: VOL-2494
-#if 0
////////////////////////////////////////////////////////////////////////////
// For testing FlowRemove functionality
////////////////////////////////////////////////////////////////////////////
@@ -1655,7 +1665,6 @@
Status status = UplinkPacketOut_(pon_id, pkt);
ASSERT_TRUE( status.error_message() != Status::OK.error_message() );
}
-#endif
////////////////////////////////////////////////////////////////////////////
// For testing CreateTrafficSchedulers functionality
@@ -2084,6 +2093,9 @@
res.state = state;
res.status = status;
+ // We need to wait for some time to allow the Alloc Cfg Request to be triggered
+ // before we push the result.
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
bcmos_fastlock_lock(&alloc_cfg_wait_lock);
std::map<alloc_cfg_compltd_key, Queue<alloc_cfg_complete_result> *>::iterator it = alloc_cfg_compltd_map.find(k);
if (it == alloc_cfg_compltd_map.end()) {
@@ -2097,8 +2109,6 @@
}
};
-// TODO: VOL-2494
-#if 0
// Test 1 - RemoveTrafficSchedulers-Upstream success case
TEST_F(TestRemoveTrafficSchedulers, RemoveTrafficSchedulersUpstreamSuccess) {
traffic_sched->set_direction(tech_profile::Direction::UPSTREAM);
@@ -2122,7 +2132,6 @@
int res = push_alloc_cfg_complt.get();
ASSERT_TRUE( status.error_message() == Status::OK.error_message() );
}
-#endif
// Test 2 - RemoveTrafficSchedulers-Upstream success case(alloc object is not reset)
TEST_F(TestRemoveTrafficSchedulers, UpstreamAllocObjNotReset) {