Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python scripts is not reinitialized during SIGHUP #1370

Closed
mitzkia opened this issue Mar 7, 2017 · 2 comments
Closed

python scripts is not reinitialized during SIGHUP #1370

mitzkia opened this issue Mar 7, 2017 · 2 comments

Comments

@mitzkia
Copy link
Contributor

mitzkia commented Mar 7, 2017

  • I would expected from syslog-ng to reinitialize already processed python script during SIGHUP
  • syslog-ng correctly reinitialize python script only if it is embedded to syslog-ng configuration

Reproduction:

  • create example.py with the following content
class DummyPythonScript(object):
  def init(self, options):
    print("init")
    return True

  def send(self, msg):
    print("send")
    return True
  • start syslog-ng with the given config (in foreground mode)
@version: 3.9
source s_internal { internal();};
source s_system { system();};
destination d_python {python(class('example.DummyPythonScript') );};
log {
	source(s_internal);
	source(s_system);
	destination(d_python);
};
  • modify python script with some new print("test message")
  • reload syslog-ng
  • you should notice that the newly added print() statement is not logged to console
@mitzkia mitzkia assigned mitzkia and kvch and unassigned mitzkia Mar 27, 2017
@presidento presidento added the bug label Jun 23, 2017
@furiel furiel self-assigned this Nov 10, 2017
@furiel
Copy link
Collaborator

furiel commented Nov 29, 2017

Just a little summary on the progress on this topic:

The documentation does not state that we should recompile user code during reload. This one is the closest one to it:
"[The Python object is initiated only once, when syslog-ng OSE is started or reloaded.]"(https://www.balabit.com/documents/syslog-ng-ose-latest-guides/en/syslog-ng-ose-guide-admin/html/python-parser.html)
In strict terms, this means we just reinitialize the same object, not mentioning recompiling user code. In reality I suspect more happens: in _py_init_bindings I think we create a new python object, the former one is not used (left for garbage collector). If we implement this, the documentation would worth updating that reload will also recompile the user code.

As for the implementation: python does not support unimporting modules. At least there is no api call for it. One might think if we drop a reference count, that would trigger the garbage collector, but the thing is when you import a module, the reference counter of the returned module object will be 2 instead of 1. The reason is an additional reference is set, due to the module object is added to the _builtin_ list of modules automatically. Seeing the extra logic here, I feel manual reference dropping is a little risky.

An alternative could be is: we could stop the whole python machinery, and recreate it after reload. A downside of this that it would disable such techniques we use in syslog-ng core: like persist-config. Users might want to save data across reloads, which could not be achieved if the python machinery is stopped in the middle.

The good news is: reimporting modules is supported by the c api PyObject* PyImport_ReloadModule(PyObject *m). I tried this, and it works as expected, the modified user code is activated after reload. We could store the module object in the PythonDestDriver class: create it with PyObject* PyImport_Import(PyObject *name), reload it with PyImport_ReloadModule.
Though this one also has its own risks: I am a little afraid of the garbage collection of the objects created in the previous module. If the original objects are not freed, it could lead hard (python machinery is not really valgrind friendly) to debug memory leaks after each reload (logrotate). This is to be explored next.

@furiel furiel removed their assignment Mar 29, 2018
@furiel
Copy link
Collaborator

furiel commented May 17, 2018

I was tinkering with this one a little bit more, and it seems my patch had an unwanted side effect, it broke the persistence: with the following config

from mem_top import mem_top

a = 0

class DummyPythonScript():
    def init(self, options):
        global a
        a += 1
        print "initialized: {}".format(a)
        return True
    def send(self, message):
        print mem_top(verbose_types=[dict, list], verbose_file_name="/tmp/detailed_log.txt")
        print "send method: " + message["MSG"]
        return True

the original code prints increasing values of a after each reload, but constant 1 in case of my patch (unsurprisingly). I don't know how could users persist their objects between reloads with my version. If we plan to fix this issue, we need to ensure users can persist their objects in the end.

Another topic: I found this exciting module: mem_top which can be installed from pip. This module can help to check if we leak python objects in our module. One can use to check leak problems with reloads.

@MrAnno MrAnno removed the bug label Feb 24, 2023
@MrAnno MrAnno closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants