feat(dhcp): self-contained heartbeat liveness for config-sync sidecar#171
feat(dhcp): self-contained heartbeat liveness for config-sync sidecar#171dmathur0 wants to merge 1 commit into
Conversation
Replace the config-sync-v4 backstop HTTP livenessProbe (which hit the shared healthcheck sidecar) with a self-contained heartbeat mechanism so the sidecar's liveness depends only on its own event-loop progress. - Touch a heartbeat file (default /tmp/config-sync-v4.heartbeat) after every completed reconcile attempt in _sync_kea_configuration_async. The heartbeat represents event-loop progress, not successful config application: recoverable Redis/PostgreSQL/Kea errors are logged/counted but still advance it, while a genuinely wedged loop stops advancing it. - Add a check-sync-heartbeat CLI subcommand (--heartbeat-file, --max-age; env CONFIG_SYNC_HEARTBEAT_FILE / CONFIG_SYNC_HEARTBEAT_MAX_AGE; default max-age 90s) that exits 0 only when the file is fresh and non-zero when stale/missing. - Bound the Redis load and Kea set_config calls with asyncio.wait_for (10s / 15s, overridable via CONFIG_SYNC_REDIS_TIMEOUT / CONFIG_SYNC_KEA_TIMEOUT) so a hung dependency cannot suspend the loop and freeze the heartbeat. - Track last-successful-reconciliation separately from loop progress (minimal module-level state) for later readiness/alerting use. - Swap the Helm config-sync-v4 livenessProbe to an exec probe running check-sync-heartbeat, keeping thresholds looser than the kea/api probes. Adds unit tests for the CLI exit codes, heartbeat advancement on success and on recoverable errors, and bounded-timeout behavior. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
📝 WalkthroughWalkthroughChangesThe DHCP config-sync loop now uses bounded Redis and KEA operations, maintains a heartbeat file, and records successful reconciliations. A new CLI command validates heartbeat freshness, and the Helm liveness probe invokes it. Tests cover heartbeat behavior, CLI outcomes, recoverable errors, and hung dependencies. DHCP config-sync liveness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigSync
participant Redis
participant KEA
participant HeartbeatFile
ConfigSync->>Redis: Load cached KEA configuration with timeout
Redis-->>ConfigSync: Configuration or recoverable error
ConfigSync->>KEA: Apply configuration with timeout
KEA-->>ConfigSync: Apply result
ConfigSync->>HeartbeatFile: Touch heartbeat after each attempt
ConfigSync->>HeartbeatFile: Record successful reconciliation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)deploy/helm/templates/dhcp.yamlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/helm/templates/dhcp.yaml (1)
191-213: 📐 Maintainability & Code Quality | 🔵 TrivialValidate this probe change with
helm templatebefore merge.As per coding guidelines, dry-run this change with
helm template ... -f values-ci.yaml(layeringvalues-observability.yamlif exercising the observability stack) rather thanvalues.yamlalone, sincevalues.yamlneeds secrets/vault paths unavailable in CI.🤖 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 `@deploy/helm/templates/dhcp.yaml` around lines 191 - 213, Validate the updated livenessProbe template with helm template using values-ci.yaml, layering values-observability.yaml when exercising the observability stack; do not validate against values.yaml alone because it requires unavailable CI secrets and vault paths.Source: Coding guidelines
src/nv_config_manager/dhcp/heartbeat.py (1)
64-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOnly
FileNotFoundErroris treated as "missing"; otherOSErrors will crash the CLI.If
os.statfails for a reason other than a missing file (e.g., permission denied,ENOTDIR), the exception propagates uncaught throughcheck-sync-heartbeat, producing a raw traceback instead of the intended clean stale/missing verdict for the liveness probe. Broadening the catch is a small, cheap hardening given this function backs alivenessProbe execcheck.🛡️ Proposed fix
try: mtime = os.stat(path).st_mtime - except FileNotFoundError: + except OSError: return None🤖 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 `@src/nv_config_manager/dhcp/heartbeat.py` around lines 64 - 74, Update heartbeat_age_seconds to catch the broader OSError from os.stat, not only FileNotFoundError, and return None for any stat failure so check-sync-heartbeat preserves its clean stale/missing liveness result without an uncaught traceback.
🤖 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 `@src/nv_config_manager/dhcp/cli.py`:
- Around line 256-275: Update the initial config-wait loop in the reconciliation
flow to call touch_heartbeat(heartbeat_file) on each polling iteration while
config is None, before sleeping or retrying _load_kea_config_with_timeout.
Preserve the existing heartbeat update after successful configuration
application so both waiting and completed states advance the liveness marker.
---
Nitpick comments:
In `@deploy/helm/templates/dhcp.yaml`:
- Around line 191-213: Validate the updated livenessProbe template with helm
template using values-ci.yaml, layering values-observability.yaml when
exercising the observability stack; do not validate against values.yaml alone
because it requires unavailable CI secrets and vault paths.
In `@src/nv_config_manager/dhcp/heartbeat.py`:
- Around line 64-74: Update heartbeat_age_seconds to catch the broader OSError
from os.stat, not only FileNotFoundError, and return None for any stat failure
so check-sync-heartbeat preserves its clean stale/missing liveness result
without an uncaught traceback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6f0390b-0c58-4395-bf4b-6855d491c277
📒 Files selected for processing (4)
deploy/helm/templates/dhcp.yamlsrc/nv_config_manager/dhcp/cli.pysrc/nv_config_manager/dhcp/heartbeat.pysrc/tests/dhcp/test_heartbeat.py
| try: | ||
| config = await redis_client.load_kea_config(ip_version) | ||
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | ||
| while config is None: | ||
| logger.info( | ||
| f"Waiting for KEA DHCP{ip_version} Configuration to be available in Redis..." | ||
| ) | ||
| await asyncio.sleep(1) | ||
| config = await redis_client.load_kea_config(ip_version) | ||
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | ||
|
|
||
| # Inject Lease DB details after loading from Redis | ||
| # so that secrets are not stored in the Redis cache | ||
| config = inject_lease_db_config(config, ip_version) | ||
|
|
||
| # Run once | ||
| logger.info(f"Setting initial KEA DHCPv{ip_version} Configuration from Redis.") | ||
| await kea_client.set_config(config, version=ip_version) | ||
| await _set_kea_config_with_timeout(kea_client, config, ip_version) | ||
| record_successful_reconciliation() | ||
| # Seed the heartbeat immediately so the liveness probe has a fresh | ||
| # marker before the first monitoring iteration completes. | ||
| touch_heartbeat(heartbeat_file) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initial "waiting for config in Redis" loop never advances the heartbeat.
touch_heartbeat is only called after the initial config is successfully loaded and applied (line 275). While config is None, the loop polls every second — genuine, non-wedged progress — but the heartbeat file stays missing the entire time. Per heartbeat.py's own contract, only a truly wedged loop should fail to advance the heartbeat; a slow-to-populate Redis (e.g. cold-start ordering, first deployment before any producer has written config) is exactly the "recoverable, still-progressing" case this feature is meant to tolerate. Once initialDelaySeconds+failureThreshold*periodSeconds elapses (300s+ per the dhcp.yaml probe), check-sync-heartbeat will report "missing" and kubelet will start recycling a healthy sidecar that's simply waiting on upstream data.
🩹 Proposed fix
config = await _load_kea_config_with_timeout(redis_client, ip_version)
while config is None:
logger.info(
f"Waiting for KEA DHCP{ip_version} Configuration to be available in Redis..."
)
+ touch_heartbeat(heartbeat_file)
await asyncio.sleep(1)
config = await _load_kea_config_with_timeout(redis_client, ip_version)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| config = await redis_client.load_kea_config(ip_version) | |
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | |
| while config is None: | |
| logger.info( | |
| f"Waiting for KEA DHCP{ip_version} Configuration to be available in Redis..." | |
| ) | |
| await asyncio.sleep(1) | |
| config = await redis_client.load_kea_config(ip_version) | |
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | |
| # Inject Lease DB details after loading from Redis | |
| # so that secrets are not stored in the Redis cache | |
| config = inject_lease_db_config(config, ip_version) | |
| # Run once | |
| logger.info(f"Setting initial KEA DHCPv{ip_version} Configuration from Redis.") | |
| await kea_client.set_config(config, version=ip_version) | |
| await _set_kea_config_with_timeout(kea_client, config, ip_version) | |
| record_successful_reconciliation() | |
| # Seed the heartbeat immediately so the liveness probe has a fresh | |
| # marker before the first monitoring iteration completes. | |
| touch_heartbeat(heartbeat_file) | |
| try: | |
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | |
| while config is None: | |
| logger.info( | |
| f"Waiting for KEA DHCP{ip_version} Configuration to be available in Redis..." | |
| ) | |
| touch_heartbeat(heartbeat_file) | |
| await asyncio.sleep(1) | |
| config = await _load_kea_config_with_timeout(redis_client, ip_version) | |
| # Inject Lease DB details after loading from Redis | |
| # so that secrets are not stored in the Redis cache | |
| config = inject_lease_db_config(config, ip_version) | |
| # Run once | |
| logger.info(f"Setting initial KEA DHCPv{ip_version} Configuration from Redis.") | |
| await _set_kea_config_with_timeout(kea_client, config, ip_version) | |
| record_successful_reconciliation() | |
| # Seed the heartbeat immediately so the liveness probe has a fresh | |
| # marker before the first monitoring iteration completes. | |
| touch_heartbeat(heartbeat_file) |
🤖 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 `@src/nv_config_manager/dhcp/cli.py` around lines 256 - 275, Update the initial
config-wait loop in the reconciliation flow to call
touch_heartbeat(heartbeat_file) on each polling iteration while config is None,
before sleeping or retrying _load_kea_config_with_timeout. Preserve the existing
heartbeat update after successful configuration application so both waiting and
completed states advance the liveness marker.
1 participant
Summary
Gives the
config-sync-v4sidecar its own liveness signal so a genuinely wedged reconcile loop is restarted by kubelet, without adding an HTTP server and without restart-looping during external dependency outages.The sync loop now touches a heartbeat file after every completed reconcile attempt (advancing even on recoverable Redis/PG/Kea errors, but never advancing if an iteration is truly stuck). A new
check-sync-heartbeatCLI subcommand backs anexeclivenessProbe. Dependency calls are bounded by timeouts, and "last successful reconciliation" is tracked separately from loop progress.Changes
heartbeat.py(new): touch/age/freshness helpers + separate last-successful-reconciliation state.cli.py: heartbeat touches (seed after initialset_config, then in thefinallyof every loop iteration) + bounded timeouts in the sync loop; newcheck-sync-heartbeatcommand;--heartbeat-fileonsync-kea-configuration.deploy/helm/templates/dhcp.yaml:config-sync-v4HTTP probe →execheartbeat probe.check-sync-heartbeat--heartbeat-file(default/tmp/config-sync-v4.heartbeat, envCONFIG_SYNC_HEARTBEAT_FILE)--max-ageseconds (default90, envCONFIG_SYNC_HEARTBEAT_MAX_AGE)Timeouts (via
asyncio.wait_for; timeout = recoverable error → logged/counted, heartbeat still advances)load_kea_config: 10s (CONFIG_SYNC_REDIS_TIMEOUT)set_config: 15s (CONFIG_SYNC_KEA_TIMEOUT)Helm probe
exec: [nv-config-manager-dhcp-confgen, check-sync-heartbeat],initialDelaySeconds: 180,periodSeconds: 30,timeoutSeconds: 10,failureThreshold: 10(300s — looser than kea/api at 100s).Test plan
helm templaterendersPart 2 of 5 from the DHCP config-sync recovery proposal, opened independently against
main.Merge note
Both
cli.py(_sync_kea_configuration_async) anddeploy/helm/templates/dhcp.yamlare also touched by #168 (Part 1), #170 (Part 3), and Part 4. Whichever merges later needs a small rebase — heartbeat advancement should wrap Part 1's hash-reconcile body, and theconfig-sync-v4probe here (exec) replaces the HTTP probe Part 3 repoints to/livez.Summary by CodeRabbit
New Features
Bug Fixes
Tests