Alert manager addition to dolphin framework #8

sushanthakumar · 2020-04-19T18:11:05Z

What this PR does / why we need it:
This PR is to add alert manager skeleton to dolphin framework

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Test report

Trap receiver is up and listening
V2 and V3 traps are received and processed

Release note:

NajmudheenCT · 2020-04-20T06:01:09Z

dolphin/alert_manager/trap_receiver.py

+        # Load all the mibs
+        self._mib_builder(LOAD_MIB_MODULE)
+
+        self._configure_snmp_userpara()


What if the user opts v1/V2 traps ?

Currently one community (V1/V2) and usm user(v3) are internally configured, this will be replaced once config code is added

NajmudheenCT · 2020-04-20T06:06:40Z

dolphin/alert_manager/trap_receiver.py

+        return
+
+    def start_trap_receiver(self):
+        snmpEngine = engine.SnmpEngine()


engine is the the device who send trap , right?

engine is like a server context at receiver side, everything is attached to this engine

NajmudheenCT · 2020-04-20T06:08:29Z

dolphin/alert_manager/trap_receiver.py

+
+
+        config.addV3User(
+            self.snmpEngine, userName=constants.SNMP_USM_USER,


This user info is the USM user info hat is going to be sending the trap.? How do we achieve hetrogenioty here? Do we start one trap reciever instance for each device registered?

For the usm user,there is remote engine id which is unique for device. For v2, there is a way to configure comunity string along with device ip. Will update code for V2 part

My question is, is config.addV3User is to add a backend device SNMP user info? if so how do we map different device user info ?

config.addV3User is to add snmp user info to snmp engine(not to backened). Here prerequisite is snmp user info at device side is configured. remote engine id field uniquely identifies the device which sends V3 trap. so snmp engine can differentiate between various devices and their corresponding user configurations

wisererik · 2020-04-21T01:29:00Z

dolphin/alert_manager/constants.py

@@ -0,0 +1,31 @@
+# Copyright 2018 The OpenSDS Authors.
+#


2020 The SODA Authors.

Is there any part of the code which is taken from some other place? If yes, then maybe it's not a good idea to change the copyright..We need to be careful here

Copyright updated. Does not contain copied code

wisererik · 2020-04-21T01:29:34Z

dolphin/alert_manager/trap_receiver.py

@@ -0,0 +1,132 @@
+# Copyright 2018 The OpenSDS Authors.
+#


wisererik · 2020-04-21T01:30:11Z

dolphin/alert_manager/trap_receiver.py

+class TrapReceiver(object):
+
+    """Trap receiver functions"""
+    def __init__(self, mibViewController=None, snmpEngine=None):


plz follow python code style

wisererik · 2020-04-21T01:30:58Z

dolphin/alert_manager/trap_receiver.py

+    # Callback function to process the incoming trap
+    def _cbFun(self, stateReference, contextEngineId, contextName,
+              varBinds, cbCtx):
+        LOG.info("####################### NEW Notification #######################")


change info to debug, or remove it

wisererik · 2020-04-21T01:32:03Z

dolphin/alert_manager/trap_receiver.py

+        # Add transport info(ip, port) and start the listener
+        self._add_transport()
+
+        snmpEngine.transportDispatcher.jobStarted(1)


what's is 1 here? or it can be configure?

wisererik · 2020-04-21T01:34:18Z

dolphin/service.py

+
+    def start(self):
+        """Trigger trap receiver creation"""
+        self.manager.start_trap_receiver()


s/start_trap_receiver()/start()/g

wisererik · 2020-04-21T01:41:39Z

dolphin/alert_manager/constants.py

+SNMP_ENGINE_ID="800000d30300000e112245"
+
+# Temporary mib lod dir. This mechanism to be changed later
+MIB_DIR_PATH = '/usr/local/lib/python3.6/dist-packages/pysnmp/smi/mibs'


The following 4 parameters should be configurable in your PR.

# Default values for trap receiver ip address and port DEF_TRAP_RECV_ADDR = "0.0.0.0" DEF_TRAP_RECV_PORT = 162 TRAP_RECEIVER_CLASS = "dolphin.alert_manager.trap_receiver.TrapReceiver" MIB_DIR_PATH = '/usr/local/lib/python3.6/dist-packages/pysnmp/smi/mibs'

kumarashit · 2020-04-21T06:09:06Z

dolphin/alert_manager/constants.py

@@ -0,0 +1,31 @@
+# Copyright 2018 The OpenSDS Authors.
+#


Is there any part of the code which is taken from some other place? If yes, then maybe it's not a good idea to change the copyright..We need to be careful here

kumarashit · 2020-04-21T06:24:10Z

dolphin/alert_manager/constants.py

+# limitations under the License.
+
+
+# Default values for trap receiver ip address and port


If there are different groups of constants, it's better to make them class constants. For example, all MIB constants in class MIB and all Trap constants in class Trap. Advantage is the code readability and flow.
access them like MIB.MIB_DIR_PATH

Not moved to class currently

Any particular reason?

kumarashit · 2020-04-21T06:44:00Z

dolphin/alert_manager/constants.py

+TRAP_RECEIVER_CLASS = "dolphin.alert_manager.trap_receiver.TrapReceiver"
+
+# Temporary snmp community and user configurations
+SNMP_COMMUNITY_STR="public"


One optimisation, if it's constant then better to use single quotes rather than double quotes. This will avoid interpolation.

kumarashit · 2020-04-21T07:09:07Z

dolphin/alert_manager/trap_receiver.py

+        try:
+            self.mibViewController = view.MibViewController(mibBuilder)
+
+            mib_list = MIB_LOAD_LIST


Why is this extra copying required?

kumarashit · 2020-04-21T07:09:39Z

dolphin/alert_manager/trap_receiver.py

+            if (len(mib_list) > 0):
+                mibBuilder.loadModules_new(*mib_list)
+
+        except error.MibNotFoundError as excep:


Better to log the excep.error()

kumarashit · 2020-04-21T07:33:08Z

dolphin/alert_manager/trap_receiver.py

+
+    # Stops the snmp trap receiver
+    def stop_trap_receiver(self):
+        if self.snmpEngine:


exception handling required?

Not needed as it is shutdown flow

kumarashit · 2020-04-21T07:33:46Z

dolphin/alert_manager/trap_receiver.py

+            LOG.info("Trap Listener started .....")
+            snmpEngine.transportDispatcher.runDispatcher()
+        except:
+            snmpEngine.transportDispatcher.closeDispatcher()


Add error log that cannot start

kumarashit · 2020-04-21T07:36:33Z

dolphin/service.py

@@ -63,6 +65,12 @@
                default=False,
                help='Wraps the socket in a SSL context if True is set. '
                     'A certificate file and key file must be specified.'),
+    cfg.HostAddressOpt('trap_receiver_addr',
+                       default="0.0.0.0",


These are already defined as constants then why hard code as "0.0.0.0" and "162" here?

kumarashit · 2020-04-21T07:39:08Z

dolphin/service.py

+        self.manager = manager_class()
+
+    def start(self):
+        """Trigger trap receiver creation"""


Are these comments or doc strings? usually doc strings are put under """

Doc strings for each functions

kumarashit · 2020-04-21T07:40:01Z

dolphin/service.py

+
+    def start(self):
+        """Trigger trap receiver creation"""
+        self.manager.start_trap_receiver()


You are not catching exception here but had raised it in the function?

sfzeng · 2020-04-23T01:09:29Z

dolphin/alert_manager/constants.py

+SNMP_MIB_PATH = '/usr/local/lib/python3.6/dist-packages/pysnmp/smi/mibs'
+
+# SNMP dispatcher job id (static identifier)
+SNMP_DISPATCHER_JOB_ID = 1


Why dispatcher job id is static here?

sfzeng · 2020-04-23T01:13:12Z

dolphin/alert_manager/trap_receiver.py

+
+from oslo_log import log
+
+from pysnmp.entity import engine, config


Should pysnmp be added to requirements.txt, and what kind of license it used?

sfzeng · 2020-04-23T01:16:58Z

dolphin/alert_manager/trap_receiver.py

+            mib_builder.setMibSources(*mib_path)
+            if len(MIB_LOAD_LIST) > 0:
+                mib_builder.loadModules(*MIB_LOAD_LIST)
+        except error.MibNotFoundError:


Is this exception need to be throw out to the upper level?

Ok, updated

sfzeng · 2020-04-23T01:32:16Z

dolphin/alert_manager/trap_receiver.py

+                udp.domainName,
+                udp.UdpTransport().openServerMode((self.trap_receiver_address, int(self.trap_receiver_port)))
+            )
+        except Exception:


Same here..

Ok, updated

wisererik

please add pysnmp==4.4.11 in to requirements.txt

Alert manager addition to dolphin

e759dcc

NajmudheenCT requested changes Apr 20, 2020

View reviewed changes

sushanthakumar changed the title ~~[WIP] Alert manager addition to dolphin framework~~ Alert manager addition to dolphin framework Apr 20, 2020

Cleanup of initial commit

7f2c38e

sushanthakumar force-pushed the alert-manager-addition branch from 238fde5 to 7f2c38e Compare April 20, 2020 19:16

wisererik requested changes Apr 21, 2020

View reviewed changes

kumarashit requested changes Apr 21, 2020

View reviewed changes

sushanthakumar added 2 commits April 22, 2020 00:22

Review comment fixes

adc8ce9

Updated for review comments

5b38ee6

wisererik approved these changes Apr 23, 2020

View reviewed changes

sfzeng reviewed Apr 23, 2020

View reviewed changes

wisererik requested changes Apr 23, 2020

View reviewed changes

sushanthakumar added 2 commits April 23, 2020 16:26

Review comments fix

ac1bcdf

Merge branch 'master' into alert-manager-addition

6e4a47e

sfzeng approved these changes Apr 23, 2020

View reviewed changes

wisererik approved these changes Apr 23, 2020

View reviewed changes

wisererik merged commit 246b417 into sodafoundation:master Apr 23, 2020



		config.addV3User(
		self.snmpEngine, userName=constants.SNMP_USM_USER,

		# limitations under the License.


		# Default values for trap receiver ip address and port


		from oslo_log import log

		from pysnmp.entity import engine, config

Alert manager addition to dolphin framework #8

Alert manager addition to dolphin framework #8

Conversation

sushanthakumar commented Apr 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisererik left a comment

Choose a reason for hiding this comment