Fix - Race condition when trying to add a log file that's been scheduled for removal

[alesnovak-s1]

• Fixes https://sentinelone.atlassian.net/browse/DTIN-4631
• __lock has been change to reentrant RLock.

Based on the comments the lock was initially meant to lock status fields used in generate_status.
Now it ensures synchronized access to fields needed for asynchonous workflow of log files.
This is already causing a timeout when retrieving status from the agent (https://sentinelone.atlassian.net/browse/DTIN-4769) and will be handled in another PR.

The Bug:

When a log file (path, worker_id) is scheduled for removal, the removal is done in two separate steps:

• __remove_logs_scheduled_for_deletion
  • Instructs all matchers from __logs_pending_removal to finish
  • Clears __logs_pending_removal
• __purge_finished_log_matchers
  • Removes all finished matchers from __logs_pending_removal, __dynamic_matchers, __dynamic_paths

The race condition can be reached when a path is scheduled for removal and the matchers are still finishing.
When add_log_config is called it considers log path to be active and skips adding it. The log file is later removed.

In log we can see sequence like this one:

Log ( log_path='xy', worker_id='default' ) for monitor 'scalyr_agent.builtin_monitors.kubernetes_monitor' is pending removal

Tried to add new log file 'xy' for monitor 'scalyr_agent.builtin_monitors.kubernetes_monitor', but it is already being monitored by 'scalyr_agent.builtin_monitors.kubernetes_monitor'

Removing log file ( log_path='xy', worker_id='default' ) for 'scheduled-deletion'

[github-actions]

Test Results

     20 files  ±  0       20 suites  ±0   31m 28s ⏱️ -11s
1 497 tests +  8  1 477 ✔️ +  8    20 💤 ±0  0 ❌ ±0 
7 156 runs  +40  6 929 ✔️ +40  227 💤 ±0  0 ❌ ±0 

Results for commit 423aea8. ± Comparison against base commit c189190.

♻️ This comment has been updated with latest results.

[jmakar-s1]

Up to you but I think this is ok: return self.__paths.get(path, {}).items()

[alesnovak-s1]

Yes, definitely!

[jmakar-s1]

Don't we need to be careful that path exists in self.__paths?
Maybe better to be value = self.__paths.get(path, {}).pop(worker_id, default) or something similar

[alesnovak-s1]

self.__path is a defaultdict. But this way it's not consistent and a bit confusing. I will ditch the defaultdict, use this way and modify a set method as well so it's obvious what's expected to happen.

[jmakar-s1]

Similarly this could be if path in self.__paths and not self.__paths[path]:

[alesnovak-s1]

It will work because of defaultdict, but it's confusing, I will change it. Thanks!

[jmakar-s1]

Looks good. One consideration for future work, to make the use of the lock clearer could use with self.__lock: to avoid having to explicitly release it