Conversation
Add --notify-script, --server-name, and --notify-level CLI args.
On each kill/stop action (and optionally on recovery) the sentinel
calls the given script via subprocess with MONIT_HOST/SERVICE/DESCRIPTION
env vars, matching the monit_clickup_notifications.py convention.
State file format updated to store full tier objects ({type, name})
instead of bare name strings, with backwards-compatible load_state().
cgbautista
left a comment
cgbautista
left a comment
There was a problem hiding this comment.
There are an extreme case that is ignored: having no free memory at all or failing to retrieve free memory (that is probably one symptom that there is no free memory).
Also, the monit service names, even though they are checked and issue a warning, do not prevent the oom-monitoring service from starting when it will be unable to do anything in the worst-case scenario.
| if trigger_ram is not None | ||
| else f"RAM after stop: {ram_after} MB." | ||
| ) | ||
| notify(f"{tier['type'].upper()} {name}: service stopped. {ram_info}") |
There was a problem hiding this comment.
There is no actual check on the stop_tier() return, so it may report that some mispelled/non-existant service was stopped.
Also, at least for the stop command, monit may require some time to render the service stopped (it is a background process), so I would rather say "sent a {tier["type"]} signal to the service" as it may even fail to stop at all.
|
|
||
| for tier in cfg.tiers: | ||
| available_ram = stop_tier_and_settle(cfg, tier, stopped_tiers, trigger_ram=available_ram) | ||
| if available_ram > 0 and available_ram >= cfg.emergency_trigger: |
There was a problem hiding this comment.
As in the other case, 0 should be treated as a critical condition.
| last_log = 0.0 | ||
|
|
||
| while available_ram < cfg.safe_recovery: | ||
| if 0 < available_ram < cfg.emergency_trigger and remaining_tiers: |
There was a problem hiding this comment.
Even though available_ram would not actually be 0.000 in most scenarios, it is technically possible to have that value returned.
I understand that this 0 < available_ram part is designed for the "failure" of the get_available_memory_mb() function and not for the unlikely real 0mb-free scenario, assuming that not being able to retrieve the free memory is something "that may happen at any time", so any action to free ram is not necessary. But in the current code (calling free -m in a subprocess) that function would probably fail in a high memory pressure scenario as forking the new process would likely be refused by the OS.
IMO, it is safer to assume that the failure of the get_available_memory_mb() is equivalent to the 0mb-free scenario and remove the 0 < part of the condition.
| while True: | ||
| try: | ||
| available_ram = get_available_memory_mb() | ||
| if 0 < available_ram < cfg.emergency_trigger: |
There was a problem hiding this comment.
As in the other case, 0 should be treated as a critical condition.
| def run_cmd(cmd, shell=False, timeout=None): | ||
| try: | ||
| if isinstance(cmd, str) and not shell: | ||
| cmd = cmd.split() |
There was a problem hiding this comment.
Should use instead:
import shlex
[...]
cmd = shlex.split(cmd)to ensure that any parameter that may contain spaces is treated as a whole and not just splited in half (i.e. run_cmd(f"monit status {monit_name}") where monit_name contains spaces)
|
|
||
| def get_available_memory_mb(): | ||
| try: | ||
| output = run_cmd("free -m") |
There was a problem hiding this comment.
Instead of running a command (as it requires to create a new process and it demands extra memory, which could be blocked in OOM situations), it would be better to read /proc/meminfo directly:
with open("/proc/meminfo", "r") as f:
for line in f:
if line.startswith("MemAvailable:"):
# meminfo outputs in KB, convert it to MB instead
return int(line.split()[1]) // 1024
raise RuntimeError("MemAvailable couldn't be found in /proc/meminfo")| def wait_for_ram_settle(threshold_mb, max_seconds=SETTLE_SECONDS): | ||
| """Polls RAM for up to max_seconds. Returns available RAM when stable or timeout.""" | ||
| deadline = time.monotonic() + max_seconds | ||
| while time.monotonic() < deadline: |
There was a problem hiding this comment.
Just to avoid the extra call to get_available_memory_mb() as you are already extracting the value in the loop. The ram > 0 part seems unnecesary as it is already implied in the other comparison.
while True:
ram = get_available_memory_mb()
if ram >= threshold_mb or time.monotonic() >= deadline:
return ram
time.sleep(CHECK_INTERVAL_SECONDS)| ), | ||
| ) | ||
| parser.add_argument( | ||
| "--notify-script", |
There was a problem hiding this comment.
After setting up the notify script, I would make sure it exists as a file to issue a warning and disable notifications (or refusing to start with a wrong argument, as it could be removed)
| env["MONIT_SERVICE"] = "RAM-SENTINEL" | ||
| env["MONIT_DESCRIPTION"] = f"[{level.upper()}] {description}" | ||
| subprocess.run( | ||
| ["python3", _NOTIFY_SCRIPT], |
There was a problem hiding this comment.
I would rather assume that the notification script has a shebang and execution permissions than to assume it is a python3 script.
…ee -m Spawning `free -m` requires forking a new process, which the kernel may refuse under heavy memory pressure, leaving the sentinel unable to read available memory exactly when it is needed. Read /proc/meminfo directly instead; the value is the same one `free` reports as "available". On failure the function still returns 0, which will be treated as a critical condition by the callers.
The `0 < available_ram` guards skipped both a real 0 MB reading and a failed memory read. A failed read is most likely a symptom of the OOM condition itself, so ignoring it left the host unprotected in the worst case. Remove the guard in the main loop, the escalation loop and the wait/restore loop so that 0 always triggers (or continues) recovery.
The snapshot is logged right when the emergency trigger fires, before any tier is stopped. Running `free -m` and a `ps | head` shell pipeline at that moment requires several forks that may fail or hang under memory pressure, delaying the recovery. Read /proc/meminfo and /proc/<pid>/status (VmRSS) directly to log the memory summary and the top processes by RSS.
At runtime a failed memory read is treated as 0 MB (critical). If the read fails permanently because of the platform (no /proc mounted, kernel without MemAvailable), that would stop every tier right after startup and never restore them, since 0 never reaches --safe-recovery. Split the reader into read_mem_available_mb(), which raises on failure, and get_available_memory_mb(), which returns 0 for runtime use. Call the raising version once at startup and exit with an error if it fails. Only readability is checked, not the value, so a restart during a real 0 MB situation still starts the sentinel.
Check both exit conditions (threshold reached or deadline passed) in a single `if`, so each iteration reads available memory once and the function always returns the value it has just checked. This removes the extra read after the timeout and the redundant `ram > 0` guard, which is implied by `ram >= threshold_mb`.
…ames Use shlex.split() instead of str.split() in run_cmd() so quoted arguments are kept together, and quote the Monit service name with shlex.quote() where it is interpolated into monit commands (shlex.split alone would still split an unquoted name). Monit service names are single tokens, so this is not expected to change behaviour with valid tier names; it only makes argument handling robust.
…y sent
stop_tier() returned nothing, so a notification saying "service stopped"
was sent even when the tier name was wrong, no PID was found or the
SIGKILL failed. It also claimed the service was stopped although
`monit stop` only queues the request in the Monit daemon.
stop_tier() now returns (sent, action) and the notification reports
either the action sent ("SIGKILL sent to PID N", "stop request sent to
Monit") or a FAILED message. Failed tiers are still recorded as stopped
because they were already unmonitored and must be re-monitored on
restore.
…valid Tiers not found in `monit summary` were only logged as a warning, so a misspelled tier still cost 20-50 s of recovery time (unmonitor, stop timeout, settle wait) and a sentinel with no valid tier kept running while being unable to act. validate_tiers() now returns only the tiers known to Monit and replaces cfg.tiers with them. The sentinel exits with an error if no valid tier is left or if Monit can't be queried, since every action goes through Monit. systemd's Restart=always retries until Monit is available.
- Refuse to start if --notify-script is set but the file doesn't exist or isn't executable, so broken alerting is caught at deploy time instead of silently failing during an emergency. - Execute the script directly instead of through `python3`, so any language with a shebang works (same convention as Monit's exec). - Log a warning with stderr when the script exits with a non-zero code, which was previously ignored. README and --help updated with the shebang/execute permission requirement.
Compute the shell-quoted Monit service name once at the top of stop_tier() and reuse it in the unmonitor/stop commands instead of calling shlex.quote() on every command.
A tier whose action couldn't be sent (no live PID, SIGKILL failed or monit stop failed) still went through the 3 s reclaim sleep and the settle wait (up to 20 s), losing ~23 s while RAM is critical and nothing has been freed. Skip both waits when stop_tier() reports a failure so recovery moves on to the next tier right away.
A `monit stop` that times out was reported as a failure and, since the previous commit, skipped the settle wait. Monit may still be stopping the service in that case, so escalating immediately could stop the next tier unnecessarily. run_cmd()/maybe_run() get a raise_on_timeout flag so stop_tier() can tell a rejected request (error exit, escalate right away) from a timeout (reported as "result unknown", settle wait applies as usual).
run_cmd() only caught TimeoutExpired and CalledProcessError. If the kernel refuses to create the child process (OSError ENOMEM, the typical OOM situation) or the binary is missing, the exception propagated up to the main loop and aborted the whole recovery: remaining tiers were not tried and the next iteration restarted from the first tier with an empty stopped list, overwriting the state file. Catch OSError and return None like any other command failure, so the tier is reported as failed and recovery escalates to the next one.
notify() waited for the notification script (up to 15 s, it usually makes an HTTP call) in the middle of the recovery, delaying the escalation to the next tier even when the current one had failed. Run the script in a daemon thread so recovery continues immediately. The thread still waits for the script, so non-zero exit codes and errors keep being logged.
stopped_names is only used to build remaining_tiers before the loop; adding to it inside the loop had no effect.
The only shell=True caller was the `ps | head` pipeline removed when the system snapshot moved to /proc. Removing the parameter also ensures no command is ever passed through a shell.
…itical Once every tier has been stopped, the sentinel just waits for RAM to recover, logging every 30 s but sending no alert. If RAM is still below the emergency trigger at that point, send a single critical notification so someone can step in manually.
Resuming a pending recovery at startup ran outside any try block, so an unexpected error crashed the sentinel and systemd restarted it in a loop, leaving the host unprotected. Log the error like the main loop does and continue; the state file is only cleared after a successful restore, so the pending tiers are not lost.
Replies to reviewGeneral comment (no free memory / tiers not preventing startup)Both covered. A failed memory read is now treated as 0 MB (critical), and the sentinel refuses to start if no valid tier is left (see the replies below).
|
|
@cgbautista thanks for the review, you found some critical improvements. |
2 participants
📌 References
Issue: https://app.clickup.com/t/4528615/869e36g3p
📝 Implementation
Adds
ram_sentinel.py, a daemon that monitors available RAM and stopsMonit-managed services when memory drops critically low, then restores
them once RAM recovers. Configured per-environment via systemd ExecStart
flags — no code changes needed between servers.
Key design decisions:
kill(SIGKILL via Monit PID, for Java/Tomcat)and
stop(clean monit stop, for Docker/Postgres)is resumed on daemon restart
--dry-runmode reads RAM and PIDs for real but skips all destructivecommands
How to install
ram_sentinel.pyto/usr/local/bin/ram_sentinel.pyforce an emergency cycle and verify tier names and PID resolution:
python3 /usr/local/bin/ram_sentinel.py
--emergency-trigger <above_current_available_MB to test>
--safe-recovery <target_MB>
--tier kill:<monit_name>
--dry-run
[DRY-RUN] would...foreach action
.servicefile for the server to/etc/systemd/system/ram-sentinel.servicesystemctl daemon-reload && systemctl enable --now ram-sentinel
journalctl -u ram-sentinel -f