[DASH] ACL stage 1 implementation. #5
Conversation
oleksandrivantsiv
force-pushed
the
dash-acl
branch
4 times, most recently
from
86ec72a to
6d0c507
Compare
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| if (exists(group_id)) |
There was a problem hiding this comment.
Why do you want to return failed for duplicate creation. I believe we can add warn log and return success?
There was a problem hiding this comment.
Is there a possibility to get a request to create a group twice?
There was a problem hiding this comment.
We can check the group attributes and if they are the same return and warning instead of error. But I'm not sure that this can happen in the production. If duplicates are not possible the is no reason to additional logic to the code.
orchagent/dash/dashaclgroupmgr.cpp
Outdated
| for (const auto& stage: stages) | ||
| { | ||
| auto status = bind(new_group, *eni, DashAclDirection::IN, stage); | ||
| ABORT_IF_NOT(status == SAI_STATUS_SUCCESS, "Failed to bind new group to ENI"); |
There was a problem hiding this comment.
Why are we not using handleSaiStatus and use this abort_if_not API
There was a problem hiding this comment.
It is a good question. If an error happens in the middle of the ACL group refreshing during binding of the new group to the ENI this means that something is wrong with the system. We can't recover from a situation like that because it is unknown what to do. If the binding of the new group failed we are not sure in which state that stage of the ENI is left. Does the old group is still bound? If we try to bind the old group will it fail? If we recover the old group in which state is the binding of the other ENIs? Do we need to back up to the old group?
In this situation asserting is the right way to handle the error. Orchagent will crash and restart. The controller will detect this and really the configuration
There was a problem hiding this comment.
@dgsudharsan done
| } | ||
|
|
||
| auto& group = group_it->second; | ||
| if (isBound(group)) |
There was a problem hiding this comment.
Can you please add a comment explaining why you need to refresh ACL group only in bound scenario?
There was a problem hiding this comment.
This is explained in the HDL document (sonic-net/SONiC#1427). I prefer to not duplicate the information in two places.
There was a problem hiding this comment.
@dgsudharsan done
| auto& group = group_it->second; | ||
|
|
||
| auto acl_rule_it = group.m_dash_acl_rule_table.find(rule_id); | ||
| ABORT_IF_NOT(acl_rule_it == group.m_dash_acl_rule_table.end(), "Failed to create ACL rule %s. Rule already exist in ACL group %s", rule_id.c_str(), group_id.c_str()); |
There was a problem hiding this comment.
Should we abort for duplicate? Instead can't we thrown warn and return?
There was a problem hiding this comment.
createRule method should be called only if the rule doesn't exist. Otherwise, updateRule should be called. It is the responsibility of the called to check and call the right method. The assertion is to prevent coding errors. It should not happen in a normal run.
orchagent/orchdaemon.cpp
Outdated
| @@ -252,6 +252,7 @@ bool OrchDaemon::init() | |||
| gDirectory.set(dash_route_orch); | |||
|
|
|||
| vector<string> dash_acl_tables = { | |||
| "DASH_PREFIX_TAG_TABLE", | |||
There was a problem hiding this comment.
Why don't we define a macro just like other tables in swss common?
There was a problem hiding this comment.
I even created a PR for that in swss-common :)
Just forgot to replace the string
oleksandrivantsiv
force-pushed
the
dash
branch
from
12ad49c to
e6f134f
Compare
**What I did**
Fix the Mem Leak by moving the raw pointers in type_maps to use smart pointers
**Why I did it**
```
Indirect leak of 83776 byte(s) in 476 object(s) allocated from:
#0 0x7f0a2a414647 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
#1 0x5555590cc923 in __gnu_cxx::new_allocator, std::allocator > const, referenced_object> > >::allocate(unsigned long, void const*) /usr/include/c++/10/ext/new_allocator.h:115
#2 0x5555590cc923 in std::allocator_traits, std::allocator > const, referenced_object> > > >::allocate(std::allocator, std::allocator > const, referenced_object> > >&, unsigned long) /usr/include/c++/10/bits/alloc_traits.h:460
#3 0x5555590cc923 in std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_get_node() /usr/include/c++/10/bits/stl_tree.h:584
#4 0x5555590cc923 in std::_Rb_tree_node, std::allocator > const, referenced_object> >* std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_create_node, std::allocator > const&>, std::tuple<> >(std::piecewise_construct_t const&, std::tuple, std::allocator > const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:634
#5 0x5555590cc923 in std::_Rb_tree_iterator, std::allocator > const, referenced_object> > std::_Rb_tree, std::allocator >, std::pair, std::allocator > const, referenced_object>, std::_Select1st, std::allocator > const, referenced_object> >, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::_M_emplace_hint_unique, std::allocator > const&>, std::tuple<> >(std::_Rb_tree_const_iterator, std::allocator > const, referenced_object> >, std::piecewise_construct_t const&, std::tuple, std::allocator > const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:2461
#6 0x5555590e8757 in std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >::operator[](std::__cxx11::basic_string, std::allocator > const&) /usr/include/c++/10/bits/stl_map.h:501
#7 0x5555590d48b0 in Orch::setObjectReference(std::map, std::allocator >, std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >*, std::less, std::allocator > >, std::allocator, std::allocator > const, std::map, std::allocator >, referenced_object, std::less, std::allocator > >, std::allocator, std::allocator > const, referenced_object> > >*> > >&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&, std::__cxx11::basic_string, std::allocator > const&) orchagent/orch.cpp:450
sonic-net#8 0x5555594ff66b in QosOrch::handleQueueTable(Consumer&, std::tuple, std::allocator >, std::__cxx11::basic_string, std::allocator >, std::vector, std::allocator >, std::__cxx11::basic_string, std::allocator > >, std::allocator, std::allocator >, std::__cxx11::basic_string, std::allocator > > > > >&) orchagent/qosorch.cpp:1763
sonic-net#9 0x5555594edbd6 in QosOrch::doTask(Consumer&) orchagent/qosorch.cpp:2179
sonic-net#10 0x5555590c8743 in Consumer::drain() orchagent/orch.cpp:241
sonic-net#11 0x5555590c8743 in Consumer::drain() orchagent/orch.cpp:238
sonic-net#12 0x5555590c8743 in Consumer::execute() orchagent/orch.cpp:235
sonic-net#13 0x555559090dad in OrchDaemon::start() orchagent/orchdaemon.cpp:755
sonic-net#14 0x555558e9be25 in main orchagent/main.cpp:766
sonic-net#15 0x7f0a299b6d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09)
```
oleksandrivantsiv
force-pushed
the
dash-acl
branch
from
6d0c507 to
87130eb
Compare
oleksandrivantsiv
force-pushed
the
dash-acl
branch
from
87130eb to
4dae936
Compare
| } | ||
|
|
||
| task_process_status DashAclOrch::unbindAclFromEni(DashAclBindTable &acl_bind_table, const string &key) | ||
| task_process_status DashAclOrch::unbindAclFromEni(DashAclDirection direction, const std::string table_id) |
What I did
Why I did it
How I verified it
Details if related