|
| 1 | +#### What type of PR is this? |
| 2 | +Bug fix |
| 3 | + |
| 4 | +#### What this PR does / why we need it: |
| 5 | + |
| 6 | +Following the official demo |
| 7 | +([QoS Interference Detection And Active Avoidance](https://gocrane.io/zh-cn/docs/tutorials/colocation-with-enhanced-qos/qos-interference-detection-and-active-avoidance.zh/)), |
| 8 | +the `EnsuranceAnalyzedPressure:PreferNoSchedule` node taint added by an |
| 9 | +`AvoidanceAction` (e.g. `disablescheduling`) is **never removed** after the |
| 10 | +metric drops back below the watermark — users expect it to be cleared once |
| 11 | +`coolDownSeconds` has elapsed. |
| 12 | + |
| 13 | +**Root cause — two cooperating defects in `pkg/ensurance/analyzer/analyzer.go`:** |
| 14 | + |
| 15 | +1. **One-shot restore.** `computeActionContext` fired `Restored` exactly |
| 16 | + once: it reset the trigger counter at the very moment the restore count |
| 17 | + reached `restoreThreshold`. The cool-down was therefore evaluated only at |
| 18 | + that single instant. With the usual defaults (`restoreThreshold: 2`, |
| 19 | + probe interval ~10s), that instant arrives ~20s after the usage recovers — |
| 20 | + almost always *inside* the `coolDownSeconds` window (60s in the reporter's |
| 21 | + YAML), so the merge step kept the node disabled, and since `Restored` |
| 22 | + never fired again, the taint stayed forever. |
| 23 | +2. **Cool-down never elapses.** While waiting out the cool-down, |
| 24 | + `mergeSchedulingActions` kept calling `ToggleScheduleSetting(true)`, which |
| 25 | + refreshed `lastTriggeredTime` on *every* analysis round — the cool-down |
| 26 | + start moved forward each round, so `now > lastTriggeredTime + |
| 27 | + coolDownSeconds` could never become true. |
| 28 | + |
| 29 | +**Fix (no API changes):** |
| 30 | + |
| 31 | +- `computeActionContext`: the restore now stays **pending** while the usage |
| 32 | + remains below the watermark — the trigger/restore counters are no longer |
| 33 | + reset when `Restored` first fires, so the cool-down is re-evaluated on |
| 34 | + every analysis round. |
| 35 | +- `mergeSchedulingActions`: when the cool-down finally elapses and the |
| 36 | + restore is executed (`ToBeDisable=false, ToBeRestore=true`), the counters |
| 37 | + are cleared so the rule stops firing and can trigger again on the next |
| 38 | + interference. |
| 39 | +- `lastTriggeredTime` is now refreshed **only** by real trigger rounds |
| 40 | + (`ac.Triggered`), so the cool-down always starts from the last actual |
| 41 | + breach. |
| 42 | +- New `recordRestoreDecision` audit log: one structured line per scheduling |
| 43 | + decision (`action`, `rule`, `nodeQOS`, `direction=enable|disable`, |
| 44 | + `coolDown`, `lastTriggered`, `now`), so the restore path is observable in |
| 45 | + production logs without a debugger. |
| 46 | + |
| 47 | +After `coolDownSeconds` elapses, `ScheduleExecutor.Restore` runs and removes |
| 48 | +the taint plus the node condition, exactly as the reporter expected. |
| 49 | + |
| 50 | +**Verification:** |
| 51 | + |
| 52 | +- New unit tests in `pkg/ensurance/analyzer/analyzer_test.go` (all pass, |
| 53 | + `go test ./pkg/ensurance/...` green, full-repo `go build ./...` green): |
| 54 | + - `TestComputeActionContext_RestoreStaysPendingUntilExecuted` — core |
| 55 | + regression: restore stays pending until executed, stops after, and the |
| 56 | + rule re-triggers on the next interference. |
| 57 | + - `TestMergeSchedulingActions_CoolDownNotExtendedWhileRestoring` — |
| 58 | + cool-down clock is not refreshed while restoring; restore executes after |
| 59 | + the cool-down and clears the counters. |
| 60 | + - `TestMergeSchedulingActions_SingleRestoredPastCooldownEnables` — the |
| 61 | + reporter's exact scenario (one NodeQOS `watermark1`, one action |
| 62 | + `disablescheduling` with `coolDownSeconds: 60`). |
| 63 | + - `TestMergeSchedulingActions_TriggeredAfterCooldownDisables` — a |
| 64 | + concurrently triggered rule still keeps the node disabled. |
| 65 | + - `TestMergeSchedulingActions_EmptyContextsEnables` — no matching rules |
| 66 | + still means scheduling is restored. |
| 67 | +- Manual e2e (reporter's setup): apply the `AvoidanceAction` + `NodeQOS` |
| 68 | + from the issue, stress CPU above `cpu_total_usage: 3000` → taint appears; |
| 69 | + stop the load → after ~`restoreThreshold` rounds + `coolDownSeconds: 60`, |
| 70 | + `kubectl describe node` shows the `EnsuranceAnalyzedPressure` taint removed. |
| 71 | + |
| 72 | +#### Which issue(s) this PR fixes: |
| 73 | +<!-- |
| 74 | +*Automatically closes linked issue when PR is merged. |
| 75 | +Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. |
| 76 | +--> |
| 77 | +Fixes #894 |
| 78 | + |
| 79 | +#### Special notes for your reviewer: |
| 80 | + |
| 81 | +- **Design note — no `break` after the restore branch.** An earlier variant |
| 82 | + of this patch short-circuited `mergeSchedulingActions` after a past-cooldown |
| 83 | + restore; it was rejected because that skipped later contexts in the same |
| 84 | + round and violated the existing "any `Triggered` keeps the node disabled" |
| 85 | + contract. `TestMergeSchedulingActions_TriggeredAfterCooldownDisables` |
| 86 | + guards this ordering. |
| 87 | +- **Restore event timing changes slightly.** `Restored` now fires on every |
| 88 | + analysis round while it is pending (previously exactly once). The k8s |
| 89 | + event/log spam is unchanged: the `RestoreTriggered` event is still recorded |
| 90 | + only on the triggered→restored transition, and the warning log fires only |
| 91 | + on the transition round. |
| 92 | +- **Throttle restore is also re-evaluated during the cool-down** (same code |
| 93 | + path). `ThrottleExecutor.Restore` is idempotent once pods are restored to |
| 94 | + the watermark, so the extra rounds are no-ops; this also fixes the same |
| 95 | + "restore lost if inside cool-down" class of bug for throttle-up. |
| 96 | +- **`lastTriggeredTime` remains a single per-node timestamp** (shared across |
| 97 | + rules), as before this PR. With multiple NodeQOS rules targeting the same |
| 98 | + node, the cool-down starts from the last trigger of *any* rule; making it |
| 99 | + per-rule would be a larger refactor and is left as follow-up. |
| 100 | +- If the executor's node update itself fails (API conflict after retries), |
| 101 | + the taint can still be left behind — `execute()` only logs the error |
| 102 | + (pre-existing `TODO: if it failed in action, how to retry`). Also left as |
| 103 | + follow-up. |
0 commit comments