Skip to content

[monitor-server, monitor-config] fix: preserve Prometheus targets across server restarts - #153

Merged
tardis-key merged 1 commit into
verl-project:mainfrom
mayunaise:fix/prometheus-target-persistence
Sep 1, 2026
Merged

[monitor-server, monitor-config] fix: preserve Prometheus targets across server restarts#153
tardis-key merged 1 commit into
verl-project:mainfrom
mayunaise:fix/prometheus-target-persistence

Conversation

@mayunaise

@mayunaise mayunaise commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #145.

rl-insight server start previously regenerated the runtime prometheus.yml from an empty template and discarded dynamically registered scrape targets. Prometheus then stopped scraping running trainers until they registered again.

This change separates generated Prometheus configuration from dynamic target state:

  • persist dynamic targets under ~/.rl-insight/data/targets/prometheus-targets.yml
  • reference the persisted target file through Prometheus file_sd_configs
  • preserve targets across monitor stack restarts
  • migrate targets from legacy runtime configurations
  • serialize concurrent updates with thread and cross-process locks
  • write configuration and target files atomically
  • keep the existing manual and registration-triggered Prometheus reload behavior
  • load fcntl only in the Linux server-side file-lock path, so Windows training clients can still import and use the public APIs

No public API behavior changes are introduced. The prometheus_reloaded response field and both manual and automatic reload capabilities remain unchanged.

Closes #145.

Checklist Before Starting

Test

ruff check: passed
ruff format --check: passed
compileall: passed
tests/monitor/ut: 60 passed
git diff --check: passed

The fix was also validated against a live verl training job: after restarting the RL-Insight stack, Prometheus rediscovered the persisted trainer, TransferQueue, and vLLM targets without restarting training.

API and Usage Example

No public trainer API changes are required. Existing registration continues to work:

update_prometheus_config(
    ["trainer-host:9092"],
    job_name="trainer_metrics",
)

Registered targets are stored in:

~/.rl-insight/data/targets/prometheus-targets.yml

Design & Code Changes

  • generate a stable Prometheus scrape job backed by file_sd_configs
  • keep dynamic targets outside generated prometheus.yml
  • preserve original job labels through target relabeling
  • migrate legacy dynamically added static_configs
  • preserve existing manual and automatic reload behavior and API semantics
  • keep POSIX file locking server-only
  • document target persistence behavior

Checklist Before Submitting

  • Read the Contribute Guide.
  • Apply Ruff, formatting, compile, and unit test checks.
  • Update documentation.

Comment thread rl_insight/utils/prometheus_utils.py Outdated
Comment thread rl_insight/server/http_api.py Outdated
@mayunaise
mayunaise force-pushed the fix/prometheus-target-persistence branch from 55a3659 to 01671fa Compare August 31, 2026 11:54
@mengchengTang

Copy link
Copy Markdown
Collaborator

Since the collected data is already persisted across restarts, is my understanding correct that this PR specifically addresses the loss of Prometheus scrape targets when RL-Insight is stopped and restarted while a training job is still running?

@mayunaise

Copy link
Copy Markdown
Contributor Author

Since the collected data is already persisted across restarts, is my understanding correct that this PR specifically addresses the loss of Prometheus scrape targets when RL-Insight is stopped and restarted while a training job is still running?

Yes, that is correct. Actually The collected data have been already stored in the TSDB, so Grafana can display them normally; they are not affected by this issue. This PR persists the dynamically registered scrape targets, so when RL-Insight and Prometheus are restarted while a training job is still running, Prometheus can rediscover those targets and continue collecting new metrics without requiring the training job to register again.

@mengchengTang

Copy link
Copy Markdown
Collaborator

[review] Could you please provide examples of both the generated prometheus-targets.yml and the corresponding runtime prometheus.yml? I’d like to understand what the final file structures look like after this change.

@mengchengTang

Copy link
Copy Markdown
Collaborator

[review] Could you clarify the main benefit of introducing file_sd_configs here? Since all monitoring targets are still stored together in a YAML file, would it be simpler to continue writing them directly to prometheus.yml and preserve that file across restarts? The current approach seems to introduce additional label conversion and migration complexity.

@mayunaise

Copy link
Copy Markdown
Contributor Author

[review] Could you please provide examples of both the generated prometheus-targets.yml and the corresponding runtime prometheus.yml? I’d like to understand what the final file structures look like after this change.

Here are examples of the final generated files.
~/.rl-insight/data/targets/prometheus-targets.yml:

- targets:
    - 178.110.22.2:9092
  labels:
    rl_insight_job: trainer_metrics

- targets:
    - 178.110.22.2:38079
  labels:
    rl_insight_job: transfer_queue

- targets:
    - 178.110.22.2:34245
  labels:
    replica: "1"
    rl_insight_job: vllm

- targets:
    - 178.110.22.2:43475
  labels:
    replica: "0"
    rl_insight_job: vllm

The corresponding runtime prometheus.yml:

global:
  scrape_interval: 10s

scrape_configs:
  - job_name: rl-insight-dynamic
    file_sd_configs:
      - files:
          - /root/.rl-insight/data/targets/prometheus-targets.yml
        refresh_interval: 5s
    relabel_configs:
      - source_labels:
          - rl_insight_job
        target_label: job
      - regex: rl_insight_job
        action: labeldrop

Prometheus reads all entries from the targets file every five seconds. The rl_insight_job label is copied to the standard job label and then removed, while labels such as replica are preserved. On restart, RL-Insight regenerates prometheus.yml with the same file_sd_configs reference without overwriting the persisted targets file.

@mayunaise

Copy link
Copy Markdown
Contributor Author

[review] Could you clarify the main benefit of introducing file_sd_configs here? Since all monitoring targets are still stored together in a YAML file, would it be simpler to continue writing them directly to prometheus.yml and preserve that file across restarts? The current approach seems to introduce additional label conversion and migration complexity.

The main motivation is to separate relatively static Prometheus service configuration from dynamic target-discovery state.

  1. The runtime prometheus.yml is generated from the current configuration template and should remain reproducible and replaceable. It is an output of the RL-Insight startup process rather than a persistent state store.
  2. Preserving the generated file could retain stale settings when the user configuration, Prometheus setup, or RL-Insight version changes. The current design regenerates prometheus.yml from the latest template while reconnecting it to the persisted targets file, so new settings take effect without losing targets. Changes to the persistent-state model, such as the data directory, job names, or file schema, still require explicit migration, but migrating the separate targets file is simpler than migrating the complete runtime configuration.
  3. file_sd_configs is Prometheus’s native mechanism for dynamic target discovery. RL-Insight only needs to update the dedicated targets file atomically, while Prometheus monitors that file independently, without repeatedly rewriting its main configuration.
    From a distributed-system perspective, multiple training processes act as target producers and register their endpoints with the RL-Insight server, while Prometheus acts as the consumer:
training processes → RL-Insight target registry → file_sd → Prometheus

This creates a clear responsibility boundary: Prometheus service configuration remains stable and reproducible, while dynamic targets can be updated independently and concurrently. Each registration only modifies a small discovery file instead of coupling every target change to a rewrite of the complete Prometheus configuration.

@mengchengTang

Copy link
Copy Markdown
Collaborator

[review] Could you clarify the main benefit of introducing file_sd_configs here? Since all monitoring targets are still stored together in a YAML file, would it be simpler to continue writing them directly to prometheus.yml and preserve that file across restarts? The current approach seems to introduce additional label conversion and migration complexity.

The main motivation is to separate relatively static Prometheus service configuration from dynamic target-discovery state.

  1. The runtime prometheus.yml is generated from the current configuration template and should remain reproducible and replaceable. It is an output of the RL-Insight startup process rather than a persistent state store.
  2. Preserving the generated file could retain stale settings when the user configuration, Prometheus setup, or RL-Insight version changes. The current design regenerates prometheus.yml from the latest template while reconnecting it to the persisted targets file, so new settings take effect without losing targets. Changes to the persistent-state model, such as the data directory, job names, or file schema, still require explicit migration, but migrating the separate targets file is simpler than migrating the complete runtime configuration.
  3. file_sd_configs is Prometheus’s native mechanism for dynamic target discovery. RL-Insight only needs to update the dedicated targets file atomically, while Prometheus monitors that file independently, without repeatedly rewriting its main configuration.
    From a distributed-system perspective, multiple training processes act as target producers and register their endpoints with the RL-Insight server, while Prometheus acts as the consumer:
training processes → RL-Insight target registry → file_sd → Prometheus

This creates a clear responsibility boundary: Prometheus service configuration remains stable and reproducible, while dynamic targets can be updated independently and concurrently. Each registration only modifies a small discovery file instead of coupling every target change to a rewrite of the complete Prometheus configuration.

I agree that file_sd_configs is valuable for supporting multiple experiments. At the moment, however, targets from all experiments are still stored in a single discovery file, which is not significantly different from keeping them together in prometheus.yml from an experiment-isolation perspective.
Could we add a TODO to consider storing targets for different experiments in separate files and having Prometheus monitor them collectively through file_sd_configs in the future?

@mayunaise

Copy link
Copy Markdown
Contributor Author

Thanks, and your understanding is correct: the current implementation stores all targets in a single discovery file, so it does not yet provide per-experiment isolation. To make sure we’re aligned on the scope of this PR, are you suggesting that this bugfix should only prevent the existing scrape_configs in the runtime prometheus.yml from being overwritten or emptied, while introducing file_sd_configs should be deferred to a follow-up feature? Or would you prefer to keep file_sd_configs in this PR and defer only per-experiment target-file partitioning as a TODO?

@mengchengTang

Copy link
Copy Markdown
Collaborator

@mayunaise I prefer the second option: keep file_sd_configs in this PR, and defer per-experiment target-file partitioning to a follow-up TODO.

@mayunaise

Copy link
Copy Markdown
Contributor Author

@mengchengTang That makes sense. I’ll add a corresponding TODO and leave per-experiment target-file partitioning as a follow-up feature.

@mayunaise
mayunaise force-pushed the fix/prometheus-target-persistence branch from 01671fa to 6554200 Compare August 31, 2026 14:22
@mengchengTang

Copy link
Copy Markdown
Collaborator

approve

@tardis-key
tardis-key merged commit b87bc8e into verl-project:main Sep 1, 2026
8 checks passed
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

Successfully merging this pull request may close these issues.

Prometheus scrape targets are lost after rl-insight server start, leaving all dashboards

4 participants