Skip to content

feat(xmrig): widen the thermal band and fix control-1 membership churn - #4283

Merged
Tanguille merged 4 commits into
mainfrom
feat/xmrig-thermal-response
Aug 1, 2026
Merged

feat(xmrig): widen the thermal band and fix control-1 membership churn#4283
Tanguille merged 4 commits into
mainfrom
feat/xmrig-thermal-response

Conversation

@Tanguille

@Tanguille Tanguille commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Follow-up to #4268. That change delivered the 8x by gating each node independently; this one addresses where the remaining losses actually are.

Where the losses are

Replaying 7d of real guard telemetry through the production DwellPolicy class:

variant c1 c2 c3 replica-h Δ
current (60/64, 600s/120s) 97.8% 44.7% 31.5% 65.4h
c1 recovery dwell 600→180 99.1% 44.7% 31.5% 66.7h +1.3h
c1 recovery limit 50→60 98.5% 44.7% 31.5% 66.4h +0.9h
this PR (62/65, 300s/60s + 68C panic) 97.8% 62.3% 40.4% 84.2h +18.8h (+29%)

control-1 is already 97.8% available over 7d, so its thresholds are unchanged. The drives are the whole lever.

Thresholds and response chain move together

The trip is sized against the chain, so changing one without the other invalidates the margin.

leg before after
guard evaluation interval 60s 30s
trip dwell 120s 60s
KEDA pollingInterval 60s 30s
drain 15s 15s
trip to drain 255s 135s
trip threshold 64C 65C
peak at 1.1C/min vs 70C rating 68.9C (1.1C margin) 67.5C (2.5C margin)

Recovery moves 60C→62C with the dwell halved to 300s: the drives idle in the 60-64C band on warm afternoons, which is what latched them closed while already cool.

New dwell-free 68C panic trip bounds the case where the ramp beats the dwell (75s, ~69.4C), with a matching XmrigGuardThermalPanic critical alert. test_trip_to_drain_budget_fits_the_thermal_margin pins the two Python-owned legs; reverting the interval to 60s fails it.

Bug: mining on control-1 tripped control-1

_new_source_set compared sources positionally. control-1's CPU sample set is (host, presence) plus xmrig only while a miner runs, so every start or stop changed the count and read as tampering:

evaluation failed for control-1: ValueError('source membership changed')

19 fail-closed evaluations in 12h, each draining the miner, which changed the count back. control-2 and control-3 had zero: query_nvme always returns exactly 2 sensors.

Sources are now keyed by identity (host/presence/xmrig, sensor tuples for NVMe), so a genuinely new source is exempt for one cycle while shared keys keep their gap and anti-replay checks. CPUObservation already carried these names; evaluate() was flattening them to a tuple.

Verification

check result
unit tests 23 pass
budget test mutation fails at EVALUATION_INTERVAL_SECONDS = 60, passes at 30
replay reproduces current policy c2 20.0% modelled vs 20.4% actual, c3 8.5% vs 8.5%
committed policy == simulated policy asserted in the replay harness
kubectl kustomize, both dirs clean
kubectl apply --dry-run=server ResourceSet, PrometheusRule, HelmRelease, ServiceMonitor accepted
XmrigGuardThermalPanic expr against live VM parses, 0 results; join binds to control-2 and control-3 with the threshold lowered

Caveat

The +18.8h is an upper bound. It replays a permissive policy against temperatures recorded under the restrictive one, and more mining means more heat. It reproduces the current policy to within 0.4pp per node, which validates the harness, not the counterfactual. Worth confirming against the weekly max Composite before treating the number as delivered.

Also

24h of post-rollout data on #4268: 4.40 running replica-hours against 8.67 solar hours >=25W, 51% capture against 6% before, 3 concurrent miners at peak, no alerts. 1.69 replica-hours were lost to control-2 sitting near 99% memory requests, which is node capacity rather than an autoscaler question.

The completed plan is reworked into docs/xmrig-solar-mining.md as a reference document, per its own process instructions.

Summary by CodeRabbit

  • New Features

    • Added a critical alert for dangerously high NVMe temperatures while mining remains active.
    • Added solar-aware Monero mining documentation covering monitoring, safeguards, alerts, and operating guidance.
  • Bug Fixes

    • Improved thermal protection response, panic handling, and recovery behavior.
    • Improved handling of changing or stale monitoring data with fail-closed safeguards.
  • Performance

    • Reduced monitoring and scaling intervals from 60 to 30 seconds for faster responses.
  • Documentation

    • Removed the outdated per-node mining plan.

Replaying 7d of guard telemetry through DwellPolicy puts the drives, not
control-1, on the critical path: 65.4h -> 84.2h of delivered replica-hours
(+29%), with control-2 44.7% -> 62.3% safe and control-3 31.5% -> 40.4%.

Thresholds move together with the response chain, since the trip is sized
against them. Trip-to-drain is now 135s (30s evaluation + 60s dwell + 30s
KEDA poll + 15s drain) = 2.5C of rise to a ~67.5C peak against a 70C rating,
widening the margin from 1.1C. A dwell-free 68C panic trip bounds the case
where the ramp beats the dwell, with a matching critical alert.

Separately, _new_source_set compared sources positionally, so a miner
starting or stopping on control-1 changed the sample count and read as
tampering: 19 fail-closed evaluations in 12h, each draining the miner, which
changed the count back. Sources are now keyed by identity, so only a
genuinely new source is exempt rather than the whole evaluation going
unchecked.

The replay is an upper bound: it applies a permissive policy to temperatures
recorded under the restrictive one. It reproduces the current policy to
within 0.4pp per node, which validates the harness, not the counterfactual.

Reworks the completed plan into docs/xmrig-solar-mining.md.
@deepsource-io

deepsource-io Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

DeepSource Code Review

We reviewed changes in 1cf2d8f...8eca807 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
JavaScript Aug 1, 2026 10:03p.m. Review ↗
Shell Aug 1, 2026 10:03p.m. Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f0502190-8b4d-4a2b-92aa-bd3b4cc2f4aa

📥 Commits

Reviewing files that changed from the base of the PR and between 6cf7a9f and 8eca807.

📒 Files selected for processing (1)
  • kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Tanguille/LLMKube (auto-detected)
  • Tanguille/toolhive (auto-detected)
  • Tanguille/pr-reviewer-action (auto-detected)
  • Tanguille/2x-R9700-RDNA4-GFX1201-sglang-inference (auto-detected)
🚧 Files skipped from review as they are similar to previous changes (1)
  • kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py

📝 Walkthrough

Walkthrough

The change documents solar-gated XMRig mining and updates per-node polling, thermal thresholds, panic handling, source tracking, evaluation scheduling, and Prometheus alerts.

Changes

XMRig solar gating

Layer / File(s) Summary
Per-node deployment and polling
docs/xmrig-solar-mining.md, kubernetes/apps/web3/monero/xmrig/resourceset.yaml
Documents per-node workloads, telemetry, allocation, measurements, alerts, limitations, and operating constraints. Changes XMRig polling from 60 to 30 seconds.
Guard policy and evaluation timing
kubernetes/apps/web3/xmrig-guard/app/resources/controller.py
Reduces evaluation and HTTP timeout intervals, adds panic-limit handling, updates NVMe policies, and uses deadline-based scheduling.
Identity-keyed source validation
kubernetes/apps/web3/xmrig-guard/app/resources/controller.py, kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
Tracks source timestamps by identity. New XMRig sources are accepted for one cycle while shared sources must remain fresh and ordered. Tests cover source membership, gaps, ordering, and panic recovery.
Thermal alert rules and timing validation
kubernetes/apps/web3/xmrig-guard/app/prometheusrule.yaml, kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
Changes the latched-unsafe recovery threshold to 62°C and adds a critical 68°C thermal panic alert. Tests validate policy configuration and the trip-to-drain timing budget.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EvaluationScheduler
  participant XmrigGuardController
  participant VictoriaMetrics
  participant DwellPolicy
  participant Prometheus
  EvaluationScheduler->>XmrigGuardController: Start evaluation at monotonic deadline
  XmrigGuardController->>VictoriaMetrics: Query telemetry samples
  VictoriaMetrics-->>XmrigGuardController: Return source samples
  XmrigGuardController->>DwellPolicy: Observe aggregated temperature
  DwellPolicy-->>XmrigGuardController: Return safe or unsafe state
  Prometheus->>Prometheus: Evaluate thermal alert rules
Loading

Possibly related PRs

  • Tanguille/cluster#4195: Both changes update XMRig thermal alerting, thresholds, and DwellPolicy behavior.
  • Tanguille/cluster#4268: This change follows the per-node XMRig gating implementation with timing, panic-threshold, and thermal-alert updates.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the two primary changes: widening the XMRig thermal band and fixing control-1 membership churn.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/xmrig-thermal-response

Comment @coderabbitai help to get the list of available commands.

@tanguille-cluster

Copy link
Copy Markdown
@@ spec.resources.keda.sh/v1alpha1/ScaledObject/web3/xmrig-<< inputs.node >>.spec.pollingInterval @@
# fluxcd.controlplane.io/v1/ResourceSet/web3/xmrig
! ± value change
- 60
+ 30

@@ spec.pollingInterval @@
# keda.sh/v1alpha1/ScaledObject/web3/xmrig-control-1
! ± value change
- 60
+ 30

@@ spec.pollingInterval @@
# keda.sh/v1alpha1/ScaledObject/web3/xmrig-control-2
! ± value change
- 60
+ 30

@@ spec.pollingInterval @@
# keda.sh/v1alpha1/ScaledObject/web3/xmrig-control-3
! ± value change
- 60
+ 30

@@ spec.groups.xmrig-guard.rules.rules @@
# monitoring.coreos.com/v1/PrometheusRule/web3/xmrig-guard-rules
! + one list entry added:
+ - alert: XmrigGuardThermalPanic
+   annotations:
+     summary: "{{ $labels.node }} is above the 68C panic trip with a miner still running — the zero-dwell fast path did not shed it and the drive is near its 70C rating"
+   expr: |
+     max by (node) (xmrig_guard_nvme_temp_max_celsius) > 68
+     and on (node)
+     label_replace(
+       kube_deployment_status_replicas{namespace="web3", deployment=~"xmrig-control-.+"} > 0,
+       "node", "$1", "deployment", "xmrig-(.*)"
+     )
+   for: 2m
+   labels:
+     severity: critical

@@ spec.groups.xmrig-guard.rules.rules.XmrigGuardLatchedUnsafe.annotations.summary @@
# monitoring.coreos.com/v1/PrometheusRule/web3/xmrig-guard-rules
! ± value change
- {{ $labels.node }} has been gated unsafe for 6h while its NVMe stayed within the 60C recovery band, so the guard is latched rather than thermally tripped and mining is disabled with no signal
+ {{ $labels.node }} has been gated unsafe for 6h while its NVMe stayed within the 62C recovery band, so the guard is latched rather than thermally tripped and mining is disabled with no signal

@@ spec.groups.xmrig-guard.rules.rules.XmrigGuardLatchedUnsafe.expr @@
# monitoring.coreos.com/v1/PrometheusRule/web3/xmrig-guard-rules
! ± value change in multiline text (one insert, one deletion)
  max_over_time(xmrig_guard_safe[6h]) == 0
  and on (node)
- max_over_time(xmrig_guard_nvme_temp_max_celsius[2h]) <= 60
+ max_over_time(xmrig_guard_nvme_temp_max_celsius[2h]) <= 62

@@ data.controller.py @@
# v1/ConfigMap/web3/xmrig-guard
! ± value change in multiline text (19 inserts, 16 deletions)
  """Small, dependency-free XMRig safety signal controller.
  
  The controller deliberately treats telemetry as untrusted input.  A complete
  set of fresh samples is required before a node can become safe.  Policy is
  
  [29 lines unchanged)]
  
  # scarce watts. It lives here rather than in the manifests because it is a property of the
  # node telemetry, and a future headroom-derived ranking replaces this tuple alone.
  PRIORITY = ("control-1", "control-2", "control-3")
  ENDPOINT = "http://vmauth-victoria-metrics.observability.svc.cluster.local:8427"
- EVALUATION_INTERVAL_SECONDS = 60
+ EVALUATION_INTERVAL_SECONDS = 30
  SOURCE_SAMPLE_MAX_AGE_SECONDS = 120
  MAX_SOURCE_GAP_SECONDS = 120
- HTTP_TIMEOUT_SECONDS = 10
+ # 7 serial queries per CPU evaluation: at 10s a hung VictoriaMetrics blocked 70s against a 30s interval
+ HTTP_TIMEOUT_SECONDS = 5
  
  
  def _dt(value):
      value = float(value)
  
  [eight lines unchanged)]
  
      return 0 <= age <= max_age
  
  
  class DwellPolicy:
-     def __init__(self, recovery_limit, trip_limit, recovery_dwell, trip_dwell, max_gap_seconds=120):
+     def __init__(self, recovery_limit, trip_limit, recovery_dwell, trip_dwell, max_gap_seconds=120, panic_limit=None):
          self.recovery_limit, self.trip_limit = recovery_limit, trip_limit
          self.recovery_dwell, self.trip_dwell = recovery_dwell, trip_dwell
+         self.panic_limit = panic_limit
          self.max_gap = float(max_gap_seconds)
          self.safe = False
          self._last_source = None
          self._pending = None
  
  [14 lines unchanged)]
  
                  return self.safe  # duplicate/out-of-order samples cannot advance dwell
              if gap > self.max_gap:
                  self._pending = self._since = None
          self._last_source = source_seconds
+         # third, dwell-free state: 2C from the rating, confirming the reading over a 60s dwell
+         # would spend most of the margin. Clearing _pending forces a full recovery dwell after.
+         if self.panic_limit is not None and value >= self.panic_limit:
+             self.safe = False
+             self._pending = self._since = None
+             return False
          kind = "recover" if value <= self.recovery_limit else "trip" if value >= self.trip_limit else None
          if kind is None:
              self._pending = self._since = None
              return self.safe
  
  [147 lines unchanged)]
  
  class GuardController:
      def __init__(self, telemetry, clock=time.monotonic, wall_clock=lambda: datetime.now(timezone.utc)):
          self.telemetry = telemetry
          self.clock, self.wall_clock = clock, wall_clock
-         # Trip 64C / recover 60C on Composite, against a 70C drive rating. Mining raises
-         # Composite at up to 1.1C/min, so the 6C band is ~5.5 minutes wide. Worst-case
-         # response is ~4.25: up to one evaluation interval to sample the crossing, 120s
-         # trip dwell, 60s of KEDA polling, then the HPA drop (scaledobject.yaml sheds all
-         # replicas at once for exactly this reason). That leaves the peak near 69C, and is
-         # conservative because it assumes full heat output until the last miner exits.
-         # Idle Composite never exceeded 62C over 7d on either node, so the trip does not
-         # false-fire, and it sits at or below 60C for 100%/90% of the time, so recovery is
-         # reachable rather than the permanent latch the old 60C/70C pair produced.
+         # Trip 65C / recover 62C on Composite, against a 70C drive rating. Mining raises
+         # Composite at up to 1.1C/min. Trip-to-drain is 135s (30s evaluation + 60s dwell + 30s
+         # KEDA poll + 15s drain), so 2.5C of rise to a ~67.5C peak; test_trip_to_drain_budget
+         # pins it. The 68C panic trip is dwell-free and peaks near 69.4C.
+         # The drives idle in the 60-64C band on warm afternoons, so recovery sits at 62C:
+         # replaying 7d puts control-2/3 at 55.0%/38.8% safe against 44.7%/31.5% at 60C/600s.
          # control-1 is keyed the same way on CPU headroom rather than temperature; SENSORS
-         # decides which source feeds which node, so the policy dict needs no special case.
+         # decides which source feeds which node, so the policy dict needs no special case. No
+         # panic limit there: a busy CPU has no equivalent of a drive's absolute rating.
          self.policies = {
              "control-1": DwellPolicy(50, 70, 600, 120, MAX_SOURCE_GAP_SECONDS),
-             "control-2": DwellPolicy(60, 64, 600, 120, MAX_SOURCE_GAP_SECONDS),
-             "control-3": DwellPolicy(60, 64, 600, 120, MAX_SOURCE_GAP_SECONDS),
+             "control-2": DwellPolicy(62, 65, 300, 60, MAX_SOURCE_GAP_SECONDS, panic_limit=68),
+             "control-3": DwellPolicy(62, 65, 300, 60, MAX_SOURCE_GAP_SECONDS, panic_limit=68),
          }
          self.ready = False
          self.metrics = {
              "evaluations": 0, "query_errors": {node: 0 for node in SENSORS},
  
  [two lines unchanged)]
  
              "source_age_seconds": {node: 0.0 for node in SENSORS},
              "cpu_non_xmrig": {node: 0.0 for node in SENSORS if not SENSORS[node]},
              "rank": {node: PRIORITY.index(node) for node in SENSORS},
          }
-         self._last_source_stamps = {node: () for node in SENSORS}
+         self._last_source_stamps = {node: {} for node in SENSORS}
  
      def _new_source_set(self, node, sources):
-         stamps = tuple(source.timestamp.timestamp() for source in sources)
+         # Keyed by source identity, not position: a miner starting or stopping adds or removes
+         # the xmrig source, and comparing by position made that look like tampering. It failed
+         # control-1 closed 19 times in 12h, draining the miner, which changed the set back.
+         # Only keys in both sets are comparable; a genuinely new key is exempt for one cycle
+         # rather than the whole evaluation going unchecked.
+         stamps = {key: source.timestamp.timestamp() for key, source in sources.items()}
          previous = self._last_source_stamps[node]
-         if previous:
-             if len(previous) != len(stamps):
-                 raise ValueError("source membership changed")
-             if any(current - old > MAX_SOURCE_GAP_SECONDS for current, old in zip(stamps, previous)):
-                 raise ValueError("source gap exceeded maximum")
-             if any(current <= old for current, old in zip(stamps, previous)):
-                 return False
+         shared = stamps.keys() & previous.keys()
+         if any(stamps[key] - previous[key] > MAX_SOURCE_GAP_SECONDS for key in shared):
+             raise ValueError("source gap exceeded maximum")
+         if shared and any(stamps[key] <= previous[key] for key in shared):
+             return False
          self._last_source_stamps[node] = stamps
          return True
  
      def evaluate(self, evaluation=None):
  
  [three lines unchanged)]
  
              try:
                  # no sensors means a node with no visible NVMe (control-1, a VM): it is gated
                  # on CPU headroom instead. The dwell policy and metrics are keyed identically.
                  if sensors:
-                     samples = self.telemetry.query_nvme(node, sensors, evaluation)
+                     samples = dict(zip(sensors, self.telemetry.query_nvme(node, sensors, evaluation)))
                      # trip on the hottest drive, date it by the newest sample it was read from
-                     value, stamp = max(item.value for item in samples), max(item.timestamp for item in samples)
+                     value = max(item.value for item in samples.values())
+                     stamp = max(item.timestamp for item in samples.values())
                      self.metrics["nvme_temp_max"][node] = value
                  else:
                      obs = self.telemetry.query_cpu(node, evaluation)
-                     samples = (obs.host, obs.presence) + ((obs.xmrig,) if obs.xmrig else ())
-                     value, stamp = cpu_value(obs), min(item.timestamp for item in samples)
+                     samples = {"host": obs.host, "presence": obs.presence}
+                     if obs.xmrig:
+                         samples["xmrig"] = obs.xmrig
+                     value = cpu_value(obs)
+                     stamp = min(item.timestamp for item in samples.values())
                      self.metrics["cpu_non_xmrig"][node] = value
-                 if not samples or not all(_fresh(item.timestamp, evaluation, SOURCE_SAMPLE_MAX_AGE_SECONDS) for item in samples):
+                 if not samples or not all(_fresh(item.timestamp, evaluation, SOURCE_SAMPLE_MAX_AGE_SECONDS) for item in samples.values()):
                      raise ValueError("stale or future source")
                  policy = self.policies[node]
                  safe = policy.observe(value, stamp, now) if self._new_source_set(node, samples) else policy.safe
-                 self.metrics["source_age_seconds"][node] = max(0.0, evaluation.timestamp() - min(item.timestamp for item in samples).timestamp())
+                 self.metrics["source_age_seconds"][node] = max(0.0, evaluation.timestamp() - min(item.timestamp for item in samples.values()).timestamp())
                  self.metrics["safe"][node] = int(safe)
              except Exception as exc:
                  # one line per failure, no traceback: the query text travels in the exception
                  logging.error(f"evaluation failed for {node}: {exc!r}")
                  self.metrics["query_errors"][node] += 1
                  self.policies[node].invalidate()
-                 self._last_source_stamps[node] = ()
+                 self._last_source_stamps[node] = {}
                  self.metrics["safe"][node] = 0
                  self.metrics["source_age_seconds"][node] = float("nan")
                  if sensors:
                      self.metrics["nvme_temp_max"][node] = float("nan")
  
  [44 lines unchanged)]
  
      _StatusHandler.controller = controller
      server = ThreadingHTTPServer(("0.0.0.0", 8080), _StatusHandler)
      threading.Thread(target=server.serve_forever, daemon=True).start()
      while True:
+         # sleep to a deadline, not a flat interval: sleeping after the work made the true
+         # period drift by the evaluation duration, stretching it against a fixed freshness budget
+         deadline = time.monotonic() + EVALUATION_INTERVAL_SECONDS
          controller.evaluate()
-         time.sleep(EVALUATION_INTERVAL_SECONDS)
+         time.sleep(max(0, deadline - time.monotonic()))
  
  
  if __name__ == "__main__":
      main()

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py (1)

302-306: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Read the deployed timing values in this safety check.

keda_poll and drain duplicate pollingInterval and terminationGracePeriodSeconds from kubernetes/apps/web3/monero/xmrig/resourceset.yaml. A manifest-only change can increase the trip-to-drain time while this test still passes. Load these values from the manifest, or add a separate consistency test that fails when they differ.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py` around
lines 302 - 306, Update test_trip_to_drain_budget_fits_the_thermal_margin so
keda_poll and drain are derived from the deployed xmrig resourceset.yaml values
for pollingInterval and terminationGracePeriodSeconds, or add a dedicated
consistency assertion linking the test constants to those manifest fields.
Ensure the test fails whenever either timing value changes without the safety
calculation being updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py`:
- Around line 302-306: Update test_trip_to_drain_budget_fits_the_thermal_margin
so keda_poll and drain are derived from the deployed xmrig resourceset.yaml
values for pollingInterval and terminationGracePeriodSeconds, or add a dedicated
consistency assertion linking the test constants to those manifest fields.
Ensure the test fails whenever either timing value changes without the safety
calculation being updated.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ea1e128-2cb6-4e0c-abf2-76d60ee32c81

📥 Commits

Reviewing files that changed from the base of the PR and between 1cf2d8f and 30f5741.

📒 Files selected for processing (6)
  • docs/xmrig-per-node-gating-plan.md
  • docs/xmrig-solar-mining.md
  • kubernetes/apps/web3/monero/xmrig/resourceset.yaml
  • kubernetes/apps/web3/xmrig-guard/app/prometheusrule.yaml
  • kubernetes/apps/web3/xmrig-guard/app/resources/controller.py
  • kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Tanguille/LLMKube (auto-detected)
  • Tanguille/toolhive (auto-detected)
  • Tanguille/pr-reviewer-action (auto-detected)
  • Tanguille/2x-R9700-RDNA4-GFX1201-sglang-inference (auto-detected)
💤 Files with no reviewable changes (1)
  • docs/xmrig-per-node-gating-plan.md

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

AI Automated Review (incremental)

Analysis engine: qwen-3.6-fast@http://litellm.ai.svc.cluster.local/v1 (openai)

Recommendation: Approve with minor documentation verification note.

The PR widens the thermal band for XMRig mining nodes and fixes a critical bug in control-1 source membership tracking. The changes are internally consistent, well-tested, and align with repository standards.

Must-Check Verification

  • Verify route access controls are in place: The changed Python controller (controller.py) serves an internal HTTP endpoint for health/status checks (implied by BaseHTTPRequestHandler usage in previous context). No new public-facing routes or API endpoints are exposed to the cluster network or internet. Access is restricted to internal service mesh/namespace boundaries via standard Kubernetes Service resources (unchanged in this diff).
  • Check for unintended public endpoints: No new Services, Ingresses, or Routes are added. The existing xmrig-guard deployment remains internal-only. The ResourceSet changes only affect internal HelmRelease generation for node-specific miners.
  • Review for path traversal vulnerabilities: The diff contains no file system operations, user input parsing for paths, or directory listing logic. The _new_source_set fix uses dictionary keys based on sensor identity tuples, not file paths. No risk identified.
  • Test with edge-case paths (null bytes, symlinks): Not applicable. The codebase handles telemetry metrics and timestamps, not file system paths. The Source dataclass stores floats and datetimes; no string-based path manipulation occurs in the changed logic.

Change Analysis

  1. controller.py:
    • Fixes _new_source_set to key sources by identity (host, presence, xmrig) rather than positional tuple order. This resolves the ValueError('source membership changed') churn on control-1 when the miner starts/stops.
    • Updates thermal thresholds: Trip 64C→65C, Recovery 60C→62C, Dwell times halved (300s/60s), Evaluation interval 60s→30s.
    • Adds a 68C panic trip for NVMe drives to handle rapid ramps that bypass dwell logic.
    • Logic is consistent with the stated goal of reducing false-positive drains while maintaining safety margins against the 70C drive rating.
  2. test_controller.py:
    • Updates tests to verify the new identity-keyed source set logic, including reordering and anti-replay checks.
    • Tests pass according to PR body claims.
  3. resourceset.yaml & prometheusrule.yaml:
    • Manifest updates reflect the new timing parameters (e.g., pollingInterval: 30s).
    • Prometheus rule updated for the new panic alert.
  4. Documentation:
    • docs/xmrig-solar-mining.md updated with new policy rationale and telemetry replay results.

Standards Compliance

  • Follows Conventional Commit title format.
  • Changes are scoped to the xmrig subsystem.
  • No secrets or age keys committed.
  • Shell scripts not touched; Python code follows existing patterns.

Unknowns / Needs Verification

  • Thermal Margin Validation: The PR notes that the +18.8h gain is an upper bound based on permissive policy replay against restrictive temperature data. While the harness validates the current policy within 0.4pp, real-world validation under high-load conditions is recommended post-merge to confirm the 2.5C margin holds at peak ramp rates. This is noted as a caveat in the PR body and does not block merge but warrants monitoring.

The trip-to-drain budget spans four legs, two of them in resourceset.yaml.
Hardcoding pollingInterval and terminationGracePeriodSeconds in the test meant
a manifest-only change could stretch the chain past the thermal margin while
the test still passed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py (1)

282-301: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Extend source-churn coverage through the controller path.

These tests cover an xmrig addition, but not miner removal, reordered NVMe sensor tuples, or GuardController.evaluate(). Add regression cases for those transitions so the start/stop and identity-key contracts are tested outside the private helper.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py` around
lines 282 - 301, Extend the controller-level tests around
GuardController.evaluate() to cover miner removal, reordered NVMe sensor tuples,
and miner start/stop transitions rather than only calling _new_source_set().
Verify source identity keys remain stable when NVMe tuples are reordered, and
preserve the expected start/stop behavior while exercising the public controller
path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py`:
- Around line 282-301: Extend the controller-level tests around
GuardController.evaluate() to cover miner removal, reordered NVMe sensor tuples,
and miner start/stop transitions rather than only calling _new_source_set().
Verify source identity keys remain stable when NVMe tuples are reordered, and
preserve the expected start/stop behavior while exercising the public controller
path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cc19364-b0fb-4c60-9758-686d8290766d

📥 Commits

Reviewing files that changed from the base of the PR and between 30f5741 and d0da77b.

📒 Files selected for processing (1)
  • kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Tanguille/LLMKube (auto-detected)
  • Tanguille/toolhive (auto-detected)
  • Tanguille/pr-reviewer-action (auto-detected)
  • Tanguille/2x-R9700-RDNA4-GFX1201-sglang-inference (auto-detected)

The source-identity tests only exercised the private helper and only covered a
miner arriving. Adds start/stop churn through GuardController.evaluate(), which
is where the fail-closed regression actually surfaced, plus NVMe key stability
under reordering. Reinstating the positional check fails the new test with two
query errors.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py`:
- Around line 312-321: Update test_nvme_source_keys_survive_reordering so the
two sensors use distinct initial timestamps, then create later timestamps by
advancing each sensor’s own timestamp consistently. Keep the later source
mapping in reversed iteration order and retain the repeated-call anti-replay
assertion, ensuring the test validates key-based identity rather than positional
timestamp comparison.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3fe1f458-05a3-4ddc-b785-64974a9d849c

📥 Commits

Reviewing files that changed from the base of the PR and between d0da77b and 6cf7a9f.

📒 Files selected for processing (1)
  • kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Tanguille/LLMKube (auto-detected)
  • Tanguille/toolhive (auto-detected)
  • Tanguille/pr-reviewer-action (auto-detected)
  • Tanguille/2x-R9700-RDNA4-GFX1201-sglang-inference (auto-detected)

Comment thread kubernetes/apps/web3/xmrig-guard/app/resources/test_controller.py Outdated
@Tanguille
Tanguille merged commit 14b78ca into main Aug 1, 2026
15 checks passed
@Tanguille
Tanguille deleted the feat/xmrig-thermal-response branch August 1, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant