fix(nvue-client): Add polling in NvueClient::apply_config_revision - #4729
Conversation
|
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 client adds revision response models, status classification, revision retrieval, and polling for configuration application. It reports applied, failed, and timed-out revisions with structured error details. The public types module now exposes revision types and ChangesRevision Apply Status
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NvueClient
participant NVUE_API
participant RevisionData
NvueClient->>NVUE_API: apply configuration revision
NVUE_API-->>NvueClient: revision identifier
NvueClient->>NVUE_API: get revision status
NVUE_API-->>NvueClient: revision response
NvueClient->>RevisionData: classify status and extract issues
RevisionData-->>NvueClient: applied, pending, or failed
NvueClient->>NVUE_API: poll pending revision after interval
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 3
🤖 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/nvue-client/src/client.rs`:
- Around line 150-155: Update the NvueClientError::SchemaMismatch message in the
revision response handling to use the lowercase text “revision response was
null”.
- Around line 192-224: Update the revision polling loop around get_revision to
check the deadline before each request and wrap the request in
tokio::time::timeout using the remaining duration. Convert an expired
pre-request deadline or timed-out response into
RevisionApplyFailureReason::Timeout, including the elapsed wait and latest
available revision context, so delayed Applied responses cannot return success;
add a test covering a delayed response within the configured deadline budget.
- Around line 145-147: Update get_revision and apply_config_revision to
percent-encode revision_id as a single URL path segment before constructing
their NVUE URLs, and update replace_config_revision to encode revision_id as a
query parameter value. Preserve the existing request behavior while ensuring IDs
containing URL-reserved characters are not interpreted as URL syntax.
🪄 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: 8f6eea00-24fb-422a-84d9-6136dda42c79
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/nvue-client/Cargo.tomlcrates/nvue-client/src/client.rscrates/nvue-client/src/lib.rscrates/nvue-client/src/types/mod.rscrates/nvue-client/src/types/revision.rs
| pub async fn get_revision(&self, revision_id: &str) -> Result<RevisionData, NvueClientError> { | ||
| let revision_path = format!("/nvue_v1/revision/{revision_id}"); | ||
| let request = self.request(Method::GET, &revision_path)?.build()?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- client structure ---'
ast-grep outline crates/nvue-client/src/client.rs
printf '%s\n' '--- relevant source ---'
cat -n crates/nvue-client/src/client.rs | sed -n '120,205p'
printf '%s\n' '--- revision path usages ---'
rg -n -C 3 'revision_id|/nvue_v1/revision' crates/nvue-client
printf '%s\n' '--- URL construction and dependency context ---'
rg -n 'urlencoding|percent_encode|path_segments|reqwest|Url::|revision' Cargo.toml crates/nvue-client/Cargo.toml crates/nvue-client/srcRepository: NVIDIA/infra-controller
Length of output: 21543
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- URL helpers and request flow ---'
cat -n crates/nvue-client/src/client.rs | sed -n '1,120p'
cat -n crates/nvue-client/src/client.rs | sed -n '238,305p'
printf '%s\n' '--- dependency versions ---'
rg -n -A 8 -B 2 '^name = "(reqwest|url|urlencoding)"' Cargo.lock
printf '%s\n' '--- existing URL/path encoding tests or helpers ---'
rg -n -i 'percent.?encod|path.?segment|url::|construct_url_string|revision_path|rev=' --glob '*.rs' --glob '*.toml' .Repository: NVIDIA/infra-controller
Length of output: 30914
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.parse import urlsplit
base = "http://localhost"
for revision_id in ["plain", "a/b", "a?b", "a#b", "a b", "a%2Fb"]:
raw = f"{base}/nvue_v1/revision/{revision_id}"
parsed = urlsplit(raw)
print(f"{revision_id!r} -> path={parsed.path!r}, query={parsed.query!r}, fragment={parsed.fragment!r}")
PY
printf '%s\n' '--- workspace dependency selection ---'
sed -n '180,215p' Cargo.toml
cat -n crates/nvue-client/Cargo.toml | sed -n '1,55p'
printf '%s\n' '--- NVUE revision ID parsing and tests ---'
cat -n crates/nvue-client/src/config.rs | sed -n '125,165p'
rg -n -C 4 'NvueClient|get_revision\\(|apply_config_revision\\(|replace_config_revision\\(' crates/nvue-client crates/healthRepository: NVIDIA/infra-controller
Length of output: 4719
🌐 Web query:
reqwest 0.13 RequestBuilder client.request URL string percent-encoding path query fragment behavior url crate Url::parse
💡 Result:
In reqwest 0.13, the client.request and related methods (get, post, etc.) accept a URL argument that implements the IntoUrl trait, which typically handles standard URL strings [1][2]. When these methods are called, the URL is parsed using the url crate [1]. 1. URL Parsing and Percent-Encoding: The reqwest crate uses the url crate for parsing URLs. When you pass a string to client.request or similar methods, it is parsed into a Url object. The url crate automatically handles percent-encoding for path, query, and fragment components according to RFC standards during this parsing and subsequent usage [1][2]. 2. RequestBuilder Behavior: - RequestBuilder::new or those created via Client methods maintain the URL state [2]. - If the provided URL contains embedded credentials (e.g., https://user:pass@example.com), reqwest automatically extracts these, percent-decodes them, removes them from the URL, and converts them into HTTP Basic Authentication headers [2]. - The RequestBuilder::query method, available when the query feature is enabled, provides a structured way to add or modify query parameters [3][4]. It serializes provided key-value pairs and appends them to the URL's query component, ensuring proper percent-encoding is applied [3][5]. 3. Special Configurations: While the library handles standard percent-encoding for general request components, specific features like multipart::Form offer additional configuration for percent-encoding behavior, such as percent_encode_noop, to support servers with non-standard requirements [6][7][8]. In general, users should rely on the default URL parsing behavior provided by the url crate, which ensures compliance with standard URI percent-encoding rules for paths, queries, and fragments [1][2].
Citations:
- 1: https://docs.rs/reqwest/0.13.2/reqwest/struct.Client.html
- 2: https://deepwiki.com/seanmonstar/reqwest/3.2-request-building
- 3: https://docs.rs/reqwest/latest/reqwest/struct.RequestBuilder.html
- 4: https://github.com/seanmonstar/reqwest/releases/tag/v0.13.0
- 5: https://docs.rs/reqwest/latest/reqwest/blocking/struct.RequestBuilder.html
- 6: https://github.com/seanmonstar/reqwest/blob/HEAD/CHANGELOG.md
- 7: https://github.com/seanmonstar/reqwest/blob/master/CHANGELOG.md
- 8: https://github.com/seanmonstar/reqwest/blob/d31ffbbf/CHANGELOG.md
🌐 Web query:
site:docs.nvidia.com/networking-ethernet-software/cumulus-linux-516 "revision" "NVUE API" revision ID URL
💡 Result:
In the NVIDIA User Experience (NVUE) API for Cumulus Linux, the revision ID is a system-generated identifier used to manage, track, and apply configuration changes [1][2]. The NVUE API typically resides at the endpoint path starting with /nvue_v1/ [1]. To perform operations involving revisions, you interact with the /nvue_v1/revision endpoint [1]. Key concepts for using the revision ID via the REST API include: 1. Creating a Revision: You initiate a new configuration session by sending a POST request to the /nvue_v1/revision endpoint, which returns a new revision ID (e.g., "1" or "2") [1]. 2. Applying Changes: When making configuration changes (using PATCH requests), you link the changes to a specific revision ID. This can be done by including the revision in the URL query parameter (e.g., /nvue_v1/interface/lo/ip/address?rev=2) or by interacting directly with the revision resource (e.g., /nvue_v1/revision/2) [1]. 3. Committing or Managing: You can check the status of a revision (e.g., pending, applied) or finalize the application of changes by sending PATCH requests to the specific revision's URL (e.g., /nvue_v1/revision/2) [1]. For further details on interacting with the NVUE API, including full request structures and authentication, refer to the official NVIDIA documentation for your specific version of Cumulus Linux [1].
Citations:
- 1: https://docs.nvidia.com/networking-ethernet-software/cumulus-linux-516/System-Configuration/NVIDIA-User-Experience-NVUE/NVUE-API/
- 2: https://docs.nvidia.com/networking-ethernet-software/cumulus-linux-516/System-Configuration/NVIDIA-User-Experience-NVUE/NVUE-CLI/
🌐 Web query:
docs.rs url 2.5 Url::parse path slash question mark fragment path_segments_mut push percent encode reserved characters
💡 Result:
In the Rust url crate (version 2.5), the Url struct and its associated PathSegmentsMut helper provide functionality to parse and manipulate URL paths [1][2][3]. When you call Url::parse, the URL is parsed according to the WHATWG URL Standard [4][5]. Components such as the query and fragment are delimited by the ? and # characters, respectively [6][7][8]. For URLs that are not "cannot-be-a-base" (i.e., they have a host), the path is treated as a series of slash-separated segments [1][5]. You can access these segments mutably using url.path_segments_mut() [2][6]. Regarding the push method on PathSegmentsMut: - The push (and extend) method appends a segment to the end of the URL path [1][9]. - Crucially, push performs percent-encoding on the input segment [1][9]. - Unlike the initial Url::parse, which may leave existing percent-encoding as-is, push and extend will percent-encode reserved characters such as / (to %2F) and % (to %25) [1][9]. This ensures that the added segment remains a single, valid path component [1][10]. Example usage: rust use url::Url; let mut url = Url::parse("http://example.net/foo")?; // Path becomes "/foo/img/2%2F100%25.png" url.path_segments_mut().map_err(|_| "cannot be base")? .push("img").push("2/100%.png"); [1][9] If you need to perform additional manipulation, such as handling trailing slashes, you can use methods like pop() or pop_if_empty() on the PathSegmentsMut object before pushing new segments [1][9].
Citations:
- 1: https://docs.rs/url/2.5.2/url/struct.PathSegmentsMut.html
- 2: https://docs.rs/url/2.5.8/i686-pc-windows-msvc/url/struct.Url.html
- 3: https://docs.rs/url/latest/url/
- 4: percent ("%) character must be percent-encoded as "%25" servo/rust-url#822
- 5: https://doc.servo.org/url/struct.Url.html
- 6: https://docs.rs/url/2.5.7/url/
- 7: https://docs.rs/url/2.5.0/url/index.html
- 8: https://github.com/servo/rust-url/blob/master/url/src/lib.rs
- 9: https://doc.servo.org/url/path_segments/struct.PathSegmentsMut.html
- 10: DEFAULT_ENCODE_SET doesn't percent-encode / and % servo/rust-url#154
Encode revision_id before building NVUE URLs.
format! treats revision_id as URL syntax. Encode it as one path segment in get_revision and apply_config_revision, and as a query value in replace_config_revision.
🤖 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/nvue-client/src/client.rs` around lines 145 - 147, Update get_revision
and apply_config_revision to percent-encode revision_id as a single URL path
segment before constructing their NVUE URLs, and update replace_config_revision
to encode revision_id as a query parameter value. Preserve the existing request
behavior while ensuring IDs containing URL-reserved characters are not
interpreted as URL syntax.
There was a problem hiding this comment.
I'm aiming to fix up all of the URL construction bugs in this crate in a separate branch, so while this is valid I don't want to fix it without fixing the ones in the other methods (outside the scope for this branch).
There was a problem hiding this comment.
@DrewBloechl, acknowledged. A partial fix in this PR would make URL construction inconsistent across the crate.
I will leave this finding unresolved for the separate URL-construction cleanup. Would you like me to open a follow-up issue for that work?
🐇
You are interacting with an AI system.
d04f9d8 to
8043dda
Compare
…4791) This is a backport to v2.1 of #4729 (which the following text is from). I've been overly optimistic in `NvueClient::apply_config_revision()`, assuming that we don't need to care about checking the state of the revision we just applied. QA found a bug (tracked internally as NVbugs 6563638) that suggests this was a mistake. This branch adds polling logic in `apply_config_revision()`, with parsing of the revision data derived from the OpenAPI spec from NVUE in Cumulus Linux 5.16.0 (the most recent version I had handy). ## Related issues - NVbugs 6563638 ## 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
…VIDIA#4729) I've been overly optimistic in `NvueClient::apply_config_revision()`, assuming that we don't need to care about checking the state of the revision we just applied. QA found a bug (tracked internally as NVbugs 6563638) that suggests this was a mistake. This branch adds polling logic in `apply_config_revision()`, with parsing of the revision data derived from the OpenAPI spec from NVUE in Cumulus Linux 5.16.0 (the most recent version I had handy). ## Related issues - NVbugs 6563638 ## 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
2 participants
I've been overly optimistic in
NvueClient::apply_config_revision(), assuming that we don't need to care about checking the state of the revision we just applied. QA found a bug (tracked internally as NVbugs 6563638) that suggests this was a mistake. This branch adds polling logic inapply_config_revision(), with parsing of the revision data derived from the OpenAPI spec from NVUE in Cumulus Linux 5.16.0 (the most recent version I had handy).Related issues
Type of Change
Breaking Changes
Testing
Additional Notes