fix(agent): compare all HBN inputs before skipping apply - #4746
Conversation
|
@coderabbitai full_review, thanks! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe agent now computes a canonical fingerprint from all HBN-rendering inputs. It removes non-rendering fields, normalizes unordered collections, preserves meaningful ordering, and compares the fingerprint with managed-host and instance network versions. Tests cover cache invalidation and ignored changes. ChangesHBN rendering fingerprint
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CurrentNetworkVersion
participant RenderedInputResponse
participant ProtobufEncoder
CurrentNetworkVersion->>RenderedInputResponse: obtain HBN-rendering inputs
RenderedInputResponse->>RenderedInputResponse: remove non-rendering fields and normalize collections
RenderedInputResponse->>ProtobufEncoder: encode canonical response
ProtobufEncoder-->>CurrentNetworkVersion: return encoded bytes
CurrentNetworkVersion->>CurrentNetworkVersion: compare fingerprint with network versions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/agent/src/main_loop.rs (3)
2038-2074: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scenario for the initial state before any
update_fromcall.Every row in these tables calls
update_fromfirst, so the(_, None)arm at line 522 is never exercised. That arm guarantees that a freshly constructedCurrentNetworkVersionnever reports a match, which is what forces the first HBN apply after an agent restart. The arm is the safety-critical default and it currently has no test.💚 Proposed additional test
#[test] fn current_network_version_never_matches_before_first_update() { let config = comparison_network_config(); assert!(!CurrentNetworkVersion::default().matches_versions_from(&config)); }🤖 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 `@crates/agent/src/main_loop.rs` around lines 2038 - 2074, Add a focused test alongside current_network_version_detects_rendered_input_changes that constructs comparison_network_config and asserts CurrentNetworkVersion::default().matches_versions_from(&config) returns false before any update_from call, covering the initial-state safety behavior.
663-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the prefix-entry sort key into one helper.
The comparator
|left, right| left.prefix.cmp(&right.prefix)appears three times across two protobuf types. If a future change alters this key in only some call sites, the fingerprint becomes order-sensitive again for the missed collection. Centralize the key so the normalization contract has one definition.♻️ Proposed extraction
+ fn sort_prefix_entries(entries: &mut [rpc::PrefixFilterPolicyEntry]) { + entries.sort_by(|left, right| left.prefix.cmp(&right.prefix)); + } + fn normalize_interface(interface: &mut rpc::FlatInterfaceConfig) { interface.vpc_prefixes.sort_unstable(); interface.vpc_peer_prefixes.sort_unstable(); interface.vpc_peer_vnis.sort_unstable(); if let Some(routing_profile) = &mut interface.vpc_routing_profile { Self::normalize_routing_profile(routing_profile); } if let Some(routing_profile) = &mut interface.interface_routing_profile { - routing_profile - .allowed_anycast_prefixes - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); + Self::sort_prefix_entries(&mut routing_profile.allowed_anycast_prefixes); }fn normalize_routing_profile(routing_profile: &mut rpc::RoutingProfile) { routing_profile .route_target_imports .sort_by_key(|route_target| (route_target.asn, route_target.vni)); routing_profile .route_targets_on_exports .sort_by_key(|route_target| (route_target.asn, route_target.vni)); - routing_profile - .accepted_leaks_from_underlay - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); - routing_profile - .allowed_anycast_prefixes - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); + Self::sort_prefix_entries(&mut routing_profile.accepted_leaks_from_underlay); + Self::sort_prefix_entries(&mut routing_profile.allowed_anycast_prefixes); }🤖 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 `@crates/agent/src/main_loop.rs` around lines 663 - 697, Extract the repeated prefix-entry sort key used by normalize_interface and normalize_routing_profile into a shared helper, then use that helper for all allowed_anycast_prefixes and accepted_leaks_from_underlay sorts across both protobuf types. Preserve the existing prefix-based ordering while ensuring future key changes have a single definition.
606-633: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument
ntp_serversas a DHCP-owned HBN input.update_dhcpconsumes it before the HBN skip check, so its exclusion from the fingerprint is safe.host_interface_idanduse_admin_network_changedhave no HBN rendering consumers.🤖 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 `@crates/agent/src/main_loop.rs` around lines 606 - 633, Update the comments around ntp_servers in the HBN fingerprint reset logic to explicitly document that it is a DHCP-owned input consumed by update_dhcp before the HBN skip check, so excluding it is intentional. Keep host_interface_id and use_admin_network_changed identified as having no HBN rendering consumers.
🤖 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 `@crates/agent/src/main_loop.rs`:
- Around line 2038-2074: Add a focused test alongside
current_network_version_detects_rendered_input_changes that constructs
comparison_network_config and asserts
CurrentNetworkVersion::default().matches_versions_from(&config) returns false
before any update_from call, covering the initial-state safety behavior.
- Around line 663-697: Extract the repeated prefix-entry sort key used by
normalize_interface and normalize_routing_profile into a shared helper, then use
that helper for all allowed_anycast_prefixes and accepted_leaks_from_underlay
sorts across both protobuf types. Preserve the existing prefix-based ordering
while ensuring future key changes have a single definition.
- Around line 606-633: Update the comments around ntp_servers in the HBN
fingerprint reset logic to explicitly document that it is a DHCP-owned input
consumed by update_dhcp before the HBN skip check, so excluding it is
intentional. Keep host_interface_id and use_admin_network_changed identified as
having no HBN rendering consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f62124f-b59d-435b-bb3c-13a4583347c8
📒 Files selected for processing (1)
crates/agent/src/main_loop.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/agent/src/main_loop.rs (4)
663-682: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated prefix-entry sort into one helper.
allowed_anycast_prefixesis sorted by the same comparator in two places: inline at Lines 672-674 and insidenormalize_routing_profileat Lines 694-696. The two sites must stay identical for the fingerprint to be canonical. Nothing currently enforces that.The inline form is correct today because
FlatInterfaceRoutingProfilecarries onlyallowed_anycast_prefixes. A single helper keeps both call sites aligned and makes a future field addition to that type easier to spot.♻️ Proposed refactor
+ fn sort_prefix_entries(prefixes: &mut [rpc::PrefixFilterPolicyEntry]) { + prefixes.sort_by(|left, right| left.prefix.cmp(&right.prefix)); + } + fn normalize_interface(interface: &mut rpc::FlatInterfaceConfig) {if let Some(routing_profile) = &mut interface.interface_routing_profile { - routing_profile - .allowed_anycast_prefixes - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); + Self::sort_prefix_entries(&mut routing_profile.allowed_anycast_prefixes); }- routing_profile - .accepted_leaks_from_underlay - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); - routing_profile - .allowed_anycast_prefixes - .sort_by(|left, right| left.prefix.cmp(&right.prefix)); + Self::sort_prefix_entries(&mut routing_profile.accepted_leaks_from_underlay); + Self::sort_prefix_entries(&mut routing_profile.allowed_anycast_prefixes);🤖 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 `@crates/agent/src/main_loop.rs` around lines 663 - 682, Extract the shared allowed-anycast-prefix sorting comparator from normalize_interface and normalize_routing_profile into a single helper, then call that helper at both sites. Preserve sorting by each entry’s prefix and ensure both normalization paths use the same implementation for canonical fingerprints.
499-499: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOptional: store the canonical bytes instead of a 64-bit hash.
DefaultHasherproduces a 64-bit value. A collision makesmatches_versions_fromreturntruefor a changed configuration, and the agent then skips an HBN apply silently. The probability is very small, but the failure mode is a silent stale render.The encoded canonical response is a few kilobytes at most and is already materialized inside
hash_rendered_inputs. StoringOption<Vec<u8>>and comparing with==removes the collision class entirely and also removes theDefaultHasherimport from this path.♻️ Proposed change
- rendered_inputs_hash: Option<u64>, + rendered_inputs: Option<Vec<u8>>,- fn hash_rendered_inputs(conf: &ManagedHostNetworkConfigResponse) -> u64 { + fn canonical_rendered_inputs(conf: &ManagedHostNetworkConfigResponse) -> Vec<u8> { let mut canonical = conf.clone(); Self::remove_non_rendered_inputs(&mut canonical); Self::normalize_set_like_inputs(&mut canonical); - - let mut hasher = DefaultHasher::new(); - canonical.encode_to_vec().hash(&mut hasher); - - hasher.finish() + canonical.encode_to_vec() }🤖 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 `@crates/agent/src/main_loop.rs` at line 499, Replace the rendered_inputs_hash state with Option<Vec<u8>> and update hash_rendered_inputs and matches_versions_from to retain and compare the canonical encoded response bytes directly using equality. Remove the DefaultHasher-based hashing and its import from this path while preserving the existing version-matching behavior.
512-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider counting fallback invalidations with an Event.
This branch is the safety net described in the TODO at Line 535. Its firing rate is the signal that tells you whether Core-side version invalidation is still missing. A plain
tracing::info!gives no rate or count.Declare a
carbide_instrument::Eventwith acarbide_-prefixed_totalcounter so the fallback rate is observable, and keep the human-readable message as the log line.Based on the coding guideline "define and emit a
carbide_instrument::Eventwhen an event deserves a count, rate, or duration".🤖 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 `@crates/agent/src/main_loop.rs` around lines 512 - 520, In the rendered-inputs mismatch branch of the config-version validation logic, replace the standalone tracing call with a carbide_instrument::Event definition using a carbide_-prefixed _total counter, then emit that event while retaining the existing human-readable info log. Keep the fallback invalidation behavior unchanged.Source: Coding guidelines
607-624: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClarify the
ntp_serversownership comment.ntp_serversfeedsupdate_dhcp, which runs before the HBN skip check.instancefeeds metadata and FMDS only, notupdate_nvueorupdate_dhcp. Change the comment to state that DHCP configuration, including NTP servers, is reconciled before the skip decision.🤖 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 `@crates/agent/src/main_loop.rs` around lines 607 - 624, Update the comment immediately before ntp_servers.clear() to state that DHCP configuration, including NTP servers, is reconciled before the HBN skip decision. Keep the ownership comments for instance and the other fields unchanged.
🤖 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 `@crates/agent/src/main_loop.rs`:
- Around line 704-718: The direction key in normalize_security_group_rule_order
must distinguish every direction variant: replace the boolean Ingress comparison
with a deterministic integer representation of attributes.direction(). In
crates/agent/src/main_loop.rs lines 1784-1811, update
comparison_security_group_rule to accept a direction parameter, include at least
one Egress fixture, and add an OrderSensitiveInputReordering or
RenderedInputChange scenario that flips a rule’s direction; both sites require
changes.
---
Nitpick comments:
In `@crates/agent/src/main_loop.rs`:
- Around line 663-682: Extract the shared allowed-anycast-prefix sorting
comparator from normalize_interface and normalize_routing_profile into a single
helper, then call that helper at both sites. Preserve sorting by each entry’s
prefix and ensure both normalization paths use the same implementation for
canonical fingerprints.
- Line 499: Replace the rendered_inputs_hash state with Option<Vec<u8>> and
update hash_rendered_inputs and matches_versions_from to retain and compare the
canonical encoded response bytes directly using equality. Remove the
DefaultHasher-based hashing and its import from this path while preserving the
existing version-matching behavior.
- Around line 512-520: In the rendered-inputs mismatch branch of the
config-version validation logic, replace the standalone tracing call with a
carbide_instrument::Event definition using a carbide_-prefixed _total counter,
then emit that event while retaining the existing human-readable info log. Keep
the fallback invalidation behavior unchanged.
- Around line 607-624: Update the comment immediately before ntp_servers.clear()
to state that DHCP configuration, including NTP servers, is reconciled before
the HBN skip decision. Keep the ownership comments for instance and the other
fields unchanged.
🪄 Autofix
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: ee911cdc-2f79-4784-b029-93dd834900ca
📒 Files selected for processing (1)
crates/agent/src/main_loop.rs
The agent used Core's versions plus a partial hash to decide whether HBN could be left alone. Unversioned changes outside that list, including SitePrefix isolation inputs, could therefore miss an apply. Build the fingerprint from the complete response, remove only fields that cannot affect HBN, and normalize order-insensitive collections to match the renderer. The exhaustive field classification makes future protobuf additions choose a comparison policy at compile time. This supports NVIDIA#3893 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Addressed the initial-cache safety test and shared prefix-entry sorting helper in f8caf2d. I left the NTP comment unchanged because it already states the relevant ownership boundary: DHCP is reconciled before the HBN skip decision on every iteration. |
1 participant
The agent uses Core version fields to decide when HBN rendering and apply can be skipped, but some rendering inputs are not covered by those versions. The existing fallback hash hand-picked a subset, so a SitePrefix isolation change or another omitted nested input could leave HBN stale even though the desired response changed.
This builds a canonical fingerprint from the complete managed-host network response, removes only fields that cannot affect HBN, and normalizes collections using the renderer's own order semantics. The top-level destructuring is exhaustive, so a future protobuf field must choose a comparison policy at compile time. DHCP remains on its existing unconditional reconciliation path.
Related issues
This supports #3893
Type of Change
Breaking Changes
Testing
Additional Notes
Passed the four focused comparison tests, all 202
carbide-agentlibrary tests, nightly formatting, full-workspace Clippy, and custom Carbide lints.