node_agent: Atom to check service status + Alerts socket #67

anmolbabu · 2016-11-22T07:33:06Z

This patch adds the following:

check_service_status.py: Atom to check service status
alert_socket.py: Opens a socket to send out alerts to etcd

tendrl-bug-id: #70
Signed-off-by: anmolbabu anmolbudugutta@gmail.com

anmolbabu · 2016-11-22T07:34:38Z

This is modified version of #19 raised in accordance with comments on the patch and discussions around provisioning, the code separation etc...
The previous pr got closed accidentally by a force rebase of my node_agent repo.

anmolbabu · 2016-11-22T07:34:56Z

@shtripat @nthomas-redhat @brainfunked @r0h4n Kindly review

shtripat · 2016-11-22T10:50:31Z

tendrl/node_agent/manager/alert_socket.py

+class AlertSocketHandler(SocketServer.BaseRequestHandler):
+    def handle(self):
+        try:
+            data = self.request.recv(4096)


I feel 4096 is size of data to be read. We should define a constant for the same.

shtripat · 2016-11-22T10:51:19Z

tendrl/node_agent/manager/alert_socket.py

+        try:
+            data = self.request.recv(4096)
+            alert = json.loads(data)
+            alertKey = '/alerts/' + alert['Host'] + "/" + alert['Plugin']


Use format string like

'/alerts/%s/%s' % (alert['Host'], alert['Plugin'])

shtripat · 2016-11-22T10:52:25Z

tendrl/node_agent/objects/node/atoms/check_service_status.py

+class CheckServiceStatus(object):
+    def run(self, parameters):
+        service_name = parameters.get("service_name")
+        response = os.system("service %s status" % service_name)


We should use systemctl command rather

r0h4n · 2016-11-22T15:21:31Z

Please refer to the github issue being worked upon for this PR

shtripat · 2016-11-28T04:33:47Z

tendrl/node_agent/manager/alert_socket.py

+            alertKey = '/alerts/%s/%s' % (alert['Host'], alert['Plugin'])
+            if 'PluginInstance' in alert:
+                alertKey = "%s/%s" % (alertKey, alert['PluginInstance'])
+            etcd_kwargs = {'port': int(config.get(


etcd_kwargs = { 'port': int(config.get("common", "etcd_port")), 'host': config.get("common", "etcd_connection") }

looks more readable to me :))

shtripat · 2016-11-28T04:35:25Z

tendrl/node_agent/manager/alert_socket.py

+        try:
+            self.server.serve_forever()
+        except Exception as e:
+            print 'Exception caught'


Why print and not log? Also include actual exception in log.

r0h4n · 2016-12-08T14:16:11Z

tendrl/node_agent/tests/test_alerts_utils.py

+    def test_alertsSuccessfulValidate(self):
+        expected_alert = {
+            'Host': '127.0.0.1',
+            'Plugin': 'cpu',


Is this "Plugin" value expected of al alerts that collectd writes to the node agent socket?

Also, ceph_integration and gluster_integration will be writing alerts to the node agent socket,

You willl need to have a generic alert json schema using which any process can write alerts to the node agent socket

Yes 'Plugin' is expected to be present in any collectd dispatched threshold alert. But yes, as you are saying I'll have a generic structure for this because gluster and ceph integrations cannot be expected to send alerts in the collectd format I have a schema proposal in Tendrl/specifications#18 could you please review if the structure there is good enough

r0h4n · 2016-12-08T14:18:50Z

tendrl/node_agent/objects/node/atoms/check_service_status.py

@@ -0,0 +1,12 @@
+import os


Where are the Tendrl definitions for the node object and these atoms under it?

https://github.com/Tendrl/performance_monitoring/pull/2/files#diff-c18a0bbefb2d14b05bfe828e93d355f1

r0h4n · 2016-12-08T14:20:30Z

tendrl/node_agent/manager/alert_socket.py

+            self, server_address, RequestHandlerClass)
+
+
+class AlertsManager(Process):


Any specific reason to choose Process instead of greenlet eg: https://github.com/Tendrl/node_agent/blob/master/tendrl/node_agent/manager/manager.py#L36

https://greenlet.readthedocs.io/en/latest/

The call @ line 80 server.serve_forever() will block all other threads unless its deployed as process

You'll need to drop using "SocketServer.ThreadingMixIn, SocketServer.TCPServer" and use http://www.gevent.org/gevent.server.html

r0h4n · 2016-12-08T14:24:01Z

tendrl/node_agent/manager/alert_socket.py

+            )
+            port = config.get(
+                "node_agent",
+                "collectd_socket_port"


This should renamed as "tendrl_alerts_socket_port" since all tendrl components and collectd will be writing alerts to this socket.

r0h4n · 2016-12-08T14:24:14Z

tendrl/node_agent/manager/alert_socket.py

+        try:
+            hostname = config.get(
+                "node_agent",
+                "collectd_socket_addr"


This should renamed as "tendrl_alerts_socket_addr" since all tendrl components and collectd will be writing alerts to this socket.

r0h4n · 2016-12-08T14:29:47Z

tendrl/node_agent/manager/alert_socket.py

+            self.request.close()
+
+
+class AlertsServer(SocketServer.ThreadingMixIn, SocketServer.TCPServer):


Use gevent socket functionality eg: http://www.gevent.org/gevent.socket.html

r0h4n · 2016-12-08T14:31:56Z

tendrl/node_agent/manager/alert_socket.py

+        EtcdRPC().client.write(key, alert)
+
+
+class AlertSocketHandler(SocketServer.BaseRequestHandler):


http://www.gevent.org/gevent.socket.html

r0h4n · 2016-12-09T09:52:47Z

tendrl/node_agent/manager/alert_socket.py

+            self, server_address, RequestHandlerClass)
+
+
+class AlertsManager(Process):


You'll need to drop using "SocketServer.ThreadingMixIn, SocketServer.TCPServer" and use http://www.gevent.org/gevent.server.html

anmolbabu · 2016-12-12T17:39:38Z

@shtripat @nthomas-redhat @r0h4n Please review

shtripat · 2016-12-13T10:26:32Z

tendrl/node_agent/flows/flow.py

@@ -25,7 +25,8 @@ def __init__(self, flow_name, job, atoms, pre_run, post_run, uuid):

        self.etcd_client = etcd.Client(**etcd_kwargs)
        self.node_id = manager_utils.get_local_node_context()
-        self.cluster_id = self.parameters['Tendrl_context.cluster_id']
+        if 'Tendrl_context.cluster_id' in self.parameters:


I feel you can use self.parameters.get("Tendrl_context.cluster_id"). This would return None if key not found.

shtripat · 2016-12-13T10:29:56Z

tendrl/node_agent/manager/tendrl_definitions_node_agent.py

+              - Service.name
+          name: "check whether the service is running"
+          run: tendrl.node_agent.objects.service.atoms.check_service_status.CheckServiceStatus
+          type: Create


Its not a Create type atom

I felt that while writing this but I found this used for check_node_up which is a similar thing(basically a check atom)

Atom names can be re-verified in future, type is not really used much. we can move on

shtripat · 2016-12-13T11:26:06Z

tendrl/node_agent/objects/node/atoms/cmd.py

@@ -7,6 +8,6 @@
 class Cmd(object):
    def run(self, parameters):
        cmd = parameters.get("Node.cmd_str")
-        cmd = ["nohup"] + cmd.split(" ")
+        cmd = ["nohup"] + shlex.split(cmd)


As part of Tendrl/commons#79 command executor comes to common module. We might need refactor later to use the same.

shtripat

Looks good to me now

r0h4n · 2016-12-15T07:30:54Z

tendrl/node_agent/manager/manager.py


    def join(self):
        LOG.info("%s joining" % self.__class__.__name__)
        self._user_request_thread.join()
        self._discovery_thread.join()
        self.persister.join()

+    def append_definitions(self):
+        defs_path = 'tendrl_definitions_node_agent/data'


Tendrl/specifications#20 (comment)

@r0h4n I'll probably change this once the framework changes related to this are in because otherwise, I won't be able to dev-test this code

anmolbabu · 2016-12-15T08:30:59Z

The spec issue for this PR is: Tendrl/specifications#40

anmolbabu · 2016-12-28T05:53:58Z

TODO(anmolbabu): Add UTs

shtripat · 2017-01-02T11:41:04Z

tendrl/node_agent/alerts/alert_socket.py

+            data = sock.recv(RECEIVE_DATA_SIZE)
+            alert_utils = AlertUtils()
+            alert = alert_utils.validate_alert_json(data)
+            alert['significance'] = 'HIGH'


hardcoded as HIGH?

Yes this is not configurable.

And as per the plan in https://github.com/Tendrl/specifications/blob/master/specs/pluggable_alert_delivery.adoc --
"Each alert will carry additional attribute that carries its significance which will be added by respective handler. The supported significance levels can be 'HIGH', 'MEDIUM' and 'LOW'.This logic will be hardcoded in the codebase that handles the alerts."

shtripat · 2017-01-02T11:42:43Z

tendrl/node_agent/alerts/base_alert_handler.py

+        try:
+            self.alert = Alert.to_obj(alert_json_string)
+            self.alert.alert_id = str(uuid.uuid4())
+            self.alert.significance = 'HIGH'


hardcoded as HIGH

This will be overriden by custom handlers. So in effect the hard coding done here(base handler) is only like a default value which will be overriden by the respective custom handler.

uts

r0h4n

@anmolbabu fix travis unit test failures, rest LGTM

r0h4n · 2017-01-11T08:38:54Z

tendrl/node_agent/alerts/alert_socket.py

@@ -0,0 +1,70 @@
+from _socket import error as _socket_error


umm.. what is _socket?

as in https://github.com/gevent/gevent/blob/master/src/gevent/server.py , gevent uses _socket which I found somewhere can throw the exception _socket._socket_error. So line 65 handles that exception. I haven't myself observed that happening though..

r0h4n

Fix travis unit test failures, rest LGTM

nthomas-redhat

lgtm

shtripat

LGTM. But we need to rebase and use latest commons module. There are changes related to config loading.

tendrl-bug-id: Tendrl#70 tendrl-spec: Tendrl/specifications#18 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

tendrl-bug-id: Tendrl#70 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

coveralls · 2017-01-18T04:48:06Z

Coverage decreased (-2.4%) to 52.666% when pulling 38136d7 on anmolbabu:monitoring into 94eb6ce on Tendrl:master.

brainfunked · 2017-01-25T07:15:09Z

tendrl/node_agent/alerts/alert_socket.py

+        ) as ex:
+            LOG.error('Failed to handle data on alert socket.Error %s' % ex)
+
+    def __init__(self, alerts_socket_addr, alerts_socket_port, etcd_client):


This is supposed to be a UNIX socket, not a TCP socket. The canonical path for such a socket would be under /run/tendrl/message.sock. This would require root privileges, which would be available when deployed via RPM. However, for scenarios where the node agent is being run directly from the git checkout or a non-global installation directory, the socket should be created in the run directory, at the top of the installation tree.

brainfunked · 2017-01-25T07:22:41Z

tendrl/node_agent/alerts/alert_socket.py

+        super(AlertsManager, self).__init__()
+        self.hostname = alerts_socket_addr
+        self.port = alerts_socket_port
+        global db_client


What's going on here?

anmolbabu · 2017-01-31T16:34:38Z

Abandoning as an alternate strategy has been adopted...

anmolbabu mentioned this pull request Nov 22, 2016

monitoring: Add collectd templates,plugins and threshold socket #19

Closed

shtripat reviewed Nov 22, 2016

View reviewed changes

shtripat reviewed Nov 28, 2016

View reviewed changes

anmolbabu force-pushed the monitoring branch from 5953bd2 to 234632a Compare December 6, 2016 11:36

r0h4n suggested changes Dec 8, 2016

View reviewed changes

r0h4n suggested changes Dec 9, 2016

View reviewed changes

anmolbabu force-pushed the monitoring branch from 70b1de3 to e6f9d9d Compare December 11, 2016 07:12

shtripat reviewed Dec 13, 2016

View reviewed changes

r0h4n suggested changes Dec 15, 2016

View reviewed changes

shtripat reviewed Jan 2, 2017

View reviewed changes

anmolbabu force-pushed the monitoring branch from ff5a00a to b2b53eb Compare January 10, 2017 05:56

r0h4n previously approved these changes Jan 11, 2017

View reviewed changes

r0h4n suggested changes Jan 11, 2017

View reviewed changes

r0h4n suggested changes Jan 12, 2017

View reviewed changes

nthomas-redhat approved these changes Jan 12, 2017

View reviewed changes

anmolbabu force-pushed the monitoring branch from 755de0d to 299abd1 Compare January 13, 2017 07:02

shtripat reviewed Jan 14, 2017

View reviewed changes

anmolbabu added 4 commits January 18, 2017 09:11

Add alert socket and alert handlers

1765970

tendrl-bug-id: Tendrl#70 tendrl-spec: Tendrl/specifications#18 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incorporated comments

06483c7

tendrl-bug-id: Tendrl#70 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

JSON to YAML transformations

0ab981a

tendrl-bug-id: Tendrl#70 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Fixes

38136d7

tendrl-bug-id: Tendrl#70 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the monitoring branch from 2fa04ae to 38136d7 Compare January 18, 2017 04:45

brainfunked reviewed Jan 25, 2017

View reviewed changes

anmolbabu closed this Jan 31, 2017

		self, server_address, RequestHandlerClass)


		class AlertsManager(Process):

		self.request.close()


		class AlertsServer(SocketServer.ThreadingMixIn, SocketServer.TCPServer):

		EtcdRPC().client.write(key, alert)


		class AlertSocketHandler(SocketServer.BaseRequestHandler):

node_agent: Atom to check service status + Alerts socket #67

node_agent: Atom to check service status + Alerts socket #67

Conversation

anmolbabu commented Nov 22, 2016 • edited

anmolbabu commented Nov 22, 2016

anmolbabu commented Nov 22, 2016

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

r0h4n commented Nov 22, 2016

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

anmolbabu commented Dec 12, 2016

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

shtripat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu commented Dec 15, 2016

anmolbabu commented Dec 28, 2016

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

anmolbabu Jan 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0h4n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0h4n left a comment

Choose a reason for hiding this comment

nthomas-redhat left a comment

Choose a reason for hiding this comment

shtripat left a comment

Choose a reason for hiding this comment

coveralls commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu commented Jan 31, 2017

anmolbabu commented Nov 22, 2016 •

edited

anmolbabu Jan 3, 2017 •

edited