fix(dhcp): reconcile KEA config using native config hash#168
Open
dmathur0 wants to merge 1 commit into
Open
Conversation
The Kea container can restart independently while the config-sync sidecar keeps running, bringing Kea back up from its bootstrap kea-dhcp4.conf. Since the desired config in Redis is unchanged, the previous sync loop compared only the Redis value against its in-process copy, concluded nothing changed, and never re-applied to Kea. Use Kea's native SHA-256 effective-configuration hash (Kea 2.4+) for drift detection instead: - set_config() now returns the hash from the config-set response. - Add get_config_hash() wrapping the config-hash-get command. - The sync loop tracks expected_hash from the initial apply and, when Redis is unchanged, compares config-hash-get against it, re-applying the desired config on drift. The effective hash is verified after every apply. A config-sync restart stays safe: startup still unconditionally applies the desired Redis config and captures a fresh expected_hash. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
July 23, 2026 01:55
📝 WalkthroughWalkthroughThe DHCP ConfGen CLI now verifies KEA’s effective configuration hash after applying configuration and detects KEA-side drift during refresh cycles. ChangesKEA hash API and synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RedisClient
participant _sync_kea_configuration_async
participant KeaClient
_sync_kea_configuration_async->>RedisClient: load desired DHCP configuration
_sync_kea_configuration_async->>KeaClient: set_config(desired configuration)
KeaClient-->>_sync_kea_configuration_async: applied configuration hash
_sync_kea_configuration_async->>KeaClient: get_config_hash()
KeaClient-->>_sync_kea_configuration_async: effective configuration hash
_sync_kea_configuration_async->>KeaClient: reapply desired configuration on hash drift
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nv_config_manager/dhcp/cli.py (1)
1-1: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHandle
config-hash-getfailures during startup
src/nv_config_manager/dhcp/cli.py#L211-L235: catchKeaExceptionfromget_config_hashhere and treat unsupported/missing hash as non-fatal, otherwise the sync process aborts before the refresh loop can recover.src/nv_config_manager/dhcp/kea.py#L166-L183: update the docstring to say unsupportedconfig-hash-getraisesKeaException; it does not returnNone.🤖 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 1, Update the startup flow around get_config_hash in the DHCP CLI to catch KeaException and treat unsupported or missing config hashes as non-fatal, allowing the refresh loop to continue; preserve propagation of other failures as appropriate. Also update get_config_hash’s docstring in the Kea integration to document that unsupported config-hash-get raises KeaException rather than returning None.
🤖 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 211-235: Update _apply_and_verify_kea_config to catch failures
from get_config_hash during startup and apply the same fallback behavior used by
the refresh loop. Allow unsupported or temporarily unavailable hash retrieval to
proceed without aborting synchronization, while preserving the existing
applied/effective hash comparison whenever a hash is successfully retrieved.
---
Outside diff comments:
In `@src/nv_config_manager/dhcp/cli.py`:
- Line 1: Update the startup flow around get_config_hash in the DHCP CLI to
catch KeaException and treat unsupported or missing config hashes as non-fatal,
allowing the refresh loop to continue; preserve propagation of other failures as
appropriate. Also update get_config_hash’s docstring in the Kea integration to
document that unsupported config-hash-get raises KeaException rather than
returning None.
🪄 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: f18424bf-ad7d-4aaa-8611-451f73e15b9d
📒 Files selected for processing (4)
src/nv_config_manager/dhcp/cli.pysrc/nv_config_manager/dhcp/kea.pysrc/tests/dhcp/test_cli.pysrc/tests/dhcp/test_kea.py
Comment on lines
+211
to
+235
| async def _apply_and_verify_kea_config( | ||
| kea_client: KeaClient, | ||
| config: dict[str, Any], | ||
| ip_version: int, | ||
| ) -> str | None: | ||
| """Apply the desired configuration to KEA and return its verified hash. | ||
|
|
||
| KEA (2.4+) returns the SHA-256 hash of the effective configuration from | ||
| ``config-set``. Before treating the sync as successful, the running | ||
| configuration is confirmed against that hash via ``config-hash-get`` so a | ||
| configuration that was rolled back or only partially applied surfaces as an | ||
| error rather than being silently trusted. The verified effective hash is | ||
| returned to track for subsequent drift detection. | ||
| """ | ||
| applied_hash = await kea_client.set_config(config, version=ip_version) | ||
| effective_hash = await kea_client.get_config_hash(version=ip_version) | ||
| if applied_hash is not None and effective_hash != applied_hash: | ||
| raise KeaException( | ||
| f"KEA effective configuration hash ({effective_hash}) does not match " | ||
| f"the hash returned by config-set ({applied_hash}); " | ||
| "the configuration was not applied cleanly." | ||
| ) | ||
| return effective_hash | ||
|
|
||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant CLI and client code paths.
git ls-files src/nv_config_manager/dhcp/cli.py src/nv_config_manager/dhcp/kea.py tests | sed 's#^`#FILE` #'
printf '\n--- cli.py around relevant lines ---\n'
sed -n '180,300p' src/nv_config_manager/dhcp/cli.py
printf '\n--- kea.py hash-related methods ---\n'
rg -n "config-hash-get|get_config_hash|config-set|KeaException|pre-2\.4|hash" src/nv_config_manager/dhcp/kea.py src/nv_config_manager/dhcp/cli.py tests -S
printf '\n--- tests touching config hash handling ---\n'
rg -n "config_hash|get_config_hash|hash-get|config-set|pre-2\.4|KeaException" tests -SRepository: NVIDIA/nv-config-manager
Length of output: 10208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- cli.py relevant slice ---\n'
sed -n '210,290p' src/nv_config_manager/dhcp/cli.py
printf '\n--- kea.py relevant slice ---\n'
sed -n '1,260p' src/nv_config_manager/dhcp/kea.py
printf '\n--- tests mentioning startup refresh / hash handling ---\n'
rg -n "refresh loop|startup|get_config_hash|config-hash-get|config-hash|get_config_hash_failure|without reapply|pre-2\.4" tests -SRepository: NVIDIA/nv-config-manager
Length of output: 14070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '210,290p' src/nv_config_manager/dhcp/cli.py
echo '---'
sed -n '1,260p' src/nv_config_manager/dhcp/kea.py
echo '---'
rg -n "config-hash-get|get_config_hash|test_config_hash_get_failure_is_handled_without_reapply|startup|refresh loop" tests src/nv_config_manager/dhcp -SRepository: NVIDIA/nv-config-manager
Length of output: 14709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- cli.py from 290 onward ---\n'
sed -n '290,380p' src/nv_config_manager/dhcp/cli.py
printf '\n--- top-level test files ---\n'
git ls-files | rg '(^|/)(test|tests)/|test_.*\.(py|ts|js)$|_test\.(py|ts|js)$' -n
printf '\n--- search for exception handling around refresh_kea_configuration ---\n'
rg -n "_sync_kea_configuration_async|refresh_kea_configuration|except .*KeaException|ClickException|sys\.excepthook|_exception_handler" src/nv_config_manager/dhcp/cli.py -n -SRepository: NVIDIA/nv-config-manager
Length of output: 24456
Handle get_config_hash() failures on startup. _apply_and_verify_kea_config() still calls get_config_hash() before entering the refresh loop, so a pre-2.4 Kea server or any transient hash-get error can abort sync during startup even though the loop later tolerates the same failure. Mirror the loop’s fallback here so unsupported servers start cleanly.
🤖 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 211 - 235, Update
_apply_and_verify_kea_config to catch failures from get_config_hash during
startup and apply the same fallback behavior used by the refresh loop. Allow
unsupported or temporarily unavailable hash retrieval to proceed without
aborting synchronization, while preserving the existing applied/effective hash
comparison whenever a hash is successfully retrieved.
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
1 participant
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.
Summary
Fixes the DHCP recovery failure observed during EKS upgrades. When the
keacontainer restarts independently of theconfig-sync-v4sidecar, Kea comes back up on the bootstrap/etc/kea/kea-dhcp4.conf(no subnets, default memfile lease DB). Because the desired config in Redis is unchanged, the old sync loop — which only compared the Redis value against its in-processprevious_config— concluded "nothing changed" and never re-applied to Kea, leaving the pod unhealthy indefinitely.This switches drift detection to Kea's native SHA-256 effective-config hash (Kea 2.4+), so any loss/modification of the running config is detected and reconciled regardless of whether Redis changed.
Changes
KeaClient.set_config()now returns the hash from theconfig-setresponse (str | None;Noneon pre-2.4 Kea).KeaClient.get_config_hash()usingconfig-hash-get._sync_kea_configuration_asyncretains anexpected_hash:expected_hash.config-hash-gettoexpected_hash; on mismatch (e.g. Kea restarted to bootstrap) re-apply the desired config and updateexpected_hash._apply_and_verify_kea_confighelper verifies the effective hash after applying before recording sync success.config-getJSON (which contains Kea-generated defaults).Note: the unreliable lease-database bootstrap heuristic referenced in the design does not exist on
main, so there was nothing to remove here.Test plan
set_configreturns hash / handles hash-absent (pre-2.4)config-setfailure propagates;config-hash-getfailure counted, no reapplyruff checkclean,ruff formatapplied,mypyclean on changed sourcePart 1 of 5 from the DHCP config-sync recovery proposal. Parts 2–5 (heartbeat liveness, strict traffic gating, observability, defense-in-depth) will follow on separate branches and stack on this.
Summary by CodeRabbit
Reliability Improvements
Tests