Bug 44786 - Improve listener module API
Improve listener module API
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Listener (univention-directory-listener)
UCS 4.2
Other Linux
: P5 normal (vote)
: UCS 4.2-3-errata
Assigned To: Daniel Tröder
Dirk Wiesenthal
:
Depends on: 45030
Blocks: 48111 48139 51313
  Show dependency treegraph
 
Reported: 2017-06-14 16:37 CEST by Daniel Tröder
Modified: 2020-05-25 19:01 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 2: Improvement: Would be a product improvement
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.046
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Tröder univentionstaff 2017-06-14 16:37:26 CEST
The listener module API makes reading and writing listener modules more cumbersome than necessary.

* It pollutes the module (file) namespace for both configuration and code with hard coded attribute names.
* → Adding configuration variables is dangerous, as a name in the module namespace might be in use in an existing module.
* The signature of the central function changes depending on the setting of another variable.
* Because all possible LDAP changes have to be handled by a single function call, every listener module repeats the same situation-classification code.
* All listener modules and the listener server log together into the same file.
* All listener modules and the listener server use the same logging level.
* The modrdn switch makes it necessary for every programmer to write (the same) code to handle it.


Create a listener module API that allows evolving the listener framework without endangering existing listener modules. The implementation should deliver common code (like modrdn and LDAP connection handling and logging) and have the framework call dedicated functions with proper names for different situations (create, modify, ...). Separate configuration from code to allow evolving them separately.

Create an adapter to allow using the new API with the existing listener framework.
Comment 1 Daniel Tröder univentionstaff 2017-06-14 16:50:05 CEST
r80186: fork listener package
r80187: add new listener module API
Comment 2 Daniel Tröder univentionstaff 2017-06-19 07:06:05 CEST
r80267: add UCR switch to deactivate listener module, add UCRV descriptions
Comment 3 Sönke Schwardt-Krummrich univentionstaff 2017-06-19 21:45:04 CEST
(In reply to Daniel Tröder from comment #0)
> * All listener modules and the listener server log together into the same
> file.

I see this as an advantage since the listener modules are processed sequentially and the order in which actions are performed by listener modules is sometimes relevant (provided that each listener module gets is own automatic logging prefix).
Comment 4 Daniel Tröder univentionstaff 2017-06-20 06:43:16 CEST
IMHO the main listener.log should only contain what it is doing, not what the modules are doing. Or at least only with log level >= ERROR.
started  LM1
finished LM1
started  LM2
finished LM2
started  LM3
ERROR in LM3
traceback of LM3
started  LM4
...

And in separate log files for each LM all the stuff they have to say in all detail.

Anyway, in the proposed API implementation the logging is done so that all output goes to both /v/l/u/listener.log (backwards compatible) as well as /v/l/u/listener_module/<name>.log.
The loglevel for messages that are sent to listener.log is taken from listener/debug/level, the loglevel for listener_module/<name>.log is taken from listener/module/<name>/debug/level.
Comment 5 Daniel Tröder univentionstaff 2017-06-26 13:23:58 CEST
r80481: fix lost modify call, add ucs-test
Comment 6 Florian Best univentionstaff 2017-10-25 16:22:59 CEST
This bug is tagged for UCS 4.2-3. You committed it into the 4.2-2 branch. Is this correct?
Comment 7 Daniel Tröder univentionstaff 2017-10-25 17:17:38 CEST
A new API for programming listener modules was added.
The Univention Directory Listener (further down called "listener server") was not modified. To allow the use of the new module API while maintaining the old API for the listener server, an adapter class was also added.

In /usr/share/doc/univention-directory-listener/examples two examples can be found. The simple one, which can be used for quickly creating simple listener modules looks like this:

--------------------------------------------------------------------

from univention.listener import ListenerModuleAdapter, ListenerModuleHandler, ListenerModuleConfiguration


class ListenerModuleTemplate(ListenerModuleHandler):
	def create(self, dn, new):
		self.logger.debug('dn=%r', dn)

	def modify(self, dn, old, new, old_dn):
		self.logger.debug('dn=%r old_dn=%r', dn, old_dn)
		if old_dn:
			self.logger.info('it is (also) a move')

	def remove(self, dn, old):
		self.logger.debug('dn=%r', dn)


class ListenerModuleTemplateConfiguration(ListenerModuleConfiguration):
	name = 'listener module template'
	description = 'a listener module template'
	ldap_filter = ''
	attributes = []
	listener_module_class = ListenerModuleTemplate


globals().update(ListenerModuleAdapter(ListenerModuleTemplateConfiguration()).get_globals())

--------------------------------------------------------------------

A Python logging object is available at self.logger. Two handlers are registered:
* One is an adapter for univention.debug, sending messages to /v/l/u/listener.log (together with all other listener modules). The loglevel is thus the same as for all: UCRV listener/debug/level
* One is a TimedRotatingFileHandler that will write to a separate file for each module with a separate loglevel for each module: /v/l/u/listener_module/<name>.log

Two new UCRVs:
* listener/module/<name>/deactivate: bool: deactivate a module (without removing it)
* listener/module/<name>/debug/level: int: set the loglevel for messages going to /v/l/u/listener_module/<name>.log

Both UCRVs require a restart of the listener server. It would be possible to change this. If desired please reopen. It would be inconsistent with the usual behavior of listener modules though.

00951c1c: add improved listener module API and test
334a44c5: add doc strings, only open LDAP connection when used
e164a6c2: user 'listener' unknown before installation of package itself
622002a4: advisory update
228047a7: fix typo
52f9b42b: style fixes (thanks fbest)
76f60003: advisory update

univention-directory-listener 11.0.1-31
ucs-test 7.0.23-72
Comment 8 Daniel Tröder univentionstaff 2017-10-25 22:46:02 CEST
b227ae6b: fix typo
5e465f4a9d: advisory update

univention-directory-listener 11.0.1-32
Comment 9 Daniel Tröder univentionstaff 2017-10-27 10:40:46 CEST
f83142f7: add uldap position object to handler class
b3c7ee9f: impove test coverage

univention-directory-listener 11.0.1-33
ucs-test 7.0.23-75
Comment 10 Daniel Tröder univentionstaff 2017-10-29 04:18:53 CET
caa13c5123: complete metadata attribute list, fix old-new comparison

univention-directory-listener 11.0.1-34
Comment 11 Philipp Hahn univentionstaff 2017-10-30 09:44:10 CET
ucs-test/tests/10_ldap/41listener_module_api_test.py fails:

http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/59/SambaVersion=s3,Systemrolle=slave/testReport/junit/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/59/SambaVersion=s4,Systemrolle=backup/testReport/junit/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/59/SambaVersion=s4,Systemrolle=member/testReport/junit/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/59/SambaVersion=s4,Systemrolle=slave/testReport/junit/10_ldap/41listener_module_api_test/test/

http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/60/SambaVersion=s3,Systemrolle=member/testReport/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/60/SambaVersion=s4,Systemrolle=backup/testReport/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/60/SambaVersion=s4,Systemrolle=slave/testReport/10_ldap/41listener_module_api_test/test/

http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/61/SambaVersion=s4,Systemrolle=master/testReport/10_ldap/41listener_module_api_test/test/
http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-2/job/AutotestJoin/61/SambaVersion=s3,Systemrolle=slave/testReport/10_ldap/41listener_module_api_test/test/
Comment 12 Daniel Tröder univentionstaff 2017-10-30 10:29:05 CET
Already fixed this morning (trying to at least - it's a timing issue of the test): 724c84fb
Comment 13 Daniel Tröder univentionstaff 2017-11-05 00:16:14 CET
Added static type checking. See https://git.knut.univention.de/univention/ucs/blob/4.2-2/management/univention-directory-listener/README.md on how to use it.
Comment 14 Daniel Tröder univentionstaff 2017-11-20 05:44:29 CET
* The code handling LDAP credentials transfer (setdata()) was fixed, so it can handle notifier restarts and LDAP upstream server changes.

* Support for creating asynchronous listener modules using the exact same simple API was added.

* Tests for asynchronous listener modules with concurrency=1 and concurrency=5 were added.

ac81260d: handle notifier restarts, pep8, extract exceptions
b3102c88: add support for creating asynchronous listener modules using the same simple API
d46a10bb: add tests for the asynchronous listener modules API
07eebd16: advisory update

univention-directory-listener 11.0.1-39A~4.2.0.201711200538
ucs-test 7.0.23-94A~4.2.0.201711200543
Comment 15 Daniel Tröder univentionstaff 2017-12-17 22:47:19 CET
* The API was simplified by moving the configuration into the handler class and modifying the modules namespace with the help of a meta class. The configuration class can be a simple class with the required attributes (will suffice in most cases) or a subclass of ListenerModuleConfiguration for advanced scenarios.
* PEP 484 type hints were added to stub files to clean up the imports in the regular Python modules.

A minimalistic listener module now looks like this:

---------------------------------------------------------------------
from __future__ import absolute_import
from univention.listener import ListenerModuleHandler


class ListenerModuleTemplate(ListenerModuleHandler):
    class Configuration:
        name = 'unique_name'
        description = 'listener module description'
        ldap_filter = '(&(objectClass=inetOrgPerson)(uid=example))'
        attributes = ['sn', 'givenName']

    def create(self, dn, new):
        self.logger.debug('dn: %r', dn)

    def modify(self, dn, old, new, old_dn):
        self.logger.debug('dn: %r', dn)
        if old_dn:
            self.logger.debug('it is (also) a move! old_dn: %r', old_dn)
            self.logger.debug('changed attributes: %r', self.diff(old, new))

    def remove(self, dn, old):
        self.logger.debug('dn: %r', dn)
---------------------------------------------------------------------


commit 04000a46a4d28abda799e4bc5dffc493d5dacc6a
    Bug #44786: move logger to handler class, make logger class better mypy parsable
commit b5f0210101bf222147c8b2c5151da7928cba8d8d
    Bug #44786: make celery config update safer
commit 158cb7f7d0d7ce90ab276152bcab6a8257616cda
    Bug #44786: simplify configuration, hide adapter usage
commit b100b8a193f0672e0743a6d9833d34e88e767e92
    Bug #44786: adapt examples
commit d9e42129bca3fa40d37f2048e6fc9fbdf609c6c1
    Bug #44786: adapt tests
commit e64e00bbb92e67ee02b4cf161343185d19e4f9aa
    Bug #44786: move PEP 484 (type hints) to stub files
commit 135ce412cd6e6b4937a4b1358519bf879616f85a
    Bug #44786: changelog
commit 7ad6ddb1253351d3e9bb07eca79bd4e2f077e199
    Bug #44786: update README, fix examples
commit 9c8c206e95bf0a8a9bdf09e40cc7631ba97889df
    Bug #44786: advisory update

Merged and build to:

Branch: ucs_4.2-0
Scope: errata4.2-3

univention-directory-listener (11.0.1-41)
ucs-test (7.0.23-109)
Comment 16 Daniel Tröder univentionstaff 2018-01-09 08:45:31 CET
Merge missing commits to 4.3 branch:

beed68c4     (cherry picked from commit 04000a4)
8d627aaf     (cherry picked from commit b5f0210)
da5cd3ea     (cherry picked from commit 158cb7f)
896db010     (cherry picked from commit b100b8a)
3b3e854d     (cherry picked from commit d9e4212)
6d8e2b7f     (cherry picked from commit e64e00b)
9ec581b1     (cherry picked from commit 7ad6ddb)
f41c23a8     Bug #44786: changelog
Comment 17 Daniel Tröder univentionstaff 2018-01-09 10:08:33 CET
[4.2-3 950b138440] Bug #44786: check type of arguments
[4.2-3 320e7f79e7] Bug #44786: changelog
[4.2-3 d26b7dc020] Bug #44786: fix typo
[4.2-3 110646e341] Bug #44786: changelog
[4.2-3 942a99e75c] Bug #44786: advisory update

univention-directory-listener 11.0.1-43A~4.2.0.201801091005

[4.3-0 d117ed93f2] Bug #44786: check type of arguments
[4.3-0 916003a33b] Bug #44786: changelog
[4.3-0 37a536c0f2] Bug #44786: fix typo
[4.3-0 65e7839bf1] Bug #44786: changelog

univention-directory-listener 12.0.0-12A~4.3.0.201801091002

[4.3-0 58826c2483] Bug #44786: changelog

ucs-test 8.0.7-8A~4.3.0.201801090857
Comment 18 Daniel Tröder univentionstaff 2018-01-12 08:24:07 CET
Patch to massively boost query performance of the main listener process: use an indexed attribute.

[4.2-3 baf9401085] Bug #44786: use indexed attribute
univention-directory-listener 11.0.1-44A~4.2.0.201801120808
---
[4.3-0 328f1d2d01] Bug #44786: use indexed attribute
univention-directory-listener 12.0.0-13A~4.3.0.201801120815
Comment 19 Florian Best univentionstaff 2018-01-19 17:00:34 CET
There is a serious questionable thing in the API. Dirk ran into the problem:

univention.listener.ListenerModuleHandler.po is a @property, which always returns a new property instance.


class Foo(ListenerModuleHandler):
   def create(self):
       self.po.setDn('zoneName=asdf,%s' % (ldap_base,))
       print self.po.getDn() 

→ prints the ldap base instead of the zoneName.

A property() instance should never have side effect. It would be a better API if it would be a method get_position().
Comment 20 Daniel Tröder univentionstaff 2018-01-22 16:41:21 CET
[4.2-3 4494d911801] Bug #44786: exception hierarchy, clear LDAP credentials for server updates, remove UDM code
[4.2-3 3f63021ecd3] Bug #44786: changelog
[4.3-0 638ae0b9b1f] Bug #44786: exception hierarchy, clear LDAP credentials for server updates, remove UDM code
[4.3-0 9501e0b287f] Bug #44786: changelog
[4.2-3 6cc8219eb9b] Bug #44786: fix mypy hints file
[4.3-0 b368ff34f18] Bug #44786: fix mypy hints file

Package: univention-directory-listener
Version: 11.0.1-46A~4.2.0.201801221622
Branch: ucs_4.2-0
Scope: errata4.2-3

Package: univention-directory-listener
Version: 12.0.0-15A~4.3.0.201801221554
Branch: ucs_4.3-0
Comment 21 Daniel Tröder univentionstaff 2018-01-30 10:07:12 CET
[4.2-3 b7ec2f15940] Bug #45968: remove dynamicmaps.cf only if it contains old paths

ucs-test (7.0.23-117)
Comment 22 Daniel Tröder univentionstaff 2018-02-28 10:45:00 CET
To enable the quick release of an erratum the code in this bug has been temporarily deactivated.

@QA: please inform me when I should reactivate the code.

[4.3-0 bcd2b923ac] Bug #44786: temporarily remove new listener module API from binary package
[4.3-0 4b7af540eb] Bug #44786: temporarily deactivate tests for new listener module API
[4.3-0 ebf8490562] Bug #44786: changelog, remove advisory

[4.2-3 fe174c8cfa] Bug #44786: temporarily remove new listener module API from binary package
[4.2-3 1f8df9d975] Bug #44786: temporarily deactivate tests for new listener module API
[4.2-3 bc3b8bb0cd] Bug #44786: changelog, advisory
[4.2-3 cefaa4425d] Bug #44786: advisory update

univention-directory-listener (11.0.1-48)
univention-directory-listener (12.0.0-18)
Comment 23 Arvid Requate univentionstaff 2018-02-28 13:20:37 CET
Ooops, updating the yaml like this results in a dangerous situation:

* check_errata_for_release checks the version number and the status of the bugs referenced by the advisory. It doesn't detect that the package has been rebuilt and this bug is still without QA. As a result we  would nearly have announced it now -- without QA.

That's a flaw in our release procedure. I think we should do it in reverse order in this case: The bug number must not be removed from an advisory before the Bug is verified.
Comment 24 Arvid Requate univentionstaff 2018-02-28 13:24:00 CET
A change like this should happen in a git branch. If git is good for anything in our work flow than it would be this.
Comment 25 Arvid Requate univentionstaff 2018-02-28 13:33:16 CET
In the mean time git history has been re-written, so the commit hashes noted at this bug cannot be used to identify the commits that needed reverting.

What about this one e.g:
=====================================================================
Merge: 29e0b68 219c8e3
Author: Daniel Troeder <troeder@univention.de>
Date:   Wed Oct 25 15:48:01 2017 +0200

    Bug #44786: Merge branch 'dtroeder/44786_listener_api' into 4.2-2
=====================================================================

Please suggest a way to get out of this situation. Quickly.
Comment 26 Arvid Requate univentionstaff 2018-02-28 14:24:58 CET
Ok, as suggested by the assignee I've compared the source packages in the scopes:

arequate@dimma:~$ pkginfo-build2 --published univention-directory-listener \
                | grep ucs_4.2-0-errata
Version:        11.0.1-27A~4.2.0.201704251445   ucs_4.2-0-errata4.2-0

arequate@dimma:~$ pkginfo-build2 univention-directory-listener \
                | grep ucs_4.2-0-errata4.2-3
Version:        11.0.1-48A~4.2.0.201802281040   ucs_4.2-0-errata4.2-3

arequate@dimma:~$ dpkg-source -x /var/univention/buildsystem2/mirror/ftp/4.2/unmaintained/component/4.2-0-errata/source/univention-directory-listener_11.0.1-27A~4.2.0.201704251445.dsc orig

arequate@dimma:~$ dpkg-source -x /var/univention/buildsystem2/apt/ucs_4.2-0-errata4.2-3/source/univention-directory-listener_11.0.1-48A~4.2.0.201802281040.dsc qa

arequate@dimma:~$ LANG=C diff -x changelog -uar orig qa | grep -v ^Only

=> Output looks ok.
Comment 27 Daniel Tröder univentionstaff 2018-03-02 13:11:17 CET
* The new listener module API has been reactivated in the UCS 4.2-3 errata scope and in 4.3-0.
* The async listener module API has been removed from both branches.

[4.3-0 13261bf9a4] Bug #44786: reactivate new listener module api (partial revert of bcd2b923ac)
[4.3-0 d8b74e7104] Bug #44786: reactivate new listener module api test (partial revert of 4b7af540eb)
[4.3-0 19877bf850] Bug #44786: changelog
[4.3-0 1d2d5d5598] Bug #44786: remove async listener module API
[4.3-0 628be71289] Bug #44786: changelog
[4.3-0 c196957f6f] Bug #44786: remove async listener module API tests
[4.3-0 bc9261460d] Bug #44786: changelog

univention-directory-listener (12.0.0-19)
ucs-test (8.0.28-60)

[4.2-3 5033d64434] Bug #44786: reactivate new listener module api (partial revert of bcd2b923ac)
[4.2-3 3aa8c56f23] Bug #44786: reactivate new listener module api (partial revert of bcd2b923ac)
[4.2-3 12567e37b8] Bug #44786: reactivate new listener module api test (partial revert of 4b7af540eb)
[4.2-3 6a9569971f] Bug #44786: remove async listener module API
[4.2-3 900245f364] Bug #44786: changelog
[4.2-3 0a8bb4c269] Bug #44786: advisory

(12567e37b8 was by accident, it is an empty commit.)

univention-directory-listener (11.0.1-49)
ucs-test (7.0.23-120)

* The async listener module API code is available in the branch dtroeder/async_listener_api, which was branched after 19877bf850.

[dtroeder/async_listener_api 8115b6a7b1] Bug #44786: reactivate async listener module API
Comment 28 Dirk Wiesenthal univentionstaff 2018-03-04 22:56:04 CET
Works great in UCS 4.2 and 4.3
Comment 29 Arvid Requate univentionstaff 2018-03-07 12:04:39 CET
<http://errata.software-univention.de/ucs/4.2/311.html>