[dhcp-relay]: Add DHCP Relay Monitor by tahmed-dev · Pull Request #3886 · sonic-net/sonic-buildimage

tahmed-dev · 2019-12-12T17:36:14Z

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. DORA is acronym for
Discover/Offer/Request/ACK DHCP messages which happen in succession for a
successful DHCP transaction. If DHCP Relay is detected to be not forwarding DORA
message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps
counts of clients DR messages, forwarded DR messages, DHCP server OA messages,
and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if
counts are monotonically increasing and record snapshot of those counters.
dhcpmon will report discrepancies when detected between current counters and
snapshot counters.

pull-request: #3886
signed-off-by: Tamer Ahmed tamer.ahmed@microsoft.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

pavel-shirshov

Great work! Thank you!
I left some comments. I'll reread all *.c code in a few days. It's huge

dockers/docker-base-stretch/Dockerfile.j2

src/dhcpmon/src/subdir.mk

src/dhcpmon/src/dhcp_device.c

src/dhcpmon/src/main.c

pavel-shirshov · 2019-12-12T22:50:13Z

Can you please update your description with DORA explanation?
Also, is it possible to avoid using threads here?

tahmed-dev · 2019-12-13T21:05:51Z

Can you please update your description with DORA explanation?
Will explain DORA in next upload. In the meantime, It is acronym for Discover/Offer/Request/ACK DHCP messages which happen in succession for a successful transaction.

Also, is it possible to avoid using threads here?
The call to any of libpcap capture routines' is a blocking call.

pavel-shirshov · 2019-12-13T21:38:51Z

Put "DORA is acronym for Discover/Offer/Request/ACK DHCP messages. what happen in succession for a successful transaction." in the first post here. This would be enough.

You can skip using libpcap at all. Check here:
https://github.com/Azure/sonic-mgmt/blob/master/ansible/roles/test/files/helpers/ferret.py#L115
I create a socket with a filter. And then I use poll() to check many such sockets in one thread.

The filter compiled by tcpdump
https://github.com/Azure/sonic-mgmt/blob/master/ansible/roles/test/files/helpers/ferret.py#L263

jleveque

Per comments. This PR is large, so like Pavel, I have just performed a cursory review. I plan to do more detailed review(s) in the coming days.

src/dhcpmon/src/dhcp_device.c

src/dhcpmon/src/dhcp_mon.c

dockers/docker-dhcp-relay/docker-dhcp-relay.supervisord.conf.j2

src/dhcpmon/src/dhcp_device.h

src/dhcpmon/src/main.c

src/dhcpmon/src/dhcp_device.c

santoshdoke · 2019-12-16T10:54:25Z

Just curious to know the motivation behind this DHCP relay monitor? Why do we need a separate monitor application to see if DHCP relay is working or not? Instead, can we add statistics support to DHCP relay itself?

tahmed-dev

@santoshdoke The motivation is to alert whenever DHCP relay is not healthy. There were situation where DHCP relay failed to forward DORA message. This is a mid-term solution to enable monitoring of DHCP Relay. In the meantime, we consider adding statistics to the relay agent as a long-term solution.

Just curious to know the motivation behind this DHCP relay monitor? Why do we need a separate monitor application to see if DHCP relay is working or not? Instead, can we add statistics support to DHCP relay itself?

jleveque · 2019-12-16T22:35:35Z

Just curious to know the motivation behind this DHCP relay monitor? Why do we need a separate monitor application to see if DHCP relay is working or not? Instead, can we add statistics support to DHCP relay itself?

@santoshdoke: Ultimately, we want to push these statistics to the State DB. Our DB connector is written in C++ but the relay agent is written in C. Adding patches of this nature to the relay agent code would be tedious and ugly and could cause more issues in the long run. Since this is a separate utility, we can easily convert it to C++ and not need to patch an existing open-source project. Plus, a utility cannot properly monitor itself. It makes more sense to have a standalone monitor application.

dockers/docker-dhcp-relay/start.sh

dockers/docker-base-stretch/Dockerfile.j2

src/dhcpmon/src/dhcp_devman.c

src/dhcpmon/src/main.c

src/dhcpmon/src/subdir.mk

pavel-shirshov

Good work!

Please check my comments.
Also check latest Joe's comments.

src/dhcpmon/src/main.c

src/dhcpmon/src/dhcp_mon.c

src/dhcpmon/src/dhcp_mon.h

src/dhcpmon/src/dhcp_devman.c

src/dhcpmon/src/dhcp_device.c

src/dhcpmon/src/dhcp_mon.c

sonic-slave-stretch/Dockerfile.j2

pavel-shirshov

Please address Joe's comment

pavel-shirshov · 2019-12-19T23:56:51Z

retest broadcom please

jleveque · 2019-12-20T21:58:14Z

Retest vsimage please

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. DORA is acronym for Discover/Offer/Request/ACK DHCP messages which happen in succession for a successful DHCP transaction. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. This patch addresses code comments from pull request. pull-request: sonic-net#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. DORA is acronym for Discover/Offer/Request/ACK DHCP messages which happen in succession for a successful DHCP transaction. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. This patch replaces libpcap packet interworking packet capture with raw sockets and libevent. Also, the program now executes as a single thread. pull-request: sonic-net#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. DORA is acronym for Discover/Offer/Request/ACK DHCP messages which happen in succession for a successful DHCP transaction. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. In this patch: 1. Address outstanding PR comments - Remove explicit package installation int the docker base image - Parameterize the compiler usage - Remove empty line 2. Update dhcpmon package dependencies 3. Remove MSG_TRUNC flag from raw socket recv as we dont report any packet truncation pull-request: sonic-net#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

address code comments, also updated some comment to refer to libevent instead of libpcap.

…l API fails.

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. pull-request: sonic-net#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

* [dhcp-relay]: Add DHCP Relay Monitor (#3886) DHCP relay MONitor (dhcpmon) keeps track of DORA messages. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. pull-request: #3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com> * Eliminate dependency on libexplain * Remove dependency on libexplain

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. pull-request: #3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. pull-request: sonic-net#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

DHCP relay MONitor (dhcpmon) keeps track of DORA messages. If DHCP Relay is detected to be not forwarding DORA message, dhcpmon will log such event to syslog. Under the hood dhcpmon keeps counts of clients DR messages, forwarded DR messages, DHCP server OA messages, and forwarded OA messages. dhcpmon will check every 12 sec (configurable) if counts are monotonically increasing and record snapshot of those counters. dhcpmon will report discrepancies when detected between current counters and snapshot counters. pull-request: sonic-net/sonic-buildimage#3886 signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

tahmed-dev requested review from jleveque and lguohan December 12, 2019 17:36

tahmed-dev self-assigned this Dec 12, 2019

tahmed-dev requested a review from pavel-shirshov December 12, 2019 18:13

pavel-shirshov reviewed Dec 12, 2019

View reviewed changes

jleveque suggested changes Dec 13, 2019

View reviewed changes

jleveque suggested changes Dec 16, 2019

View reviewed changes

src/dhcpmon/src/main.c Outdated Show resolved Hide resolved

src/dhcpmon/src/dhcp_device.c Outdated Show resolved Hide resolved

tahmed-dev commented Dec 16, 2019

View reviewed changes

jleveque suggested changes Dec 17, 2019

View reviewed changes

tahmed-dev requested a review from jleveque December 17, 2019 17:19

pavel-shirshov suggested changes Dec 19, 2019

View reviewed changes

pavel-shirshov reviewed Dec 19, 2019

View reviewed changes

sonic-slave-stretch/Dockerfile.j2 Outdated Show resolved Hide resolved

pavel-shirshov previously approved these changes Dec 19, 2019

View reviewed changes

tahmed-dev dismissed pavel-shirshov’s stale review via a60840f December 20, 2019 18:07

tahmed-dev force-pushed the taahme/add-dhcp-relay-monitor branch from a60840f to bbaf62b Compare December 20, 2019 23:21

pavel-shirshov previously approved these changes Dec 20, 2019

View reviewed changes

jleveque added the Request for 201911 Branch label Dec 23, 2019

tahmed-dev added 8 commits January 6, 2020 17:06

[dhcp-relay] Add DHCP Monitor

c5e728a

address code comments, also updated some comment to refer to libevent instead of libpcap.

Addressing PR comments. Also, added a couple of log line when externa…

db09d40

…l API fails.

Adding sigterm back in and also cleaning up upon graceful exit.

193266d

Updating .gitignore per review comments.

3279b22

Changing .gitignore as per review comment

19c24bc

tahmed-dev dismissed stale reviews from pavel-shirshov and jleveque via 19c24bc January 7, 2020 01:19

tahmed-dev force-pushed the taahme/add-dhcp-relay-monitor branch from 4e5e616 to 19c24bc Compare January 7, 2020 01:19

pavel-shirshov approved these changes Jan 7, 2020

View reviewed changes

tahmed-dev closed this Jan 8, 2020

tahmed-dev reopened this Jan 8, 2020

tahmed-dev merged commit 2658ab8 into sonic-net:master Jan 8, 2020

tahmed-dev added the Request for 201811 Branch label Jan 9, 2020

tahmed-dev mentioned this pull request Jan 10, 2020

[dhcp-relay]: Add DHCP Relay Monitor (#3886) #4004

Merged

yxieca added the Included in 201811 Branch label Jan 16, 2020

abdosi added the Included in 201911 Branch label Jan 21, 2020

jleveque added the DHCP Relay label Jul 9, 2020

Conversation

tahmed-dev commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov commented Dec 12, 2019

Uh oh!

tahmed-dev commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel-shirshov commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

santoshdoke commented Dec 16, 2019

Uh oh!

tahmed-dev left a comment

Choose a reason for hiding this comment

Uh oh!

jleveque commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov commented Dec 19, 2019

Uh oh!

jleveque commented Dec 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tahmed-dev commented Dec 12, 2019 •

edited

Loading

tahmed-dev commented Dec 13, 2019 •

edited

Loading

pavel-shirshov commented Dec 13, 2019 •

edited

Loading

jleveque commented Dec 16, 2019 •

edited

Loading