leader election framework for performance metric collection #589

AmitRoushan · 2021-06-03T14:44:28Z

What this PR does / why we need it:
The PR introduce leased based leader election for metric job scheduling in distributed performance metric collection framework

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

Special notes for your reviewer:

Release note:

1. Added changes in Service framework for hosting multiple services
2. Added lease based leader election with redis as backend

codecov · 2021-06-03T14:51:56Z

Codecov Report

Merging #589 (3155865) into master (ce9c853) will decrease coverage by 0.91%.
The diff coverage is 41.93%.

❗ Current head 3155865 differs from pull request most recent head e0ec7d0. Consider uploading reports for the commit e0ec7d0 to get more accurate results

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
- Coverage   72.10%   71.18%   -0.92%     
==========================================
  Files         141      144       +3     
  Lines       12814    12794      -20     
  Branches     1535     1528       -7     
==========================================
- Hits         9240     9108     -132     
- Misses       3050     3167     +117     
+ Partials      524      519       -5

Impacted Files	Coverage Δ
delfin/cmd/task.py	`0.00% <0.00%> (ø)`
delfin/leader_election/tooz/leader_elector.py	`24.07% <24.07%> (ø)`
delfin/coordination.py	`63.93% <24.44%> (-23.08%)`	⬇️
delfin/service.py	`27.06% <32.25%> (+1.02%)`	⬆️
delfin/task_manager/scheduler/schedule_manager.py	`55.55% <45.94%> (-4.45%)`	⬇️
delfin/leader_election/factory.py	`46.66% <46.66%> (ø)`
...er/scheduler/schedulers/telemetry/telemetry_job.py	`77.27% <58.06%> (-15.04%)`	⬇️
delfin/leader_election/interface.py	`59.37% <59.37%> (ø)`
...duler/schedulers/telemetry/failed_telemetry_job.py	`79.74% <61.90%> (-11.03%)`	⬇️
delfin/leader_election/tooz/callback.py	`66.66% <66.66%> (ø)`
... and 15 more

NajmudheenCT · 2021-06-16T05:10:36Z

delfin/leader_election/factory.py

+
+        scheduler_mgr = SchedulerManager()
+
+        if plugin == "tooz":


plugin name can be a configurable name, so that it is easy to switch from one plugin type to other in future based on configuration..

Its already a configurable parameter with name leader_election_plugin.

NajmudheenCT · 2021-06-16T05:11:48Z

delfin/leader_election/tooz/leader_elector.py

+
+        while not self._stop.is_set():
+            with timeutils.StopWatch() as w:
+                LOG.info("sending heartbeats for leader election")


This log can be DEBUG leval

logged as debug as suggested

NajmudheenCT · 2021-06-16T05:13:04Z

delfin/leader_election/tooz/leader_elector.py

+                # adjust time for leader
+                has_to_sleep_for = has_to_sleep_for / 2
+
+            LOG.info('resting after leader watch as leader=%(leader)s '


Log.debug ?

logged as debug as suggested

NajmudheenCT · 2021-06-16T05:21:17Z

delfin/coordination.py

+        self.group = None
+
+    def start(self):
+        """Connect to coordination back end."""


Can we make it switchable coordination back end based on config, current implimentaion can be only for redis with toox

created an issue to address the issue as future improvement #610

NajmudheenCT · 2021-06-16T05:24:30Z

delfin/leader_election/factory.py

+class LeaderElectionFactory:
+
+    @staticmethod
+    def construct_elector(plugin, leader_key=None):


can we give a docstring here to describe what the function does?

added docstring. Thanks

NajmudheenCT · 2021-06-16T05:25:23Z

delfin/leader_election/tooz/leader_elector.py

+    def set_leader_callback(self, *args, **kwargs):
+        self.leader = True
+
+    def cleanup(self):


can we give a docstring here to describe what the function does?

cleanup is abstract method of LeaderElector interface. The docstring is already specified there.

AmitRoushan · 2021-06-17T02:22:59Z

PTAL @wisererik @sushanthakumar @kumarashit @joseph-v @PravinRanjan10 @ThisIsClark

ThisIsClark · 2021-06-17T09:22:35Z

delfin/drivers/fake_storage/__init__.py

@@ -80,7 +82,7 @@ def wait_random(low, high):
    def _wait(f, *a, **k):
        rd = random.randint(0, 100)
        secs = low + (high - low) * rd / 100
-        time.sleep(secs)


what's the difference between time.sleep and greenthread.sleep

greenthread.sleep Yield control to another eligible coroutine until at least seconds have elapsed but with time.sleep call do not do same. It eventually keep multiple coroutine busy when we test performance metric collection with load test (register more than 100 storage).

kumarashit · 2021-06-17T12:15:57Z

delfin/coordination.py

+            # check if group exist
+            groups.index(group)
+        except Exception:
+            # create a group if not exist


can we put a debug message here? or for that sake there should be a debug message in any exception block, IMO

added debug message. Thanks

Comment starts with uppercase letter

updated comment as per suggestion

kumarashit · 2021-06-17T12:30:36Z

Changes looks good

PravinRanjan10 · 2021-06-17T15:07:54Z

delfin/leader_election/tooz/leader_elector.py

+        key = leader_election_key.encode('ascii')
+        super(Elector, self).__init__(callbacks, key)
+
+        self._coordinator = None


Any reason, why are we using _ before some variables?

Python takes "_"ed variable as private variable.

I understand the intention. But with single underscore(_), privatization can not be supported in python. If really you want, then either __(double undrscore) or getter(), setter() way required.
Anyway, otherthings looks good to me. Thanks

kumarashit

LGTM

PravinRanjan10 · 2021-06-18T03:58:45Z

LGTM

NajmudheenCT

LGTM

NajmudheenCT · 2021-06-21T05:38:52Z

delfin/drivers/fake_storage/__init__.py

@@ -521,7 +523,7 @@ def get_capabilities(context):
                        "description": "Average time taken for an IO "
                                       "operation in ms"
                    },
-                    "requests": {
+                    "iops": {


This changes are merged using #609 you may rebase

Rebased. Thanks

AmitRoushan · 2021-06-21T08:27:56Z

@ThisIsClark Please take a look.

ThisIsClark · 2021-06-21T08:27:54Z

delfin/coordination.py

+
+        self.group = group
+
+    def join_group(self):


I saw there is create group and join group, do we need remove a node from a group when a node is down

Actually we need not remove node for leader election once node is down. But for cleanup reasons we need to remove the node from the group.
Its done in code

ThisIsClark · 2021-06-21T08:59:41Z

delfin/leader_election/interface.py

+# Copyright 2010 United States Government as represented by the
+# Administrator of the National Aeronautics and Space Administration.
+# Copyright 2011 Justin Santa Barbara


Why do we need to add copyright of these

The copyright string is copied from task cmd

I hope i got your question right.

I got that, but task cmd has some other reason to have that copyright, so for these files in your PR, you could refer to ssl_utils.py to set the copyright

Updated copyright string as per suggestion. Thanks

AmitRoushan · 2021-06-21T09:33:49Z

@ThisIsClark Addressed your comments. PTAL.

NajmudheenCT

LGTM

ThisIsClark

LGTM

wisererik

please follow the python comment style

wisererik · 2021-06-22T07:30:07Z

delfin/coordination.py

+        self.started = True
+
+    def ensure_group(self, group):
+        # request groups


remove comments here

updated as per comment

wisererik · 2021-06-22T07:30:24Z

delfin/coordination.py

+        groups = req.get()
+        try:
+            # check if group exist
+            groups.index(group)


Comment starts with uppercase letter

Updated as per comment

wisererik · 2021-06-22T07:30:34Z

delfin/coordination.py

+            # check if group exist
+            groups.index(group)
+        except Exception:
+            # create a group if not exist


updated the comment

wisererik · 2021-06-22T07:31:23Z

delfin/coordination.py

+            # check if group exist
+            groups.index(group)
+        except Exception:
+            # create a group if not exist


Comment starts with uppercase letter

wisererik · 2021-06-22T07:31:37Z

delfin/coordination.py

+        self.group = group
+
+    def join_group(self):
+        # Join a group


remvoe comments here

comment removed. Thanks

wisererik · 2021-06-22T07:34:26Z

delfin/leader_election/tooz/leader_elector.py

+        self._stop.clear()
+
+        self._coordinator = LeaderElectionCoordinator()
+        # start leader coordinator


remove comment here

wisererik · 2021-06-22T07:34:35Z

delfin/leader_election/tooz/leader_elector.py

+        self._coordinator.start()
+
+        self._coordinator.ensure_group(self.election_key)
+        # join coordinator


remove comments here

comment removed

wisererik · 2021-06-22T07:34:46Z

delfin/leader_election/tooz/leader_elector.py

+        # join coordinator
+        self._coordinator.join_group()
+
+        # register callback for elected leader


remove comment here

comment removed

wisererik · 2021-06-22T07:35:04Z

delfin/leader_election/tooz/leader_elector.py

+                                               callbacks.on_started_leading)
+
+        # register internal callback to notify being a leader
+        self._coordinator. \


remove comments here

This is special case for internal callback to notify current coordinator is selected as leader.
IMO the comment is required for maintainability.

wisererik · 2021-06-22T07:35:32Z

delfin/leader_election/tooz/leader_elector.py

+                    " happen (which will not end well).", ran_for,
+                    ran_for - wait_until_next_beat)
+
+            # check if coordinator is still a leader


Comment starts with uppercase letter

Updated the comment

wisererik

LGTM

NajmudheenCT

LGTM

AmitRoushan force-pushed the dis-perf-framework branch from b6df07d to f90dc54 Compare June 3, 2021 14:50

NajmudheenCT mentioned this pull request Jun 4, 2021

Performance Metric collection Framework: Distributed scheduling for multi node task management #560

Closed

3 tasks

AmitRoushan force-pushed the dis-perf-framework branch 6 times, most recently from 678091c to cf34f14 Compare June 14, 2021 01:41

AmitRoushan changed the title ~~[WIP] leader election framework for performance metric collection~~ leader election framework for performance metric collection Jun 14, 2021

NajmudheenCT reviewed Jun 16, 2021

View reviewed changes

AmitRoushan force-pushed the dis-perf-framework branch 2 times, most recently from 7994321 to c012b7b Compare June 17, 2021 02:18

AmitRoushan requested a review from NajmudheenCT June 17, 2021 02:21

AmitRoushan force-pushed the dis-perf-framework branch from c012b7b to a1ea770 Compare June 17, 2021 04:09

ThisIsClark reviewed Jun 17, 2021

View reviewed changes

kumarashit reviewed Jun 17, 2021

View reviewed changes

PravinRanjan10 reviewed Jun 17, 2021

View reviewed changes

AmitRoushan force-pushed the dis-perf-framework branch 2 times, most recently from 45d0afc to d5bf9ad Compare June 18, 2021 00:59

AmitRoushan mentioned this pull request Jun 18, 2021

switchable coordination back end based on config, current implementation can be only for redis with tooz #610

Open

AmitRoushan requested review from kumarashit, PravinRanjan10 and ThisIsClark June 18, 2021 01:09

AmitRoushan force-pushed the dis-perf-framework branch from d5bf9ad to e9372ab Compare June 18, 2021 01:19

kumarashit previously approved these changes Jun 18, 2021

View reviewed changes

NajmudheenCT previously approved these changes Jun 18, 2021

View reviewed changes

AmitRoushan dismissed stale reviews from NajmudheenCT and kumarashit via 842b499 June 21, 2021 01:55

AmitRoushan force-pushed the dis-perf-framework branch 4 times, most recently from 38f5aa5 to afc6212 Compare June 21, 2021 02:32

NajmudheenCT reviewed Jun 21, 2021

View reviewed changes

AmitRoushan force-pushed the dis-perf-framework branch from afc6212 to 90edc9a Compare June 21, 2021 07:48

AmitRoushan requested review from NajmudheenCT, kumarashit and PravinRanjan10 June 21, 2021 07:51

ThisIsClark reviewed Jun 21, 2021

View reviewed changes

AmitRoushan requested a review from ThisIsClark June 21, 2021 09:20

AmitRoushan force-pushed the dis-perf-framework branch from 90edc9a to e2bfef3 Compare June 21, 2021 09:54

NajmudheenCT previously approved these changes Jun 21, 2021

View reviewed changes

AmitRoushan requested a review from NajmudheenCT June 21, 2021 09:58

ThisIsClark previously approved these changes Jun 21, 2021

View reviewed changes

wisererik requested changes Jun 22, 2021

View reviewed changes

AmitRoushan dismissed stale reviews from ThisIsClark and NajmudheenCT via 9804a91 June 22, 2021 09:38

AmitRoushan force-pushed the dis-perf-framework branch from 3155865 to 9804a91 Compare June 22, 2021 09:38

AmitRoushan requested a review from wisererik June 22, 2021 09:46

distributed performance metric collection framework

e0ec7d0

AmitRoushan force-pushed the dis-perf-framework branch from 9804a91 to e0ec7d0 Compare June 22, 2021 09:48

AmitRoushan requested a review from ThisIsClark June 22, 2021 09:51

wisererik approved these changes Jun 22, 2021

View reviewed changes

NajmudheenCT approved these changes Jun 22, 2021

View reviewed changes

NajmudheenCT merged commit d4a4a25 into sodafoundation:master Jun 22, 2021

leader election framework for performance metric collection #589

leader election framework for performance metric collection #589

Conversation

AmitRoushan commented Jun 3, 2021 • edited Loading

codecov bot commented Jun 3, 2021 • edited Loading

Codecov Report

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

AmitRoushan commented Jun 17, 2021 • edited Loading

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

kumarashit commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

PravinRanjan10 commented Jun 18, 2021

NajmudheenCT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitRoushan commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitRoushan Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmitRoushan commented Jun 21, 2021 • edited Loading

NajmudheenCT left a comment

Choose a reason for hiding this comment

ThisIsClark left a comment

Choose a reason for hiding this comment

wisererik left a 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

NajmudheenCT left a comment

Choose a reason for hiding this comment

AmitRoushan commented Jun 3, 2021 •

edited

Loading

codecov bot commented Jun 3, 2021 •

edited

Loading

AmitRoushan commented Jun 17, 2021 •

edited

Loading

AmitRoushan Jun 21, 2021 •

edited

Loading

AmitRoushan commented Jun 21, 2021 •

edited

Loading