Hello guys,

Follows my comments on the current state of the code.

> === added file 'lava_scheduler_app/utils.py'
> --- lava_scheduler_app/utils.py	1970-01-01 00:00:00 +0000
> +++ lava_scheduler_app/utils.py	2013-08-24 08:58:27 +0000
> @@ -0,0 +1,116 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Neil Williams <email address hidden>
> +#         Senthil Kumaran <email address hidden>
> +#
> +# This file is part of LAVA Scheduler.
> +#
> +# LAVA Scheduler is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU Affero General Public License version 3 as
> +# published by the Free Software Foundation
> +#
> +# LAVA Scheduler is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +# or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU Affero General Public License
> +# along with LAVA Scheduler.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import re
> +import copy
> +import socket
> +import urlparse
> +import simplejson
> +
> +
> +def rewrite_hostname(result_url):
> +    """If URL has hostname value as localhost/127.0.0.*, change it to the
> +    actual server FQDN.
> +
> +    Returns the RESULT_URL (string) re-written with hostname.
> +
> +    See https://cards.linaro.org/browse/LAVA-611
> +    """
> +    host = urlparse.urlparse(result_url).netloc
> +    if host == "localhost":
> +        result_url = result_url.replace("localhost", socket.getfqdn())
> +    elif host.startswith("127.0.0"):
> +        ip_pat = r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}'
> +        result_url = re.sub(ip_pat, socket.getfqdn(), result_url)
> +    return result_url

IMO we should deprecate the "server" parameter for submit_results, and always
submit to master LAVA server somehow. We probably do not need to do this right
now, so I filed a bug for doing it in the future:

https://bugs.launchpad.net/lava-dispatcher/+bug/1217061

> +def split_multi_job(json_jobdata, target_group):
> +    node_json = {}
> +    all_nodes = {}
> +    node_actions = {}
> +    if "device_group" in json_jobdata:

this conditional is redundant since split_multi_job is already called in
the context where the same condition was already tested.

> +        # get all the roles and create node action list for each role.
> +        for group in json_jobdata["device_group"]:
> +            node_actions[group["role"]] = []
> +
> +        # Take each action and assign it to proper roles. If roles are not
> +        # specified for a specific action, then assign it to all the roles.
> +        all_actions = json_jobdata["actions"]
> +        for role in node_actions.keys():
> +            for action in all_actions:
> +                new_action = copy.deepcopy(action)
> +                if 'parameters' in new_action \
> +                        and 'role' in new_action["parameters"]:
> +                    if new_action["parameters"]["role"] == role:
> +                        new_action["parameters"].pop('role', None)
> +                        node_actions[role].append(new_action)
> +                else:
> +                    node_actions[role].append(new_action)
> +
> +        group_count = 0
> +        for clients in json_jobdata["device_group"]:
> +            group_count += int(clients["count"])
> +        for clients in json_jobdata["device_group"]:
> +            role = str(clients["role"])
> +            count = int(clients["count"])
> +            node_json[role] = []
> +            for c in range(0, count):
> +                node_json[role].append({})
> +                node_json[role][c]["timeout"] = json_jobdata["timeout"]
> +                node_json[role][c]["job_name"] = json_jobdata["job_name"]
> +                node_json[role][c]["tags"] = clients["tags"]
> +                node_json[role][c]["group_size"] = group_count
> +                node_json[role][c]["target_group"] = target_group
> +                node_json[role][c]["actions"] = node_actions[role]
> +
> +                node_json[role][c]["role"] = role
> +                # multinode node stage 2
> +                node_json[role][c]["logging_level"] = json_jobdata["logging_level"]
> +                node_json[role][c]["device_type"] = clients["device_type"]
> +
> +        return node_json
> +
> +    return 0
> +
> +
> +def requested_device_count(json_data):
> +    """Utility function check the requested number of devices for each
> +    device_type in a multinode job.
> +
> +    JSON_DATA is the job definition string.
> +
> +    Returns requested_device which is a dictionary of the following format:
> +
> +    {'kvm': 1, 'qemu': 3, 'panda': 1}
> +
> +    If the job is not a multinode job, then return None.
> +    """
> +    job_data = simplejson.loads(json_data)
> +    if 'device_group' in job_data:
> +        requested_devices = {}
> +        for device_group in job_data['device_group']:
> +            device_type = device_group['device_type']
> +            count = device_group['count']
> +            requested_devices[device_type] = count
> +        return requested_devices
> +    else:
> +        # TODO: Put logic to check whether we have requested devices attached
> +        #       to this lava-server, even if it is a single node job?
> +        return None

There is already code that does almost the same thing (checking amount of
available devices for all device types) in lava_scheduler_app/api.py ... we
should probably not duplicate that logic here.

> === modified file 'lava_scheduler_app/views.py'
> --- lava_scheduler_app/views.py	2013-07-17 12:20:25 +0000
> +++ lava_scheduler_app/views.py	2013-08-24 08:58:27 +0000
> @@ -74,10 +74,16 @@
>  
>  
>  def pklink(record):
> +    job_id = record.pk
> +    try:
> +        if record.sub_id:
> +            job_id = record.sub_id
> +    except:
> +        pass
>      return mark_safe(
>          '<a href="%s">%s</a>' % (
>              record.get_absolute_url(),
> -            escape(record.pk)))
> +            escape(job_id)))
>  
>  
>  class IDLinkColumn(Column):
> @@ -100,11 +106,19 @@
>  
>  
>  def all_jobs_with_device_sort():
> +<<<<<<< TREE
>      return TestJob.objects.select_related(
>          "actual_device", "requested_device", "requested_device_type",
>          "submitter", "user", "group")\
>          .extra(select={'device_sort': 'coalesce(actual_device_id, requested_device_id, '
>                                        'requested_device_type_id)'}).all()
> +=======
> +    jobs = TestJob.objects.select_related("actual_device", "requested_device",
> +                                          "requested_device_type", "submitter", "user", "group")\
> +        .extra(select={'device_sort': 'coalesce(actual_device_id, '
> +                                      'requested_device_id, requested_device_type_id)'}).all()
> +    return jobs.order_by('submit_time')
> +>>>>>>> MERGE-SOURCE
>  
>  
>  class JobTable(DataTablesTable):
> @@ -124,7 +138,7 @@
>          else:
>              return ''
>  
> -    id = RestrictedIDLinkColumn()
> +    sub_id = RestrictedIDLinkColumn()
>      status = Column()
>      priority = Column()
>      device = Column(accessor='device_sort')
> @@ -135,9 +149,15 @@
>      duration = Column()
>  
>      datatable_opts = {
> +<<<<<<< TREE
>          'aaSorting': [[0, 'desc']],
>      }
>      searchable_columns = ['description']
> +=======
> +        'aaSorting': [[6, 'desc']],
> +    }
> +    searchable_columns = ['description']
> +>>>>>>> MERGE-SOURCE
>  
>  
>  class IndexJobTable(JobTable):
> @@ -296,6 +316,10 @@
>      class Meta:
>          exclude = ('status', 'submitter', 'end_time', 'priority', 'description')
>  
> +    datatable_opts = {
> +        'aaSorting': [[2, 'desc']],
> +    }
> +
>  
>  def failed_jobs_json(request):
>      return FailedJobTable.json(request, params=(request,))
> @@ -499,6 +523,10 @@
>      class Meta:
>          exclude = ('description', 'device')
>  
> +    datatable_opts = {
> +        'aaSorting': [[4, 'desc']],
> +    }
> +
>  
>  def health_jobs_json(request, pk):
>      device = get_object_or_404(Device, pk=pk)
> @@ -582,7 +610,10 @@
>                  job = TestJob.from_json_and_user(
>                      request.POST.get("json-input"), request.user)
>  
> -                response_data["job_id"] = job.id
> +                if isinstance(job, type(list())):
> +                    response_data["job_list"] = job
> +                else:
> +                    response_data["job_id"] = job.id
>                  return render_to_response(
>                      "lava_scheduler_app/job_submit.html",
>                      response_data, RequestContext(request))
> @@ -660,7 +691,30 @@
>  def job_definition_plain(request, pk):
>      job = get_restricted_job(request.user, pk)
>      response = HttpResponse(job.definition, mimetype='text/plain')
> -    response['Content-Disposition'] = "attachment; filename=job_%d.json" % job.id
> +<<<<<<< TREE
> +    response['Content-Disposition'] = "attachment; filename=job_%d.json" % job.id
> +=======
> +    response['Content-Disposition'] = "attachment; filename=job_%d.json" % job.id
> +    return response
> +
> +
> +def multinode_job_definition(request, pk):
> +    job = get_restricted_job(request.user, pk)
> +    log_file = job.output_file()
> +    return render_to_response(
> +        "lava_scheduler_app/multinode_job_definition.html",
> +        {
> +            'job': job,
> +            'job_file_present': bool(log_file),
> +        },
> +        RequestContext(request))
> +
> +
> +def multinode_job_definition_plain(request, pk):
> +    job = get_restricted_job(request.user, pk)
> +    response = HttpResponse(job.multinode_definition, mimetype='text/plain')
> +    response['Content-Disposition'] = "attachment; filename=multinode_job_%d.json" % job.id
> +>>>>>>> MERGE-SOURCE
>      return response
>  
>  
> @@ -764,7 +818,13 @@
>  def job_cancel(request, pk):
>      job = get_restricted_job(request.user, pk)
>      if job.can_cancel(request.user):
> -        job.cancel()
> +        if job.target_group:


I see this pattern repeated all over the code, but I don't think there's much
we can do about it. But IMO we can make it little better by creating a new
property in the job class so that you can test for `job.is_multinode` instead
of `job.target_group`, which is not obviously related to multinode unless you
understand the implementation details.

so it would be

    if job.is_multinode:
        # code for multinode
    else:
        # code for singlenode

> +            multinode_jobs = TestJob.objects.all().filter(
> +                target_group=job.target_group)
> +            for multinode_job in multinode_jobs:
> +                multinode_job.cancel()
> +        else:
> +            job.cancel()
>          return redirect(job)
>      else:
>          return HttpResponseForbidden(
> @@ -773,11 +833,38 @@
>  
>  @post_only
>  def job_resubmit(request, pk):
> +
> +    response_data = {
> +        'is_authorized': False,
> +        'bread_crumb_trail': BreadCrumbTrail.leading_to(job_list),
> +        }
> +
>      job = get_restricted_job(request.user, pk)
>      if job.can_resubmit(request.user):
> -        definition = job.definition
> -        job = TestJob.from_json_and_user(definition, request.user)
> -        return redirect(job)
> +        response_data["is_authorized"] = True
> +
> +        if job.target_group:
> +            definition = job.multinode_definition
> +        else:
> +            definition = job.definition
> +
> +        try:
> +            job = TestJob.from_json_and_user(definition, request.user)
> +
> +            if isinstance(job, type(list())):
> +                response_data["job_list"] = job
> +                return render_to_response(
> +                    "lava_scheduler_app/job_submit.html",
> +                    response_data, RequestContext(request))
> +            else:
> +                return redirect(job)
> +        except Exception as e:

Exception is too generic and might mask unrelated exceptions ... we should
catch here just the exception types we actually expect to be raised inside the
try block

> +            response_data["error"] = str(e)
> +            response_data["json_input"] = definition
> +            return render_to_response(
> +                "lava_scheduler_app/job_submit.html",
> +                response_data, RequestContext(request))
> +
>      else:
>          return HttpResponseForbidden(
>              "you cannot re-submit this job", content_type="text/plain")
> 
> === modified file 'lava_scheduler_daemon/board.py'
> --- lava_scheduler_daemon/board.py	2013-07-17 12:20:25 +0000
> +++ lava_scheduler_daemon/board.py	2013-08-24 08:58:27 +0000
> @@ -339,7 +339,7 @@
>          self.logger.info("starting job %r", job_data)
>          self.running_job = self.job_cls(
>              job_data, self.dispatcher, self.source, self.board_name,
> -            self.reactor, self.daemon_options)
> +            self.reactor, self.daemon_options, None)

This looks wrong. MonitorJob.__init__ only takes 6 arguments. This should crash
the scheduler daemon, but since that is not happening I assume this is dead
code?

>          d = self.running_job.run()
>          d.addCallbacks(self._cbJobFinished, self._ebJobFinished)
>  
> 
> === modified file 'lava_scheduler_daemon/dbjobsource.py'
> --- lava_scheduler_daemon/dbjobsource.py	2013-07-17 12:20:25 +0000
> +++ lava_scheduler_daemon/dbjobsource.py	2013-08-24 08:58:27 +0000
> @@ -3,6 +3,7 @@
>  import os
>  import shutil
>  import urlparse
> +import copy
>  
>  from dashboard_app.models import Bundle
>  
> @@ -93,9 +94,12 @@
>          return self.deferToThread(wrapper, *args, **kw)
>  
>      def getBoardList_impl(self):
> +        self.logger.info("Checking configured devices")
>          configured_boards = [
>              x.hostname for x in dispatcher_config.get_devices()]
>          boards = []
> +        for d in configured_boards:
> +            self.logger.info("%s is configured" % d.hostname)

I have the impression that this will create an unecessary amount of noise in
the scheduler logs ...  logger.debug maybe?

>          for d in Device.objects.all():
>              if d.hostname in configured_boards:
>                  boards.append({'hostname': d.hostname})
> @@ -104,9 +108,135 @@
>      def getBoardList(self):
>          return self.deferForDB(self.getBoardList_impl)
>  
> +    def _get_health_check_jobs(self):
> +        """Gets the list of configured boards and checks which are the boards
> +        that require health check.
> +
> +        Returns JOB_LIST which is a set of health check jobs. If no health
> +        check jobs are available returns an empty set.
> +        """
> +        job_list = set()
> +        configured_boards = [
> +            x.hostname for x in dispatcher_config.get_devices()]
> +        boards = []
> +        for d in Device.objects.all():
> +            if d.hostname in configured_boards:
> +                boards.append(d)
> +
> +        for device in boards:
> +            if device.status != Device.IDLE:
> +                continue
> +            if not device.device_type.health_check_job:
> +                run_health_check = False
> +            elif device.health_status == Device.HEALTH_UNKNOWN:
> +                run_health_check = True
> +            elif device.health_status == Device.HEALTH_LOOPING:
> +                run_health_check = True
> +            elif not device.last_health_report_job:
> +                run_health_check = True
> +            else:
> +                run_health_check = device.last_health_report_job.end_time < \
> +                    datetime.datetime.now() - datetime.timedelta(days=1)
> +            if run_health_check:
> +                job_list.add(self._getHealthCheckJobForBoard(device))
> +        return job_list
> +
> +    def _fix_device(self, device, job):
> +        """Associate an available/idle DEVICE to the given JOB.
> +
> +        Returns the job with actual_device set to DEVICE.
> +
> +        If we are unable to grab the DEVICE then we return None.
> +        """
> +        DeviceStateTransition.objects.create(
> +            created_by=None, device=device, old_state=device.status,
> +            new_state=Device.RUNNING, message=None, job=job).save()
> +        device.status = Device.RUNNING
> +        device.current_job = job
> +        try:
> +            # The unique constraint on current_job may cause this to
> +            # fail in the case of concurrent requests for different
> +            # boards grabbing the same job.  If there are concurrent
> +            # requests for the *same* board they may both return the
> +            # same job -- this is an application level bug though.
> +            device.save()
> +        except IntegrityError:
> +            self.logger.info(
> +                "job %s has been assigned to another board -- rolling back",
> +                job.id)
> +            transaction.rollback()
> +            return None
> +        else:
> +            job.actual_device = device
> +            job.log_file.save(
> +                'job-%s.log' % job.id, ContentFile(''), save=False)
> +            job.submit_token = AuthToken.objects.create(user=job.submitter)
> +            job.definition = simplejson.dumps(self._get_json_data(job),
> +                                              sort_keys=True,
> +                                              indent=4 * ' ')
> +            job.save()
> +            transaction.commit()
> +        return job
> +
> +    def getJobList_impl(self):
> +        jobs = TestJob.objects.all().filter(
> +            status=TestJob.SUBMITTED).order_by('-priority', 'submit_time')
> +        job_list = self._get_health_check_jobs()
> +        devices = None
> +        configured_boards = [
> +            x.hostname for x in dispatcher_config.get_devices()]
> +        self.logger.debug("Number of configured_devices: %d" % len(configured_boards))
> +        for job in jobs:
> +            if job.actual_device:
> +                job_list.add(job)
> +            elif job.requested_device:
> +                self.logger.debug("Checking Requested Device")
> +                devices = Device.objects.all().filter(
> +                    hostname=job.requested_device.hostname,
> +                    status=Device.IDLE)
> +            elif job.requested_device_type:
> +                self.logger.debug("Checking Requested Device Type")
> +                devices = Device.objects.all().filter(
> +                    device_type=job.requested_device_type,
> +                    status=Device.IDLE)
> +            else:
> +                continue
> +            if devices:
> +                for d in devices:
> +                    self.logger.info("Checking %s" % d.hostname)

too much noise too?

> +                    if d.hostname in configured_boards:
> +                       if job:
> +                           job = self._fix_device(d, job)
> +                       if job:
> +                           job_list.add(job)
> +
> +        # Remove scheduling multinode jobs until all the jobs in the
> +        # target_group are assigned devices.
> +        final_job_list = copy.deepcopy(job_list)
> +        for job in job_list:
> +            if job.target_group:
> +                multinode_jobs = TestJob.objects.all().filter(
> +                    target_group=job.target_group)
> +
> +                jobs_with_device = 0
> +                for multinode_job in multinode_jobs:
> +                    if multinode_job.actual_device:
> +                        jobs_with_device += 1
> +
> +                if len(multinode_jobs) != jobs_with_device:
> +                    final_job_list.difference_update(set(multinode_jobs))
> +                
> +        return final_job_list

Is there a possibility of starvation for multinode jobs? i.e. you request 10
devices, only got 9 so far, but the 10th device is always busy picking up job
from a queue and the multinode job sits there waiting for a long time.

> +
> +    def getJobList(self):
> +        return self.deferForDB(self.getJobList_impl)
> +
>      def _get_json_data(self, job):
>          json_data = simplejson.loads(job.definition)
> -        json_data['target'] = job.actual_device.hostname
> +        if job.actual_device:
> +            json_data['target'] = job.actual_device.hostname
> +        elif job.requested_device:
> +            json_data['target'] = job.requested_device.hostname
>          for action in json_data['actions']:
>              if not action['command'].startswith('submit_results'):
>                  continue
> @@ -230,6 +360,20 @@
>      def getJobForBoard(self, board_name):
>          return self.deferForDB(self.getJobForBoard_impl, board_name)
>  
> +    def getJobDetails_impl(self, job):
> +        job.status = TestJob.RUNNING
> +        job.start_time = datetime.datetime.utcnow()
> +        shutil.rmtree(job.output_dir, ignore_errors=True)
> +        job.log_file.save('job-%s.log' % job.id, ContentFile(''), save=False)
> +        job.submit_token = AuthToken.objects.create(user=job.submitter)
> +        job.save()
> +        json_data = self._get_json_data(job)
> +        transaction.commit()
> +        return json_data
> +
> +    def getJobDetails(self, job):
> +        return self.deferForDB(self.getJobDetails_impl, job)
> +
>      def getOutputDirForJobOnBoard_impl(self, board_name):
>          device = Device.objects.get(hostname=board_name)
>          job = device.current_job
> 
> === added file 'lava_scheduler_daemon/job.py'
> --- lava_scheduler_daemon/job.py	1970-01-01 00:00:00 +0000
> +++ lava_scheduler_daemon/job.py	2013-08-24 08:58:27 +0000
> @@ -0,0 +1,83 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Senthil Kumaran <email address hidden>
> +#
> +# This file is part of LAVA Scheduler.
> +#
> +# LAVA Scheduler is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU Affero General Public License version 3 as
> +# published by the Free Software Foundation
> +#
> +# LAVA Scheduler is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> +# or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU Affero General Public License
> +# along with LAVA Scheduler.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import logging
> +
> +from twisted.internet import defer
> +from lava_scheduler_daemon.board import MonitorJob
> +
> +
> +class NewJob(object):

Somehow the name `NewJob` feels wrong ... I think JobRunner would be better?

> +    job_cls = MonitorJob
> +
> +    def __init__(self, source, job, dispatcher, reactor, daemon_options,
> +                 job_cls=None):
> +        self.source = source
> +        self.dispatcher = dispatcher
> +        self.reactor = reactor
> +        self.daemon_options = daemon_options
> +        self.job = job
> +        if job.actual_device:
> +            self.board_name = job.actual_device.hostname
> +        elif job.requested_device:
> +            self.board_name = job.requested_device.hostname
> +        if job_cls is not None:
> +            self.job_cls = job_cls
> +        self.running_job = None
> +        self.logger = logging.getLogger(__name__ + '.NewJob.' + str(job.id))
> +
> +    def start(self):
> +        self.logger.debug("processing job")
> +        if self.job is None:
> +            self.logger.debug("no job found for processing")
> +            return
> +        self.source.getJobDetails(self.job).addCallbacks(
> +            self._startJob, self._ebStartJob)
> +
> +    def _startJob(self, job_data):
> +        if job_data is None:
> +            self.logger.debug("no job found")
> +            return
> +        self.logger.info("starting job %r", job_data)
> +
> +        self.running_job = self.job_cls(
> +            job_data, self.dispatcher, self.source, self.board_name,
> +            self.reactor, self.daemon_options)
> +        d = self.running_job.run()
> +        d.addCallbacks(self._cbJobFinished, self._ebJobFinished)
> +
> +    def _ebStartJob(self, result):
> +        self.logger.error(
> +            '%s: %s\n%s', result.type.__name__, result.value,
> +            result.getTraceback())
> +        return
> +
> +    def stop(self):
> +        self.logger.debug("stopping")
> +
> +        if self.running_job is not None:
> +            self.logger.debug("job running; deferring stop")
> +        else:
> +            self.logger.debug("stopping immediately")
> +            return defer.succeed(None)
> +
> +    def _ebJobFinished(self, result):
> +        self.logger.exception(result.value)
> +
> +    def _cbJobFinished(self, result):
> +        self.running_job = None
> 
> === modified file 'lava_scheduler_daemon/service.py'
> --- lava_scheduler_daemon/service.py	2012-12-03 05:03:38 +0000
> +++ lava_scheduler_daemon/service.py	2013-08-24 08:58:27 +0000
> @@ -5,6 +5,7 @@
>  from twisted.internet.task import LoopingCall
>  
>  from lava_scheduler_daemon.board import Board, catchall_errback
> +from lava_scheduler_daemon.job import NewJob
>  
>  
>  class BoardSet(Service):
> @@ -56,3 +57,34 @@
>          self.logger.info(
>              "waiting for %s boards", len(self.boards) - len(dead_boards))
>          return defer.gatherResults(ds)
> +
> +

It seems that the BoardSet class is now dead code. It should probably be
removed ...

The Board class from lava_scheduler_daemon/board.py also seems to be dead code now.

> +class JobQueue(Service):
> +
> +    def __init__(self, source, dispatcher, reactor, daemon_options):
> +        self.logger = logging.getLogger(__name__ + '.JobQueue')
> +        self.source = source
> +        self.dispatcher = dispatcher
> +        self.reactor = reactor
> +        self.daemon_options = daemon_options
> +        self._check_job_call = LoopingCall(self._checkJobs)
> +        self._check_job_call.clock = reactor
> +
> +    def _checkJobs(self):
> +        self.logger.debug("Refreshing jobs")
> +        return self.source.getJobList().addCallback(
> +            self._cbCheckJobs).addErrback(catchall_errback(self.logger))
> +
> +    def _cbCheckJobs(self, job_list):
> +        for job in job_list:
> +            new_job = NewJob(self.source, job, self.dispatcher, self.reactor,
> +                             self.daemon_options)
> +            self.logger.info("Starting Job: %d " % job.id)
> +            new_job.start()
> +
> +    def startService(self):
> +        self._check_job_call.start(20)
> +
> +    def stopService(self):
> +        self._check_job_call.stop()
> +        return None
> 

 review needs-fixing