Repository navigation
[dhcpmon] Delay Counter Snapshot During Indeterminate State - #5509
tahmed-dev wants to merge 1 commit into
Conversation
This PR fixes a rare, however possible, condition when the DHCP RX packet do happen before dhcpmon timer kicks in and DHCP TX packets happens after. If DHCP remains silent for the next bucket duratrion, those TX packet will be snapshot without switching DHCP state to healthy. signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
pavel-shirshov
left a comment
pavel-shirshov
left a comment
There was a problem hiding this comment.
LGTM
Please wait for other reviewers
| * The idea here is to delay snapshoting those packet and compare them | ||
| * with next opportunity when dhcp packet are received. | ||
| */ | ||
| if (state_data[0].dhcp_status != DHCP_MON_STATUS_INDETERMINATE) { |
There was a problem hiding this comment.
Is it enough to check state_data[0].dhcp_status != DHCP_MON_STATUS_INDETERMINATE here? Should you also check that state_data[1].dhcp_status != DHCP_MON_STATUS_INDETERMINATE?
|
@tahmed-dev, wonder what is the status for this pr? |
@lguohan I'd like to hold off on this PR until I get more data from the current dhcpmon. The current dhcpmon will show more logs that will prove the theory above. Also, the current dhcpmon has bucket size of 18 sec which might indeed solve the current issue as 1 min is not an integral multiple of buckets anymore. This case is very slim and would happen if there is only one dhcp client that has its timer aligned with the monitor timer. |
| * The idea here is to delay snapshoting those packet and compare them | ||
| * with next opportunity when dhcp packet are received. | ||
| */ | ||
| if (state_data[0].dhcp_status != DHCP_MON_STATUS_INDETERMINATE) { |
There was a problem hiding this comment.
Retaining the TX snapshot can mask a later relay failure: RX=1/TX=0 is unhealthy and gets snapshotted; delayed TX=1 arrives during an indeterminate interval, so the snapshot stays (1,0). A later RX=2 with no corresponding TX is then classified healthy because the old TX exceeds the snapshot. Can we retain and reconcile the outstanding RX/TX window together instead of carrying stale TX credit into the next request?
There was a problem hiding this comment.
Thanks for looking at this. You're right: keeping the TX snapshot carries stale TX credit into the next request, so RX and TX need to be reconciled together. This PR is out of date, though. dhcpmon moved to sonic-net/sonic-dhcpmon in #14285, and src/dhcpmon no longer exists here, so I'm closing it. A fix for the split RX/TX case belongs in sonic-net/sonic-dhcpmon.
5 participants
This PR fixes a rare, however possible, condition when the DHCP RX
packet do happen before dhcpmon timer kicks in and DHCP TX packets
happens after. If DHCP remains silent for the next bucket duratrion,
those TX packet will be snapshot without switching DHCP state to
healthy.
signed-off-by: Tamer Ahmed tamer.ahmed@microsoft.com
- Why I did it
Fix issue were DHCP is reported unhealthy due to perfect packet split of rx happening before check timer kicks in and tx happens after timer is serviced.
- How I did it
Do not update snapshot counters if DHCP state indeterminate
- How to verify it
When delaying TX packet to take place after timer is serviced. The issue happens
- Which release branch to backport (provide reason below if selected)