cmonitor icon indicating copy to clipboard operation
cmonitor copied to clipboard

CMonitor : Added inotify support to cmonitor.

Open satyabratabharati opened this issue 3 years ago • 2 comments

CmonitorLauncher :

It will perform following steps:

    • Watch all files below a directory and notify an event for changes.
    • Retrieves all the process and extract the process name "/proc//stat.
    • check the process name against the white-list given in the filter list.
    • Execute command to launch CMonitor if the process name matches with the filter.

satyabratabharati avatar Jun 01 '22 04:06 satyabratabharati

Hi @satyabratabharati ,

I think this MR still needs some work. Major points to raise are:

  1. log file: we need this utility to write a log file using Python logging module so that we have each event time-stamped and available for debugging inside a logfile (--log CLI option could be used to define logging destination)
  2. unit tests: it should be possible to write some unit tests for CgroupWatcher asking a test instance to watch under /tmp/cmonitor_unit_test folder and then manipulating the filesystem there (with os.mkdir and writing a "tasks" file)... check out examples under e.g. tools/filter/tests
  3. clear sleep policy: each thread pool (the "watchers" and the "launchers") need to have a clear and well defined sleep policy: they need to drain their input queues (either the Inotify list of pending events or the list of items enqueued on "queue" ) as fast as possible and then return to sleep when all inputs have been processed
  4. function-level documentation, using Python triple quotes embedded docs, at least on public methods of the classes (all non public methods should start with double underscores)
  5. a README / documentation about the whole utility, with usage examples; I typically like to embed usage example in the --help output of each utility, see e.g. cmonitor_filter.py parse_command_line() function
  6. packaging: we need the Makefile "install" target to be able to install this new utility so it will be part of "cmonitor-tools" RPM

Generally speaking I think the design is good anyway: I like the idea of using ThreadPoolExecutor to watch on a folder and then launch (synchronoulsy) cmonitor instances. I admit however that's I should study how ThreadPoolExecutor actually works because I have a lot of doubts: a) the thread running CgroupWatcher in my opinion should be just 1: it's doing very small amount of work (using inotify recursively on a folder) and filtering out non-interesting events. Having more than 1 also is tricky: how do we ensure we never watch the same event (cgroup creation) twice? b) is the "queue" object thread-safe (it should be since Python is using GIL but again I'm not sure) c) why did you use "max_workers=5" ? How does ThreadPoolExecutor decide how many threads to spawn? d) where/how do we set the limit to the max instances of "cmonitor" can be running simultaneously?

f18m avatar Aug 02 '22 13:08 f18m

Hi @satyabratabharati ,

I think this MR still needs some work. Major points to raise are:

  1. log file: we need this utility to write a log file using Python logging module so that we have each event time-stamped and available for debugging inside a logfile (--log CLI option could be used to define logging destination)
  2. unit tests: it should be possible to write some unit tests for CgroupWatcher asking a test instance to watch under /tmp/cmonitor_unit_test folder and then manipulating the filesystem there (with os.mkdir and writing a "tasks" file)... check out examples under e.g. tools/filter/tests
  3. clear sleep policy: each thread pool (the "watchers" and the "launchers") need to have a clear and well defined sleep policy: they need to drain their input queues (either the Inotify list of pending events or the list of items enqueued on "queue" ) as fast as possible and then return to sleep when all inputs have been processed
  4. function-level documentation, using Python triple quotes embedded docs, at least on public methods of the classes (all non public methods should start with double underscores)
  5. a README / documentation about the whole utility, with usage examples; I typically like to embed usage example in the --help output of each utility, see e.g. cmonitor_filter.py parse_command_line() function
  6. packaging: we need the Makefile "install" target to be able to install this new utility so it will be part of "cmonitor-tools" RPM

Generally speaking I think the design is good anyway: I like the idea of using ThreadPoolExecutor to watch on a folder and then launch (synchronoulsy) cmonitor instances. I admit however that's I should study how ThreadPoolExecutor actually works because I have a lot of doubts: a) the thread running CgroupWatcher in my opinion should be just 1: it's doing very small amount of work (using inotify recursively on a folder) and filtering out non-interesting events. Having more than 1 also is tricky: how do we ensure we never watch the same event (cgroup creation) twice? b) is the "queue" object thread-safe (it should be since Python is using GIL but again I'm not sure) c) why did you use "max_workers=5" ? How does ThreadPoolExecutor decide how many threads to spawn? d) where/how do we set the limit to the max instances of "cmonitor" can be running simultaneously?

Hi @f18m ,

I tried to implement all the points what you have suggested. Except for the unit test case from the above list. I will work on it.

Following points have been incorporated :

  1. logger for the logging events. example.log
  2. Done.
  3. regarding sleep policy, added sleep when the Queue is empty and there is no events to process. The sleep parameter can be provided now from the command line as a parameter. The default is set to 20 sec. The 20 sec sleep also used before processing the files ( meaning before applying whilte-list filter logic) , just to allow the task files to be created before applying the filter logic and store it to the Queue.
  4. Added function level comments as well for the classes.
  5. Added as suggested.
  6. Done.

Thank You.

satyabratabharati avatar Aug 19 '22 10:08 satyabratabharati