Bug 31752 - UMC server does not reload UCR variables due to error in pyinotify.WatchManager
UMC server does not reload UCR variables due to error in pyinotify.WatchManager
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC (Generic)
UCS 3.1
Other Linux
: P5 normal (vote)
: UCS 3.2
Assigned To: Florian Best
Alexander Kläser
: interim-1
: 29781 32825 (view as bug list)
Depends on:
Blocks: 30811 33192
  Show dependency treegraph
 
Reported: 2013-06-17 16:43 CEST by Janis Meybohm
Modified: 2013-11-19 06:43 CET (History)
4 users (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
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
Simple test script (226 bytes, text/x-python)
2013-06-17 16:43 CEST, Janis Meybohm
Details
inotify.patch, cleanup (3.65 KB, patch)
2013-06-19 09:51 CEST, Florian Best
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Janis Meybohm univentionstaff 2013-06-17 16:43:41 CEST
Created attachment 5277 [details]
Simple test script

pyinotify raises an OSError if one of the inotify limits is hit (max_user_instances in this case - I guess).

As the limits are user wide, this may lead to crashing umc modules or server.  Implementation in:
univention-management-console/src/univention/management/console/protocol/server.py
Comment 1 Florian Best univentionstaff 2013-06-19 09:51:05 CEST
Created attachment 5279 [details]
inotify.patch, cleanup

Some cleanup from my working copy. Patch does not let UMC-Server crash.
Comment 2 Dirk Wiesenthal univentionstaff 2013-06-20 16:16:33 CEST
I may be wrong but I think there was another bug with "delayed" results when requesting UCR variables that changed (very) recently.

I think we should remove the complete pyinotify part and just ucr.load() before each UCR variable request. ucr.load() is relatively fast and requesting UCR variables happens not very often (maybe once a minute when opening a UDM module?).

I did not look into the code but this is what the inotify thing does, no? Keeping UCR variables up to date? The inotify overhead and the ucr.load() overhead may be equal and the latter is less error prone.
Comment 3 Florian Best univentionstaff 2013-06-20 16:47:49 CEST
(In reply to Dirk Wiesenthal from comment #2)
> I may be wrong but I think there was another bug with "delayed" results when
> requesting UCR variables that changed (very) recently.
> 
> I think we should remove the complete pyinotify part and just ucr.load()
> before each UCR variable request. ucr.load() is relatively fast and
> requesting UCR variables happens not very often (maybe once a minute when
> opening a UDM module?).
> 
> I did not look into the code but this is what the inotify thing does, no?
> Keeping UCR variables up to date? The inotify overhead and the ucr.load()
> overhead may be equal and the latter is less error prone.
We had this before (reloading every X seconds instead of only reloading if changes occured) and ran into trouble...
Comment 4 Dirk Wiesenthal univentionstaff 2013-06-20 18:09:41 CEST
(In reply to Florian Best from comment #3)
> We had this before (reloading every X seconds instead of only reloading if
> changes occured) and ran into trouble...

I can imagine that. That is why I suggest reloading it at every umcp/ucr request, regardless if someone changed UCR variables before.
Comment 5 Florian Best univentionstaff 2013-07-02 10:20:14 CEST
(In reply to Dirk Wiesenthal from comment #4)
> (In reply to Florian Best from comment #3)
> > We had this before (reloading every X seconds instead of only reloading if
> > changes occured) and ran into trouble...
> 
> I can imagine that. That is why I suggest reloading it at every umcp/ucr
> request, regardless if someone changed UCR variables before.

univention.management.console.config provides a global UCR instance (the instance which is realoaded automatically). This is used and relies on it in various (at least 3) places of the UMC-Server.
Comment 6 Florian Best univentionstaff 2013-07-02 12:55:26 CEST
The autoload of UCR variables is currently not working, so i suggest to remove the code completely.
Comment 7 Alexander Kläser univentionstaff 2013-07-02 17:00:22 CEST
(In reply to Dirk Wiesenthal from comment #4)
> I can imagine that. That is why I suggest reloading it at every umcp/ucr
> request, regardless if someone changed UCR variables before.

+1 ... there are usually not many UCR request

For what else than umcp/ucr is the reload relevant?

(In reply to Florian Best from comment #6)
> The autoload of UCR variables is currently not working, so i suggest to
> remove the code completely.

+1
Comment 8 Dirk Wiesenthal univentionstaff 2013-07-02 17:28:06 CEST
*** Bug 29781 has been marked as a duplicate of this bug. ***
Comment 9 Alexander Kläser univentionstaff 2013-07-15 12:29:30 CEST
This bug currently blocks the dialog with information about anonymous user statistics (Bug 30811). Currently, I can switch anonymous user statistics off and after the next login, it is still enabled.

It should be fine to reload the internal UCR cache after each login and at each UCR request.
Comment 10 Florian Best univentionstaff 2013-07-16 16:34:39 CEST
The inotify mechanism has been removed. On login and when doing GET/ucr and GET/info UMCP-Commands the UCR variables are reloaded.

univention-management-console (6.0.4-1)
Comment 11 Alexander Kläser univentionstaff 2013-07-29 15:55:03 CEST
QA: I removed the line

> src/univention/management/console/protocol/server.py:import pyinotify

as there is no reference to pyinotify anymore.

Changes are fine. Changelog entry has been added.
Comment 12 Alexander Kläser univentionstaff 2013-08-21 10:31:49 CEST
I guess the pre-dependency to pyinotify in debian/control can also be removed:
====================
Package: python-univention-management-console
...
Pre-Depends:
 python-pyinotify
====================
Comment 13 Florian Best univentionstaff 2013-08-21 10:34:16 CEST
(In reply to Alexander Kläser from comment #12)
> I guess the pre-dependency to pyinotify in debian/control can also be
> removed:
> ====================
> Package: python-univention-management-console
> ...
> Pre-Depends:
>  python-pyinotify
> ====================
Yes, it has been removed. univention-management-console (6.0.8-2)
Comment 14 Alexander Kläser univentionstaff 2013-08-21 10:42:47 CEST
(In reply to Florian Best from comment #13)
> Yes, it has been removed. univention-management-console (6.0.8-2)

Looks good now → VERIFIED
Comment 15 Alexander Kläser univentionstaff 2013-10-09 13:06:27 CEST
*** Bug 32825 has been marked as a duplicate of this bug. ***
Comment 16 Stefan Gohmann univentionstaff 2013-11-19 06:43:31 CET
UCS 3.2 has been released:
 http://docs.univention.de/release-notes-3.2-en.html
 http://docs.univention.de/release-notes-3.2-de.html

If this error occurs again, please use "Clone This Bug".