fix(nvue-client): Don't apply an unchanged config (#5789)

This is a backport to v2.1 of #5772, whose original text follows:

Previously (#1965, #3047) we've tried to avoid updating the NVUE config
via the API by detecting changes in the input data and skipping the
apply logic when it's unchanged from the last time we updated it.
However, there was still a gap in this logic since the agent process had
to complete at least one NVUE config update before it would update its
cache, so a newly-started agent process could still send NVUE an
unchanged config, and now that we're actually polling the revision state
(#4729), that turns out to be a big problem.

This branch adds additional logic to the NVUE client so that we also
detect a no-change config revision by asking NVUE for its own diff
between what we're planning to apply and what it's currently running. We
then skip the apply operation if the diff indicates a no-op.

We do still potentially abandon one NVUE revision per agent process
lifetime; I haven't convinced myself it's safe to try to remove the
revision since HBN's OpenAPI spec flavor has patched this endpoint out.
😕

## Related issues
- #1965
- #3047
- #4729
- NVBugs ID 6718057

## Type of Change
- [ ] **Add** - New feature or capability
- [ ] **Change** - Changes in existing functionality
- [X] **Fix** - Bug fixes
- [ ] **Remove** - Removed features or deprecated functionality
- [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.)

## Breaking Changes
- [ ] **This PR contains breaking changes**

## Testing
- [X] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Manual testing performed
- [ ] No testing required (docs, internal refactor, etc.)

## Additional Notes

Author: DrewBloechl

crates/agent/src/ethernet_virtualization.rs

@@ -232,10 +232,10 @@ impl NvueClientContext {
     }
 
     // Wrap the inner nvue_client's `push_config()` and try to avoid re-applying
-    // a configuration we're already using. Returns Ok(Some(revision_id)) on
-    // a change, Ok(None) if the config was unchanged, and otherwise passes
-    // through errors from the inner client.
-    pub async fn update_config(
+    // a configuration we're already using. Returns Ok(Some(revision_id)) when
+    // a revision was applied, Ok(None) if the config was unchanged, and
+    // otherwise passes through errors from the inner client.
+    async fn update_config(
         &mut self,
         config: &NvueConfig,
     ) -> Result<Option<String>, NvueClientError> {
@@ -246,13 +246,9 @@ impl NvueClientContext {
         {
             Ok(None)
         } else {
-            self.nvue_client
-                .push_config(config)
-                .await
-                .map(|revision_id| {
-                    self.last_applied_hash.replace(new_hash);
-                    Some(revision_id)
-                })
+            let revision_id = self.nvue_client.push_config(config).await?;
+            self.last_applied_hash.replace(new_hash);
+            Ok(revision_id)
         }
     }
 }

crates/nvue-client/src/client.rs

@@ -26,7 +26,9 @@ pub use serde_json::Value as JsonValue;
 
 use crate::config::{NvueConfig, NvueConfigWithHeader, NvueRevision};
 use crate::types::bgp::{BgpNeighbors, BgpVrfInfo};
-use crate::types::revision::{RevisionApplyStatus, RevisionData, RevisionIssueSummary};
+use crate::types::revision::{
+    RevisionApplyStatus, RevisionConfigDiff, RevisionData, RevisionIssueSummary,
+};
 
 /// Repeated NVUE field-selection query parameters.
 ///
@@ -199,6 +201,27 @@ impl NvueClient {
         Ok(nvue_config)
     }
 
+    /// Get the config diff between two NVUE revisions. The order of the
+    /// arguments is significant; NVUE will return the set of changes necessary
+    /// to turn `base_revision` into `target_revision`.
+    pub async fn get_revision_config_diff(
+        &self,
+        base_revision: &str,
+        target_revision: &str,
+    ) -> Result<RevisionConfigDiff, NvueClientError> {
+        let mut request = self.request(Method::GET, "/nvue_v1/")?.build()?;
+        request
+            .url_mut()
+            .query_pairs_mut()
+            .append_pair("diff", base_revision)
+            .append_pair("rev", target_revision)
+            .append_pair("filled", "false");
+
+        let response = self.execute("get_revision_config_diff", request).await?;
+        let diff = response.json().await?;
+        Ok(diff)
+    }
+
     /// Return BGP data for a VRF.
     ///
     /// This calls `GET /nvue_v1/vrf/{vrf-id}/router/bgp` without field filters.
@@ -360,15 +383,34 @@ impl NvueClient {
         }
     }
 
-    /// Create a new configuration using the values from `config`, then  apply
-    /// it, returning the revision ID. This is a convenience method that
-    /// creates, replaces, and then applies the configuration (which a caller
-    /// could do manually if more control is desired).
-    pub async fn push_config(&self, config: &NvueConfig) -> Result<String, NvueClientError> {
+    /// Perform all of the NVUE operations required to apply a new config. Under
+    /// the hood, we create a new revision, patch it with this config, diff it
+    /// against the "applied" revision, and then apply it if it differed.
+    ///
+    /// Returns a `Some(revision_id)` if we applied the new revision, and `None`
+    /// if NVUE indicated no change compared to the "applied" revision.
+    pub async fn push_config(
+        &self,
+        config: &NvueConfig,
+    ) -> Result<Option<String>, NvueClientError> {
         let revision_id = self.create_config_revision().await?;
         self.replace_config_revision(&revision_id, config).await?;
+
+        // NVUE will not actually apply a revision if it's unchanged from what's
+        // currently applied, we need to check this before we try.
+        let diff = self
+            .get_revision_config_diff("applied", &revision_id)
+            .await?;
+        if diff.is_empty() {
+            // Note that we're leaving the old revision sitting around unused!
+            // NVUE provides a `DELETE` operation on a revision ID, but HBN
+            // patches this out in its OpenAPI spec For Some Reason(tm) so it's
+            // unclear if it's safe to try it.
+            return Ok(None);
+        }
+
         self.apply_config_revision(&revision_id).await?;
-        Ok(revision_id)
+        Ok(Some(revision_id))
     }
 
     // Retrieve the system information from the NVUE server. The fields returned

crates/nvue-client/src/types/revision.rs

@@ -2,6 +2,23 @@ use std::collections::BTreeMap;
 
 use serde_json::Value as JsonValue;
 
+/// The configuration difference returned by NVUE's config diff operation.
+#[derive(Clone, Debug, serde::Deserialize)]
+#[serde(transparent)]
+pub struct RevisionConfigDiff(
+    // We're intentionally typing this fairly loosely for code brevity, since the
+    // sole user of this is concerned only with whether the diff is empty.
+    serde_json::Map<String, serde_json::Value>,
+);
+
+impl RevisionConfigDiff {
+    /// Return `true` for an empty diff, which means a no-op change between the
+    /// revisions.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
 #[derive(Clone, Debug, serde::Deserialize)]
 #[serde(rename_all = "kebab-case")]
 pub struct RevisionData {
@@ -97,6 +114,74 @@ pub enum RevisionIssueSeverity {
 mod tests {
     use super::*;
 
+    #[test]
+    fn revision_config_diff_requires_object_root_and_reports_empty_root() {
+        struct Case {
+            name: &'static str,
+            json: &'static str,
+            expected: Option<(bool, JsonValue)>,
+        }
+
+        let cases = [
+            Case {
+                name: "empty root object means no difference",
+                json: r#"{}"#,
+                expected: Some((true, serde_json::json!({}))),
+            },
+            Case {
+                name: "nested null remains a significant removal",
+                json: r#"{"interface":{"swp1":{"link":{"state":null}}}}"#,
+                expected: Some((
+                    false,
+                    serde_json::json!({
+                        "interface": {
+                            "swp1": {
+                                "link": {
+                                    "state": null
+                                }
+                            }
+                        }
+                    }),
+                )),
+            },
+            Case {
+                name: "null root is invalid",
+                json: "null",
+                expected: None,
+            },
+            Case {
+                name: "array root is invalid",
+                json: "[]",
+                expected: None,
+            },
+            Case {
+                name: "scalar root is invalid",
+                json: r#""value""#,
+                expected: None,
+            },
+        ];
+
+        for case in cases {
+            match (
+                serde_json::from_str::<RevisionConfigDiff>(case.json),
+                case.expected,
+            ) {
+                (Ok(diff), Some((expected_empty, expected_value))) => {
+                    assert_eq!(diff.is_empty(), expected_empty, "{}", case.name);
+                    assert_eq!(JsonValue::Object(diff.0), expected_value, "{}", case.name);
+                }
+                (Err(_), None) => {}
+                (Ok(_), None) => panic!("{}: expected deserialization to fail", case.name),
+                (Err(error), Some(_)) => {
+                    panic!(
+                        "{}: expected deserialization to succeed: {error}",
+                        case.name
+                    )
+                }
+            }
+        }
+    }
+
     #[test]
     fn classifies_revision_apply_status() {
         struct Case {