Page MenuHomeSoftware Heritage
Differential D2237

lister: Add checks on expected scheduler's output tasks
AbandonedPublic
Actions

Authored by ardumont on Nov 8 2019, 11:12 AM.

Details

Reviewers
douardda
Group Reviewers
Reviewers
Summary

The functionality need is to ensure we have the required task types within the
scheduler. It must be done early in order to prevent the lister to do
computations (and hitting external apis) and then fail to insert the computed
tasks if the scheduler is not correctly setup-ed.

Let's make it fail as soon as possible.

I'm not sure where to implement the check function call though.
Within the:

  • lister's constructor
  • lister's task (the one creating the lister)
  • somewhere else?
Test Plan

tox

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8893
Build 12982: tox-on-jenkinsJenkins
Build 12981: arc lint + arc unit

Event Timeline

ardumont created this revision.Nov 8 2019, 11:12 AM
ardumont updated this revision to Diff 7724.Nov 8 2019, 11:15 AM

Improve docstring

swh-public-ci added a comment.Nov 8 2019, 11:15 AM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/481/ for more details.

Harbormaster completed remote builds in B8893: Diff 7723.Nov 8 2019, 11:15 AM
swh-public-ci added a comment.Nov 8 2019, 11:18 AM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/482/ for more details.

Harbormaster completed remote builds in B8894: Diff 7724.Nov 8 2019, 11:18 AM
ardumont updated this revision to Diff 7725.Nov 8 2019, 11:19 AM

Fix typo

vlorentz added a subscriber: vlorentz.Nov 8 2019, 11:19 AM
vlorentz added inline comments.
swh/lister/debian/lister.py
58–59

The constructor is the the right place for documenting the "output"; it should be either in the class docstring's or in a method's.

It's also unclear what you call an output (is it a return value of one of the methods?)

swh-public-ci added a comment.Nov 8 2019, 11:22 AM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/483/ for more details.

Harbormaster completed remote builds in B8895: Diff 7725.Nov 8 2019, 11:22 AM
ardumont added inline comments.Nov 8 2019, 11:24 AM
swh/lister/debian/lister.py
58–59

The constructor is the the right place for documenting the "output"; it should be either in the class docstring's or in a method's.

(I guess you meant not the right place)
I started in the class's docstring so i can put it back there.

It's also unclear what you call an output (is it a return value of one of the methods?)

Well, it's the lister's goal. It output tasks for loaders.

But it's not clearly said anywhere.

It's not a return value per say, it's written to the scheduler's db.
We can "somehow" have a glimpse when we look at the def task_dict(self, origin_type, origin_url, **kwargs): method implementations.
But it's not that apparent.

ardumont added a subscriber: olasd.EditedNov 8 2019, 12:03 PM

About the when/where:

irc discussion

This points toward the constructor as a reasonable location.
P558: @olasd and me.

pros:

  • should be simple enough to adapt the current diff with this

cons:

  • kind of side-effect within the constructor (side-effect as in "discuss with the scheduler api")
  • will that fail early enough?

Oral discussion

This points toward orchestrating within the scheduler (improving the current registering/initialization mechanism).
(@douardda and me)

pros:

  • make it fail even earlier, prior to the lister task registration (since there is no point in registering a task which cannot work)
  • no "side-effect"

cons:

  • most probably more work
  • is it possible?
ardumont planned changes to this revision.Nov 8 2019, 2:04 PM

This needs more thinking.

ardumont abandoned this revision.Feb 28 2020, 2:18 PM

Revision Contents
Changeset List

PathSize
swh/
lister/
core/
24 lines
debian/
5 lines
tests/
9 lines

Diff 7723

View Options

swh/lister/core/lister_base.py

# Copyright (C) 2015-2018 the Software Heritage developers # Copyright (C) 2015-2019 the Software Heritage developers
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import abc import abc
import datetime import datetime
import gzip import gzip
import json import json
import logging import logging
import os import os
import re import re
import time import time
from sqlalchemy import create_engine, func from sqlalchemy import create_engine, func
from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import sessionmaker
from typing import Any, Type, Union from typing import Any, Sequence, Type, Union
from swh.core import config from swh.core import config
from swh.scheduler import get_scheduler, utils from swh.scheduler import get_scheduler, utils
from .abstractattribute import AbstractAttribute from .abstractattribute import AbstractAttribute
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Linesclass ListerBase(abc.ABC, config.SWHConfig):
""" """
MODEL = AbstractAttribute( MODEL = AbstractAttribute(
'Subclass type (not instance) of swh.lister.core.models.ModelBase ' 'Subclass type (not instance) of swh.lister.core.models.ModelBase '
'customized for a specific service.' 'customized for a specific service.'
) # type: Union[AbstractAttribute, Type[Any]] ) # type: Union[AbstractAttribute, Type[Any]]
LISTER_NAME = AbstractAttribute( LISTER_NAME = AbstractAttribute(
"Lister's name") # type: Union[AbstractAttribute, str] "Lister's name") # type: Union[AbstractAttribute, str]
# Lister's output scheduler task types (default to empty)
SCHEDULED_TASK_TYPES: Sequence[str] = []
def transport_request(self, identifier): def transport_request(self, identifier):
"""Given a target endpoint identifier to query, try once to request it. """Given a target endpoint identifier to query, try once to request it.
Implementation of this method determines the network request protocol. Implementation of this method determines the network request protocol.
Args: Args:
identifier (string): unique identifier for an endpoint query. identifier (string): unique identifier for an endpoint query.
▲ Show 20 Lines • Show All 170 Lines • ▼ Show 20 Lines def __init__(self, override_config=None):
self.config.update(override_config) self.config.update(override_config)
logger.debug('%s CONFIG=%s' % (self, self.config)) logger.debug('%s CONFIG=%s' % (self, self.config))
self.scheduler = get_scheduler(**self.config['scheduler']) self.scheduler = get_scheduler(**self.config['scheduler'])
self.db_engine = create_engine(self.config['lister']['args']['db']) self.db_engine = create_engine(self.config['lister']['args']['db'])
self.mk_session = sessionmaker(bind=self.db_engine) self.mk_session = sessionmaker(bind=self.db_engine)
self.db_session = self.mk_session() self.db_session = self.mk_session()
def checks(self):
"""Basic checks on lister configuration.
This checks the expected output task types exist in the scheduler. It
it does not, this raises a ValueError.
"""
missing_types = []
for task_type in self.SCHEDULED_TASK_TYPES:
ok = self.scheduler.get_task_type(task_type)
if not ok:
missing_types.append(task_type)
if missing_types:
raise ValueError(
"Misconfigured lister: Missing %s type in scheduler" % (
','.join(missing_types)))
def reset_backoff(self): def reset_backoff(self):
"""Reset exponential backoff timeout to initial level.""" """Reset exponential backoff timeout to initial level."""
self.backoff = self.INITIAL_BACKOFF self.backoff = self.INITIAL_BACKOFF
def back_off(self): def back_off(self):
"""Get next exponential backoff timeout.""" """Get next exponential backoff timeout."""
ret = self.backoff ret = self.backoff
self.backoff *= 10 self.backoff *= 10
▲ Show 20 Lines • Show All 254 Lines • Show Last 20 Lines
View Options

swh/lister/debian/lister.py

Show All 28 Linesdecompressors = {
'xz': lzma.LZMAFile, 'xz': lzma.LZMAFile,
} }
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
class DebianLister(ListerHttpTransport, ListerBase): class DebianLister(ListerHttpTransport, ListerBase):
"""Debian Lister which lists debian distributions.
The output of the lister should be scheduled tasks of type load-deb-package
"""
MODEL = Package MODEL = Package
PATH_TEMPLATE = None PATH_TEMPLATE = None
LISTER_NAME = 'debian' LISTER_NAME = 'debian'
instance = 'debian' instance = 'debian'
SCHEDULED_TASK_TYPES = ['load-deb-package']
def __init__(self, distribution: str = 'Debian', def __init__(self, distribution: str = 'Debian',
date: Optional[datetime.datetime] = None, date: Optional[datetime.datetime] = None,
override_config: Mapping = {}): override_config: Mapping = {}):
"""Initialize the debian lister for a given distribution at a given """Initialize the debian lister for a given distribution at a given
date. date.
Args: Args:
distribution: name of the distribution (e.g. "Debian") distribution: name of the distribution (e.g. "Debian")
date: date the snapshot is taken (defaults to now if empty) date: date the snapshot is taken (defaults to now if empty)
override_config: Override configuration (which takes precedence override_config: Override configuration (which takes precedence
over the parameters if provided) over the parameters if provided)
""" """
vlorentzUnsubmitted
Not Done
Inline Actions

The constructor is the the right place for documenting the "output"; it should be either in the class docstring's or in a method's.

It's also unclear what you call an output (is it a return value of one of the methods?)

vlorentz: The constructor is the the right place for documenting the "output"; it should be either in the…
ardumontAuthorUnsubmitted
Done
Inline Actions

The constructor is the the right place for documenting the "output"; it should be either in the class docstring's or in a method's.

(I guess you meant not the right place)
I started in the class's docstring so i can put it back there.

It's also unclear what you call an output (is it a return value of one of the methods?)

Well, it's the lister's goal. It output tasks for loaders.

But it's not clearly said anywhere.

It's not a return value per say, it's written to the scheduler's db.
We can "somehow" have a glimpse when we look at the def task_dict(self, origin_type, origin_url, **kwargs): method implementations.
But it's not that apparent.

ardumont: > The constructor is the the right place for documenting the "output"; it should be either in…
ListerHttpTransport.__init__(self, url="notused") ListerHttpTransport.__init__(self, url="notused")
ListerBase.__init__(self, override_config=override_config) ListerBase.__init__(self, override_config=override_config)
self.distribution = override_config.get('distribution', distribution) self.distribution = override_config.get('distribution', distribution)
self.date = override_config.get('date', date) or datetime.datetime.now( self.date = override_config.get('date', date) or datetime.datetime.now(
tz=datetime.timezone.utc) tz=datetime.timezone.utc)
def transport_request(self, identifier): def transport_request(self, identifier):
"""Subvert ListerHttpTransport.transport_request, to try several """Subvert ListerHttpTransport.transport_request, to try several
▲ Show 20 Lines • Show All 191 Lines • Show Last 20 Lines
View Options

swh/lister/debian/tests/conftest.py

# Copyright (C) 2019 The Software Heritage developers # Copyright (C) 2019 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import pytest import pytest
from swh.lister.core.tests.conftest import * # noqa from swh.lister.core.tests.conftest import * # noqa
from swh.lister.debian import debian_init from swh.lister.debian import debian_init
@pytest.fixture @pytest.fixture
def lister_debian(swh_listers): def lister_debian(swh_listers):
lister = swh_listers['debian'] lister = swh_listers['debian']
# Initialize the debian data model # Initialize debian data model
debian_init(lister.db_engine, debian_init(lister.db_engine,
distributions=['stretch'], distributions=['stretch'],
area_names=['main', 'contrib']) area_names=['main', 'contrib'])
with pytest.raises(ValueError,
match='Misconfigured lister'):
lister.checks()
# Add the load-deb-package in the scheduler backend # Add the load-deb-package in the scheduler backend
lister.scheduler.create_task_type({ lister.scheduler.create_task_type({
'type': 'load-deb-package', 'type': 'load-deb-package',
'description': 'Load a Debian package', 'description': 'Load a Debian package',
'backend_name': 'swh.loader.debian.tasks.LoaderDebianPackage', 'backend_name': 'swh.loader.debian.tasks.LoaderDebianPackage',
'default_interval': '1 day', 'default_interval': '1 day',
}) })
# everything is now fine
lister.checks()
return lister return lister
About · Developer information · Legal