fix dhcpv6 counters are not cleared on clear command #38
Conversation
src/relay.cpp
Outdated
| state_db->hset(counterVlan, "Relay-Forward", toString(counters[DHCPv6_MESSAGE_TYPE_RELAY_FORW])); | ||
| state_db->hset(counterVlan, "Relay-Reply", toString(counters[DHCPv6_MESSAGE_TYPE_RELAY_REPL])); | ||
| state_db->hset(counterVlan, "Malformed", toString(counters[DHCPv6_MESSAGE_TYPE_MALFORMED])); | ||
| void initialize_counter(std::shared_ptr<swss::DBConnector> state_db, std::string ifname) { |
There was a problem hiding this comment.
The logic here is if counter is not zero, keep original data.
If counter not exists, initialize it to 0.
So container restart will not clear counter, clear counter cli or sonic-db-cli can clear counters.
There was a problem hiding this comment.
A container restart is supposed to happen if there is a config change or config reload. And so i think its is okay to zero all the counters initially. and then the STATE_DB is reflective of the status of counters seen during the lifetime of the current process.
we anyway have dhcpmon which tracks and dumps counters periodically or when its terminated crct.
Also, what about the stale entries. i.e. if some VLAN is removed and we do a container restart the entries in STATE_DB would remain forever unless clear is called. During process init, i suggest clearing out the entire DHCPv6_COUNTER_TABLE table
src/relay.cpp
Outdated
| state_db->hset(counterVlan, "Relay-Forward", toString(counters[DHCPv6_MESSAGE_TYPE_RELAY_FORW])); | ||
| state_db->hset(counterVlan, "Relay-Reply", toString(counters[DHCPv6_MESSAGE_TYPE_RELAY_REPL])); | ||
| state_db->hset(counterVlan, "Malformed", toString(counters[DHCPv6_MESSAGE_TYPE_MALFORMED])); | ||
| void initialize_counter(std::shared_ptr<swss::DBConnector> state_db, std::string ifname) { |
There was a problem hiding this comment.
A container restart is supposed to happen if there is a config change or config reload. And so i think its is okay to zero all the counters initially. and then the STATE_DB is reflective of the status of counters seen during the lifetime of the current process.
we anyway have dhcpmon which tracks and dumps counters periodically or when its terminated crct.
Also, what about the stale entries. i.e. if some VLAN is removed and we do a container restart the entries in STATE_DB would remain forever unless clear is called. During process init, i suggest clearing out the entire DHCPv6_COUNTER_TABLE table
jcaiMR
force-pushed
the
dev/dhcprelay_counter_fix
branch
from
1a1bd6f to
1b4c76e
Compare
jcaiMR
force-pushed
the
dev/dhcprelay_counter_fix
branch
from
2426e3c to
de3f99f
Compare
| @@ -1089,8 +1075,7 @@ void loop_relay(std::unordered_map<std::string, relay_config> &vlans) { | |||
|
|
|||
| update_vlan_mapping(vlan.first, config_db); | |||
|
|
|||
| std::string counterVlan = counter_table; | |||
| initialize_counter(vlan.second.state_db, counterVlan.append(vlan.second.interface)); | |||
| initialize_counter(vlan.second.state_db, vlan.second.interface); | |||
There was a problem hiding this comment.
Changes look good, i also suggest clearing out the entire COUNTERS Table, during init. That'll fix this issue as well https://github.com/sonic-net/sonic-buildimage/issues/15047
There was a problem hiding this comment.
Looks like the issue is handled by sonic-net/sonic-utilities#2852, not required to clear DHCPv6_COUNTERS table here. #Resolved
Description of PR
Fix dhcpv6 counters are not cleared after "clear dhcp6relay_counters", or "sonic-db-cli STATE_DB hmset "DHCPv6_COUNTER_TABLE|Vlan1000" {} 0
Summary:
Fix sonic-net/sonic-buildimage#14447
dhcpv6 relay counter cached in original relay code, so even we clear counter in STATE_DB, next time when packets come from downlink or uplink ports, counter will be flushed with relay cached data + 1, so the counter flushed back to original data + 1.
Type of change
Back port request
Need in 202205 and 202012, need manually double commit because of code conflict.
Approach
What is the motivation for this PR?
Fix clear counter issue
How did you do it?
Remove cache in dhcp_relay code, instead we will read count from STATE_DB then refresh.
How did you verify/test it?
test_dhcpv6_relay.py::test_dhcpv6_relay_counter multiple times and with clear counter.
Any platform specific information?
Supported testbed topology if it's a new test case?