fix(dhcp): re-apply KEA config when running config drifts back to boo…#23
Closed
dmathur0 wants to merge 4 commits into
Closed
fix(dhcp): re-apply KEA config when running config drifts back to boo…#23dmathur0 wants to merge 4 commits into
dmathur0 wants to merge 4 commits into
Conversation
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
📝 WalkthroughWalkthroughAdds a shared remote lease-db helper, uses it in DHCP health and Kea config generation, extends the KEA sync loop with bootstrap detection and re-apply behavior, adds async tests, and adds a Kubernetes liveness probe backstop. ChangesKEA lease-db helper and recovery flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dmathur0
force-pushed
the
port/dhcp-config-drift-recovery-mr240
branch
from
2160eda to
79b0f65
Compare
![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
🤖 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`:
- Line 259: The code reads ini_config["dhcp.lease_db"].getboolean("local") into
remote_lease_db without guarding for a missing section/option; change the access
to either check ini_config.has_section("dhcp.lease_db") and
ini_config.has_option("dhcp.lease_db", "local") before indexing, or call
ini_config["dhcp.lease_db"].getboolean("local", fallback=False) (or another
sensible default) and log a clear warning when the section/option is absent;
apply the same defensive change to the analogous lookups in api.py and
kea_dhcp_confgen.py (search for usages of ini_config[...] .getboolean("local")
and the remote_lease_db assignment) so startup/healthcheck won’t crash on
incomplete INI.
🪄 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: 547532aa-f2fc-48a6-b8b4-27a4f152fdb2
📒 Files selected for processing (3)
deploy/helm/templates/dhcp.yamlsrc/nv_config_manager/dhcp/cli.pysrc/tests/dhcp/test_cli.py
…tstrap When the kea container is recycled it reloads the baked-in bootstrap config (no remote lease-database), but the sync loop only re-applied on Redis change, so the pod deadlocked with /healthcheck returning 500. Adds _kea_running_config_diverged() to detect the drift and force a re-apply, plus a looser backstop livenessProbe on config-sync-v4 so kubelet can recover if the in-loop check itself wedges. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nv_config_manager/dhcp/cli.py (1)
259-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
dhcp.lease_db.locallookup to avoid startup crash.At Line 259, direct indexing can raise if
[dhcp.lease_db]orlocalis missing, and this is outside the looptry, so the process can fail before sync starts.Proposed defensive read
- remote_lease_db = not ini_config["dhcp.lease_db"].getboolean("local") + if ini_config.has_section("dhcp.lease_db"): + local_lease_db = ini_config["dhcp.lease_db"].getboolean("local", fallback=False) + else: + logger.warning("Missing [dhcp.lease_db] section; defaulting to remote lease-db mode.") + local_lease_db = False + remote_lease_db = not local_lease_db🤖 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` at line 259, The direct access ini_config["dhcp.lease_db"]["local"] can raise if the section or option is missing; change the assignment to safely read the value by using ConfigParser's safe lookup (e.g., ini_config.getboolean("dhcp.lease_db", "local", fallback=...) or check ini_config.has_section("dhcp.lease_db") and ini_config.has_option("dhcp.lease_db", "local")) so remote_lease_db is computed defensively (default to False or a sensible fallback) and avoid raising before sync starts; update the code that sets remote_lease_db to use this guarded lookup instead of direct indexing.
🧹 Nitpick comments (1)
src/nv_config_manager/dhcp/cli.py (1)
226-228: ⚡ Quick winAlign docstring with actual probe-failure behavior.
At Line 226, the docstring says unreachable states can warrant re-apply, but the implementation returns
Falseon KEA query errors (Line 240). Please update the wording to match current behavior.Proposed docstring correction
- Returns True if the running config looks like the bootstrap (or is - otherwise unreachable in a way that warrants re-applying). + Returns True if the running config looks like bootstrap drift for + remote lease-db setups. Probe/query failures are treated as + non-divergence (best-effort) and return False.🤖 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 226 - 228, The docstring for the function that checks whether the running config looks like the bootstrap (the probe that can return True/False) is inaccurate: update its wording to state that probe failures (specifically KEA query errors) are treated as non-unreachable and result in False rather than triggering a re-apply; mention KEA query errors explicitly so the docstring matches the implementation that returns False on KEA probe failures.
🤖 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.
Duplicate comments:
In `@src/nv_config_manager/dhcp/cli.py`:
- Line 259: The direct access ini_config["dhcp.lease_db"]["local"] can raise if
the section or option is missing; change the assignment to safely read the value
by using ConfigParser's safe lookup (e.g.,
ini_config.getboolean("dhcp.lease_db", "local", fallback=...) or check
ini_config.has_section("dhcp.lease_db") and
ini_config.has_option("dhcp.lease_db", "local")) so remote_lease_db is computed
defensively (default to False or a sensible fallback) and avoid raising before
sync starts; update the code that sets remote_lease_db to use this guarded
lookup instead of direct indexing.
---
Nitpick comments:
In `@src/nv_config_manager/dhcp/cli.py`:
- Around line 226-228: The docstring for the function that checks whether the
running config looks like the bootstrap (the probe that can return True/False)
is inaccurate: update its wording to state that probe failures (specifically KEA
query errors) are treated as non-unreachable and result in False rather than
triggering a re-apply; mention KEA query errors explicitly so the docstring
matches the implementation that returns False on KEA probe failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60adcd1c-8f9c-4b58-8efd-b15e7d0266b4
📒 Files selected for processing (3)
deploy/helm/templates/dhcp.yamlsrc/nv_config_manager/dhcp/cli.pysrc/tests/dhcp/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/helm/templates/dhcp.yaml
- src/tests/dhcp/test_cli.py
dmathur0
force-pushed
the
port/dhcp-config-drift-recovery-mr240
branch
from
79b0f65 to
82ee2c1
Compare
zsblevins
reviewed
Jun 2, 2026
| asyncio.run(_refresh_loop_async(ip_version, check, refresh_interval)) | ||
|
|
||
|
|
||
| async def _kea_running_config_diverged( |
Collaborator
There was a problem hiding this comment.
I would rename this, diverged implies you're doing a direct comparison, which would be the cleanest approach but would always report divergence as kea has a lot of defaults and minor formatting changes between what we generate and what it reports back as its config. In the absence of being able to do a true divergence test, we should name this in a way that indicates it's only checking for the default configuration state
Collaborator
Author
There was a problem hiding this comment.
Good call. Renamed it to _kea_running_config_is_bootstrap (and updated the call site/tests/docstring) to make clear it only detects the bootstrap-default state rather than performing a true divergence comparison. Pushed in dfc6342.
- Rename _kea_running_config_diverged to _kea_running_config_is_bootstrap. The check does not do a true divergence comparison (KEA reports many defaults/formatting differences that would always look divergent); it only detects the specific bootstrap-default state, so the name now reflects that. - Guard the [dhcp.lease_db] local lookup behind a shared is_remote_lease_db() helper that reads the option defensively (fallback + warning) so a missing section/option no longer crashes DHCP startup, the sync loop, or /healthcheck. Applied in cli.py, api.py, and kea_dhcp_confgen.py. - Correct the docstring to state that probe/query failures are treated as non-divergence (return False). Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nv_config_manager/dhcp/kea_dhcp_confgen.py (1)
824-831: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefensive remote default still crashes here on a missing
[dhcp.lease_db]section.
is_remote_lease_db()returnsTruewhen the section/option is absent (logging a warning), but the very next lines unconditionally indexapp_config["dhcp.lease_db"]["database"|"host"|"user"|"password"]. So a missing section/keys will raiseKeyErrorat injection time, defeating the helper's stated goal of not crashing the sync path. Consider validating the required keys (orgetitemwith a clear error) before building the dict.🛡️ Sketch
if is_remote_lease_db(app_config): + if not app_config.has_section("dhcp.lease_db"): + raise RuntimeError( + "Remote lease-db mode selected but [dhcp.lease_db] section is missing" + ) dhcp_config[dhcp_key]["lease-database"] = { "type": "postgresql", "name": app_config["dhcp.lease_db"]["database"],🤖 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/kea_dhcp_confgen.py` around lines 824 - 831, The remote lease DB fallback in the DHCP config generation still dereferences app_config["dhcp.lease_db"] directly, so a missing section or keys will raise a KeyError after is_remote_lease_db() has already returned True. Update the lease-database handling in kea_dhcp_confgen.py to validate the required dhcp.lease_db entries before building the PostgreSQL dict, or use a guarded lookup that raises a clear configuration error instead of crashing; keep the fix localized to the block that constructs dhcp_config[dhcp_key]["lease-database"] and reuse is_remote_lease_db()/app_config access patterns consistently.src/nv_config_manager/dhcp/cli.py (1)
239-251: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNormalize
get_configresult like the healthcheck does.
api.pytreatsget_config's result defensively withconfig if isinstance(config, list) else [config], but hererunning[0]is indexed directly. If KEA returns a single dict (not a list),running[0]raisesKeyError. It's swallowed by the loop'sexcept, so the bootstrap re-apply is silently skipped on that path. Mirror the healthcheck normalization for consistency.🛡️ Sketch
- if not running: - return False - arguments = running[0].get("arguments", {}) if isinstance(running[0], dict) else {} + if not running: + return False + running_list = running if isinstance(running, list) else [running] + first = running_list[0] + arguments = first.get("arguments", {}) if isinstance(first, dict) else {}🤖 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 239 - 251, The divergence check in the KEA config path assumes get_config always returns a list, but api.py handles this defensively because KEA may return a single dict instead. Update the logic around get_config in the DHCP CLI helper to normalize the result the same way as the healthcheck does before indexing, then read arguments/Dhcp{ip_version}/lease-database from the normalized collection. Keep the existing exception handling and logging, but make sure running[0] is never accessed unless the response is confirmed to be a list-like sequence.
🤖 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.
Outside diff comments:
In `@src/nv_config_manager/dhcp/cli.py`:
- Around line 239-251: The divergence check in the KEA config path assumes
get_config always returns a list, but api.py handles this defensively because
KEA may return a single dict instead. Update the logic around get_config in the
DHCP CLI helper to normalize the result the same way as the healthcheck does
before indexing, then read arguments/Dhcp{ip_version}/lease-database from the
normalized collection. Keep the existing exception handling and logging, but
make sure running[0] is never accessed unless the response is confirmed to be a
list-like sequence.
In `@src/nv_config_manager/dhcp/kea_dhcp_confgen.py`:
- Around line 824-831: The remote lease DB fallback in the DHCP config
generation still dereferences app_config["dhcp.lease_db"] directly, so a missing
section or keys will raise a KeyError after is_remote_lease_db() has already
returned True. Update the lease-database handling in kea_dhcp_confgen.py to
validate the required dhcp.lease_db entries before building the PostgreSQL dict,
or use a guarded lookup that raises a clear configuration error instead of
crashing; keep the fix localized to the block that constructs
dhcp_config[dhcp_key]["lease-database"] and reuse
is_remote_lease_db()/app_config access patterns consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 74055a62-69a4-444a-9e82-0b47c644e5bd
📒 Files selected for processing (5)
src/nv_config_manager/common/config.pysrc/nv_config_manager/dhcp/api.pysrc/nv_config_manager/dhcp/cli.pysrc/nv_config_manager/dhcp/kea_dhcp_confgen.pysrc/tests/dhcp/test_cli.py
|
/ok to test c10a6b0 |
8 tasks
Collaborator
Author
|
Split the Helm-only backstop |
dmathur0
added a commit
that referenced
this pull request
Jun 25, 2026
Adds a conservative livenessProbe to the config-sync-v4 container in deploy/helm/templates/dhcp.yaml so kubelet can recycle the sidecar if the in-loop KEA divergence check ever wedges (asyncio loop stalls or the kea control socket stays unresponsive). On restart the container re-runs the unconditional "Run once" set_config() and heals the deadlock. Thresholds are intentionally looser than the other probes (failureThreshold * periodSeconds = 300s) so the in-loop recovery wins under normal conditions and we only recycle when the loop itself is broken. Split out of #23 so this low-risk, Helm-only safety net can ship now while the KEA bootstrap-drift detection/re-apply application logic waits for QA. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
spidercensus
pushed a commit
that referenced
this pull request
Jun 25, 2026
Adds a conservative livenessProbe to the config-sync-v4 container in deploy/helm/templates/dhcp.yaml so kubelet can recycle the sidecar if the in-loop KEA divergence check ever wedges (asyncio loop stalls or the kea control socket stays unresponsive). On restart the container re-runs the unconditional "Run once" set_config() and heals the deadlock. Thresholds are intentionally looser than the other probes (failureThreshold * periodSeconds = 300s) so the in-loop recovery wins under normal conditions and we only recycle when the loop itself is broken. Split out of #23 so this low-risk, Helm-only safety net can ship now while the KEA bootstrap-drift detection/re-apply application logic waits for QA. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
The config-sync-v4 backstop livenessProbe was split out of this PR and merged to main independently in #65. Remove it here so this branch carries only the KEA bootstrap-drift detection / re-apply application logic (and tests), which is being held for QA, and the two changes no longer overlap. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…tstrap
Ported from kiwi-platform MR !240. When the kea container is recycled it reloads the baked-in bootstrap config (no remote lease-database), but the sync loop only re-applied on Redis change, so the pod deadlocked with /healthcheck returning 500. Adds _kea_running_config_diverged() to detect the drift and force a re-apply, plus a looser backstop livenessProbe on config-sync-v4 so kubelet can recover if the in-loop check itself wedges.
Description
Validation
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for review,
run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite,or narrow it only while debugging.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests