feat(ztp): make ZTP resilient to slow/unhealthy Nautobot#125
Draft
dmathur0 wants to merge 6 commits into
Draft
Conversation
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
July 13, 2026 18:32
📝 WalkthroughWalkthroughChangesZTP now reuses resilient Nautobot and storage clients with lifecycle management, bounded operations, caching, and retryable 503 handling. Helm configuration adds configurable HTTP load-balancer workers and startup, liveness, and readiness probe settings. ZTP backend resilience
ZTP deployment tuning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FastAPI
participant ZTPEndpoint
participant SharedClient
participant Backend
FastAPI->>SharedClient: initialize and warm client
ZTPEndpoint->>SharedClient: request device or storage data
SharedClient->>Backend: execute guarded operation
Backend-->>SharedClient: return data or transient failure
SharedClient-->>ZTPEndpoint: return result or availability error
FastAPI->>SharedClient: close client on shutdown
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Under a boot storm (or during a Nautobot rollout) ZTP could crashloop and amplify load on a struggling Nautobot. Root causes: a 1s-default liveness probe killed pods whenever the event loop stalled, a fresh Nautobot client per request meant no connection reuse and no global concurrency cap, a 30s timeout with unbounded in-flight requests, and read amplification from repeated identical device-data queries. Changes: - helm: split liveness/readiness and add a startupProbe for the ztp http-api/http-lb containers; liveness is now lenient (5s timeout, 15s period, 6 failures) so a slow dependency degrades instead of crashlooping. - Share one process-wide Nautobot client (clients.get_nautobot_client) with a bounded connection pool, created/closed via the FastAPI lifespan. - Add a concurrency semaphore + short request timeout for backpressure; when saturated or when the circuit breaker is open, raise NautobotUnavailableError which surfaces as HTTP 503 + Retry-After so devices retry. - Add a short-TTL device-data cache with single-flight coalescing to collapse the per-boot config_manager_device query amplification to ~1/device/window. - Add a circuit breaker that fails fast on transient Nautobot failures (timeouts/connection/5xx), excluding logical 404s. All tunables read from the [nautobot] INI section with safe fallbacks, so no config change is required. Adds tests for caching, single-flight, breaker, load shedding, and the 503 path.
dmathur0
force-pushed
the
davesh/ztp-nautobot-resilience
branch
from
July 13, 2026 18:41
528fe8b to
0954423
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: 7
🧹 Nitpick comments (1)
deploy/helm/templates/ztp.yaml (1)
83-94: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
timeoutSecondson both startup probes. Kubernetes defaultstimeoutSecondsto 1s, so these probes can still fail too quickly under transient load.
deploy/helm/templates/ztp.yaml#L89-L94: add an explicittimeoutSeconds(for example,5).deploy/helm/templates/ztp.yaml#L141-L146: add the same explicit timeout.🤖 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 `@deploy/helm/templates/ztp.yaml` around lines 83 - 94, Set an explicit 5-second timeoutSeconds on both startupProbe configurations in deploy/helm/templates/ztp.yaml: the anchor site at lines 83-94 and the sibling site at lines 139-146. Keep the existing HTTP probe paths and other settings unchanged.Sources: Coding guidelines, MCP tools
🤖 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 `@deploy/helm/values-local-azure-sso.yaml`:
- Around line 24-28: Update the browser walkthrough comments in the local Azure
SSO configuration to use config-manager.local instead of localhost:8080, and
instruct port-forwarding the gateway’s TLS listener. Preserve the existing Entra
login and redirect behavior while ensuring the documented host matches the
overlay and /etc/hosts configuration.
In `@src/nv_config_manager/ztp/api/clients.py`:
- Around line 47-50: Update reset_nautobot_client to await
close_nautobot_client() instead of directly clearing _nautobot_client, and make
the helper asynchronous so shared aiohttp resources are closed during test
isolation.
In `@src/nv_config_manager/ztp/api/device_v1.py`:
- Line 50: Update the device data handling around
get_nautobot_client().get_device_data so the returned DeviceData and its address
list are copied before the authorization flow appends "127.0.0.1". Ensure the
existing cached object remains unchanged while preserving the current
authorization behavior.
In `@src/nv_config_manager/ztp/nautobot.py`:
- Around line 235-247: Update the process-wide cache used by _cache_get and
_cache_put to enforce a maximum size and independently purge expired entries,
rather than relying only on per-key lazy expiry. Ensure stale entries are
removed during cache activity and bounded eviction removes older entries when
the limit is reached, while preserving the existing TTL behavior for active
lookups.
- Around line 102-108: Update _TRANSIENT_ERRORS and the Nautobot request error
handling to classify failures by response status: exclude logical/4xx
NautobotException failures, while counting network errors, timeouts, and 5xx
responses. Preserve the response status on the base exception, and translate
only those genuine outage failures into NautobotUnavailableError so the API
returns 503 and the circuit breaker counts them.
- Around line 265-274: Update the shared task await in the device-data loading
method to use asyncio.shield(task), preventing cancellation of one caller from
cancelling the coalesced _load_device_data operation. Enhance the _discard
callback to consume the completed task’s exception when appropriate, while
preserving removal from _inflight and normal result propagation for remaining
callers.
In `@src/tests/ztp/test_nautobot.py`:
- Around line 146-152: Update the holder task cleanup in the test around
nb.get_device_data so cancellation is followed by awaiting holder, ensuring the
task completes before the test exits; preserve the existing finally-based
cleanup and handle the expected cancellation outcome.
---
Nitpick comments:
In `@deploy/helm/templates/ztp.yaml`:
- Around line 83-94: Set an explicit 5-second timeoutSeconds on both
startupProbe configurations in deploy/helm/templates/ztp.yaml: the anchor site
at lines 83-94 and the sibling site at lines 139-146. Keep the existing HTTP
probe paths and other settings unchanged.
🪄 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: dff49928-597d-46b4-811c-7fbbdf9ca6fb
📒 Files selected for processing (19)
components/nautobot/nv_config_manager_auth/jwt_authentication.pycomponents/nautobot/nv_config_manager_auth/rbac.pycomponents/nautobot/tests/conftest.pycomponents/nautobot/tests/test_jwt_authentication.pycomponents/nautobot/tests/test_rbac.pydeploy/helm/templates/_nautobot-configmap-data.tpldeploy/helm/templates/nautobot.yamldeploy/helm/templates/ztp.yamldeploy/helm/values-local-azure-sso.yamldeploy/helm/values.yamldocs/nautobot/nautobot-integration.mdxsrc/nv_config_manager/common/client/nautobot.pysrc/nv_config_manager/ztp/api/clients.pysrc/nv_config_manager/ztp/api/device_v1.pysrc/nv_config_manager/ztp/api/main.pysrc/nv_config_manager/ztp/nautobot.pysrc/tests/ztp/api/test_main.pysrc/tests/ztp/conftest.pysrc/tests/ztp/test_nautobot.py
Comment on lines
+24
to
+28
| # # 3. Port-forward the gateway for localhost access: | ||
| # kubectl -n envoy-gateway-system port-forward svc/<your-envoy-gateway-svc> 8080:80 | ||
| # | ||
| # # 4. Browse to http://localhost:8080 -> Entra login -> redirected back with | ||
| # # the Nautobot permissions mapped from your Entra App Role(s). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the gateway Service template and inspect its listener ports.
fd -i -e yaml -e yml . deploy/helm |
xargs -r rg -n -C4 'kind:\s*Service|port:\s*(80|443)|targetPort:'Repository: NVIDIA/nv-config-manager
Length of output: 27773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deploy/helm/values-local-azure-sso.yaml ==\n'
cat -n deploy/helm/values-local-azure-sso.yaml | sed -n '1,140p'
printf '\n== config-manager.local / localhost references in deploy/helm ==\n'
rg -n -C 3 'config-manager\.local|localhost|port-forward|Entra|Azure SSO|sso' deploy/helmRepository: NVIDIA/nv-config-manager
Length of output: 24069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deploy/helm/values-local.yaml (access + gateway sections) ==\n'
cat -n deploy/helm/values-local.yaml | sed -n '1,170p'
printf '\n== gateway-related references in deploy/helm/templates ==\n'
rg -n -C 3 'gateway\.|kind:\s*Gateway|kind:\s*HTTPRoute|listener|tls:' deploy/helm/templates/httproutes.yaml deploy/helm/templates/*.yamlRepository: NVIDIA/nv-config-manager
Length of output: 50381
Use config-manager.local in the browser step. The overlay and /etc/hosts are built around config-manager.local, so http://localhost:8080 bypasses the configured host-based route and redirect URI. Forward the gateway’s TLS listener and browse through config-manager.local instead.
🤖 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 `@deploy/helm/values-local-azure-sso.yaml` around lines 24 - 28, Update the
browser walkthrough comments in the local Azure SSO configuration to use
config-manager.local instead of localhost:8080, and instruct port-forwarding the
gateway’s TLS listener. Preserve the existing Entra login and redirect behavior
while ensuring the documented host matches the overlay and /etc/hosts
configuration.
Comment on lines
+47
to
+50
| def reset_nautobot_client() -> None: | ||
| """Drop the shared client without awaiting (test isolation helper).""" | ||
| global _nautobot_client | ||
| _nautobot_client = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not drop an open shared client without closing it.
If a test opened the aiohttp session, resetting only the reference leaks its connector and can orphan in-flight tasks. Make test isolation await close_nautobot_client() rather than using this synchronous reset.
🤖 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/ztp/api/clients.py` around lines 47 - 50, Update
reset_nautobot_client to await close_nautobot_client() instead of directly
clearing _nautobot_client, and make the helper asynchronous so shared aiohttp
resources are closed during test isolation.
| nb_client = NautobotClient() | ||
| async with nb_client: | ||
| device_data = await nb_client.get_device_data(device_uuid) | ||
| device_data = await get_nautobot_client().get_device_data(device_uuid) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid mutating addresses on the cached DeviceData.
The shared client can return the same cached object repeatedly, while Lines 54-55 append "127.0.0.1" directly to its address list. Each authorization therefore grows shared cached state.
Use a copy:
- allowed_addresses = device_data.addresses
- allowed_addresses.append("127.0.0.1")
+ allowed_addresses = [*device_data.addresses, "127.0.0.1"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| device_data = await get_nautobot_client().get_device_data(device_uuid) | |
| allowed_addresses = [*device_data.addresses, "127.0.0.1"] |
🤖 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/ztp/api/device_v1.py` at line 50, Update the device
data handling around get_nautobot_client().get_device_data so the returned
DeviceData and its address list are copied before the authorization flow appends
"127.0.0.1". Ensure the existing cached object remains unchanged while
preserving the current authorization behavior.
Comment on lines
+235
to
+247
| def _cache_get(self, device_id: str) -> DeviceData | None: | ||
| entry = self._cache.get(device_id) | ||
| if entry is None: | ||
| return None | ||
| expiry, data = entry | ||
| if time.monotonic() >= expiry: | ||
| self._cache.pop(device_id, None) | ||
| return None | ||
| return data | ||
|
|
||
| def _cache_put(self, device_id: str, data: DeviceData) -> None: | ||
| if self._cache_ttl > 0: | ||
| self._cache[device_id] = (time.monotonic() + self._cache_ttl, data) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound or actively purge the process-wide device cache.
TTL expiry is lazy per key: devices that are never requested again remain in _cache forever. A long-running ZTP process will therefore retain every distinct device encountered, despite the documented short TTL.
Add bounded eviction and purge expired entries independently of future reads for the same key.
🤖 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/ztp/nautobot.py` around lines 235 - 247, Update the
process-wide cache used by _cache_get and _cache_put to enforce a maximum size
and independently purge expired entries, rather than relying only on per-key
lazy expiry. Ensure stale entries are removed during cache activity and bounded
eviction removes older entries when the limit is reached, while preserving the
existing TTL behavior for active lookups.
Comment on lines
+265
to
+274
| task = self._inflight.get(device_id) | ||
| if task is None: | ||
| task = asyncio.ensure_future(self._load_device_data(device_id)) | ||
| self._inflight[device_id] = task | ||
|
|
||
| def _discard(_task: asyncio.Task[DeviceData], key: str = device_id) -> None: | ||
| self._inflight.pop(key, None) | ||
|
|
||
| task.add_done_callback(_discard) | ||
| return await task |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Shield the shared single-flight task from individual caller cancellation.
Awaiting task directly lets one disconnected/cancelled request cancel the underlying query, causing every coalesced caller to fail. Await a shielded task and ensure its callback consumes an otherwise-unobserved exception when all waiters disappear.
Proposed core change
- return await task
+ return await asyncio.shield(task)🤖 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/ztp/nautobot.py` around lines 265 - 274, Update the
shared task await in the device-data loading method to use asyncio.shield(task),
preventing cancellation of one caller from cancelling the coalesced
_load_device_data operation. Enhance the _discard callback to consume the
completed task’s exception when appropriate, while preserving removal from
_inflight and normal result propagation for remaining callers.
Comment on lines
+146
to
+152
| holder = asyncio.ensure_future(nb.get_device_data(str(uuid4()))) | ||
| await asyncio.sleep(0.01) # let the holder grab the only slot | ||
| try: | ||
| with pytest.raises(NautobotUnavailableError): | ||
| await nb.get_device_data(str(uuid4())) | ||
| finally: | ||
| holder.cancel() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Await cancellation of the holder task.
holder.cancel() only requests cancellation; not awaiting it can leave cleanup pending and produce flaky task warnings.
Proposed fix
finally:
holder.cancel()
+ with pytest.raises(asyncio.CancelledError):
+ await holder📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| holder = asyncio.ensure_future(nb.get_device_data(str(uuid4()))) | |
| await asyncio.sleep(0.01) # let the holder grab the only slot | |
| try: | |
| with pytest.raises(NautobotUnavailableError): | |
| await nb.get_device_data(str(uuid4())) | |
| finally: | |
| holder.cancel() | |
| holder = asyncio.ensure_future(nb.get_device_data(str(uuid4()))) | |
| await asyncio.sleep(0.01) # let the holder grab the only slot | |
| try: | |
| with pytest.raises(NautobotUnavailableError): | |
| await nb.get_device_data(str(uuid4())) | |
| finally: | |
| holder.cancel() | |
| with pytest.raises(asyncio.CancelledError): | |
| await holder |
🤖 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/tests/ztp/test_nautobot.py` around lines 146 - 152, Update the holder
task cleanup in the test around nb.get_device_data so cancellation is followed
by awaiting holder, ensuring the task completes before the test exits; preserve
the existing finally-based cleanup and handle the expected cancellation outcome.
dmathur0
marked this pull request as draft
July 15, 2026 17:32
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
_guarded now retries a transient failure (timeout, connection error, 5xx) up to request_max_attempts (default 2) so a fresh attempt can land on a healthy pod/connection. If every attempt fails it records a single breaker failure and raises NautobotUnavailableError (HTTP 503 Retry-After) rather than letting a raw asyncio.TimeoutError leak out as an uncaught 500 — so a booting device backs off and retries instead of failing its boot. This closes the last failure mode seen under load: Nautobot itself stayed fast (~17ms), but an occasional cold pod / churned keepalive connection would hang past the 8s request timeout and return a 500. Tunable via request_max_attempts / request_retry_backoff in the [nautobot] INI section.
…arts Run the liveness/readiness probe handler on the event loop instead of the anyio threadpool. Under a boot storm the threadpool can be saturated by blocking per-request I/O, starving a sync probe handler and triggering spurious liveness-kill restarts of the http-lb container.
Mirror the shared Nautobot client for the two other backends a device hits during a boot storm. Previously the checksum handlers built a new S3Client (new aioboto3 session + connect/credential resolution + fresh pool) per request, and DeviceData.load_file built a new ConfigStoreClient (new TCPConnector + SSL context) per request — so under 100u concurrency the per-request setup piled unbounded I/O onto the event loop, saturating it and tipping http-lb into liveness-kill restarts and 500s. - Add ztp/api/storage_clients.py: process-wide pre-connected object storage client, Config Store clients cached per endpoint, and a bounded-concurrency semaphore + short per-op timeout (guarded_storage) that sheds load as a retryable StorageUnavailableError. - Map StorageUnavailableError -> HTTP 503 Retry-After in the ZTP app; init/close the shared clients in the FastAPI lifespan. - Route firmware/checksum (device + firmware routers) and DeviceData.load_file through the shared clients + guard. - Tests for guarded_storage (passthrough, timeout, backpressure, logical-error propagation) and the 503 handler; reset shared clients between tests.
Add a --workers flag (default 1, or $ZTP_API_WORKERS / $WEB_CONCURRENCY) to the ZTP API entrypoint. Each uvicorn worker runs its own event loop and its own shared backend-client singletons, so per-pod device concurrency is spread across loops. Under a boot storm this keeps any single loop from stalling long enough to fail the liveness probe (the remaining http-lb restart cause after pooling + bounding storage I/O).
Wire the new --workers flag into the device-facing http-lb container so the multi-worker fix is a persistent chart value instead of a live patch. Defaults to 4 (load testing showed 1 worker crashlooped at ~100 concurrent devices / 3 pods while 4 held 0 restarts at ~4x throughput); 0/unset falls back to a single uvicorn worker. http-api is intentionally left single-process.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/templates/ztp.yaml (1)
89-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet an explicit
timeoutSecondson both startup probes
Kubernetes defaultsstartupProbe.timeoutSecondsto 1s, so these health checks can still fail during slow startup or event-loop stalls. Add a bounded timeout, such astimeoutSeconds: 5, to bothstartupProbeblocks.🤖 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 `@deploy/helm/templates/ztp.yaml` around lines 89 - 94, Add an explicit bounded timeoutSeconds value of 5 to both startupProbe blocks in the ztp.yaml template, alongside the existing periodSeconds and failureThreshold settings. Keep the current HTTP health-check configuration unchanged.
🧹 Nitpick comments (1)
deploy/helm/values.yaml (1)
1385-1396: 🚀 Performance & Scalability | 🔵 TrivialSize backend limits for process-local clients.
When enabled, the default is 4 workers per pod across 3 replicas. The ZTP CLI creates separate backend-client singletons per worker, so this can yield up to 12 independent pools, caches, and circuit breakers; per-process backpressure and single-flight coalescing are not pod-global. Verify aggregate Nautobot/storage connection, concurrency, CPU, and memory budgets before relying on this default.
🤖 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 `@deploy/helm/values.yaml` around lines 1385 - 1396, Update the httpLb workers default and related configuration guidance to account for one independent ZTP backend client per worker across replicas. Size and document aggregate Nautobot/storage connections, concurrency, CPU, and memory budgets for the 4-workers-per-pod, 3-replica deployment, or adjust the default so these process-local pools, caches, and circuit breakers remain within safe limits.
🤖 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/ztp/api/main.py`:
- Around line 51-60: Update the worker default-value logic for the --workers
argument to safely parse ZTP_API_WORKERS and WEB_CONCURRENCY, accepting only
positive integers and falling back to 1 for malformed, zero, or negative values.
Keep the existing precedence of ZTP_API_WORKERS over WEB_CONCURRENCY and ensure
invalid environment values cannot raise before Uvicorn starts.
In `@src/nv_config_manager/ztp/api/storage_clients.py`:
- Around line 173-183: Update the cleanup flow around
_object_storage_client.close() to catch and log close errors with
logger.warning, matching the best-effort handling used for Config Store clients,
while still resetting _object_storage_client to None and continuing to
_config_store_clients cleanup.
- Around line 125-126: Update the storage setup flow around
_build_storage_client() and client.connect() to catch initialization or
connection failures and translate them into StorageUnavailableError, preserving
the 503 and Retry-After handling. Ensure a client that fails setup is not
cached, while successful connections continue through guarded_storage().
---
Outside diff comments:
In `@deploy/helm/templates/ztp.yaml`:
- Around line 89-94: Add an explicit bounded timeoutSeconds value of 5 to both
startupProbe blocks in the ztp.yaml template, alongside the existing
periodSeconds and failureThreshold settings. Keep the current HTTP health-check
configuration unchanged.
---
Nitpick comments:
In `@deploy/helm/values.yaml`:
- Around line 1385-1396: Update the httpLb workers default and related
configuration guidance to account for one independent ZTP backend client per
worker across replicas. Size and document aggregate Nautobot/storage
connections, concurrency, CPU, and memory budgets for the 4-workers-per-pod,
3-replica deployment, or adjust the default so these process-local pools,
caches, and circuit breakers remain within safe limits.
🪄 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: 0e7ce3a5-a2b3-4cdf-9ed4-1ca8e7a82baa
📒 Files selected for processing (10)
deploy/helm/templates/ztp.yamldeploy/helm/values.yamlsrc/nv_config_manager/ztp/api/device_v1.pysrc/nv_config_manager/ztp/api/firmware_v1.pysrc/nv_config_manager/ztp/api/main.pysrc/nv_config_manager/ztp/api/storage_clients.pysrc/nv_config_manager/ztp/device.pysrc/tests/ztp/api/test_main.pysrc/tests/ztp/api/test_storage_clients.pysrc/tests/ztp/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/ztp/conftest.py
- src/nv_config_manager/ztp/api/device_v1.py
Comment on lines
+51
to
+60
| parser.add_argument( | ||
| "--workers", | ||
| type=int, | ||
| default=int(os.environ.get("ZTP_API_WORKERS") or os.environ.get("WEB_CONCURRENCY") or "1"), | ||
| help=( | ||
| "Number of uvicorn worker processes (default: 1, or $ZTP_API_WORKERS / " | ||
| "$WEB_CONCURRENCY). Each worker runs its own event loop and its own shared " | ||
| "backend-client singletons, spreading per-pod device concurrency across loops " | ||
| "so a single loop is less likely to stall (liveness kill) under a boot storm." | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate worker environment values before converting them.
A malformed ZTP_API_WORKERS or WEB_CONCURRENCY value raises ValueError before Uvicorn starts; zero and negative values also pass through unchecked. Parse only positive integers and fall back to 1, matching the PR’s safe-fallback objective.
🤖 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/ztp/api/main.py` around lines 51 - 60, Update the
worker default-value logic for the --workers argument to safely parse
ZTP_API_WORKERS and WEB_CONCURRENCY, accepting only positive integers and
falling back to 1 for malformed, zero, or negative values. Keep the existing
precedence of ZTP_API_WORKERS over WEB_CONCURRENCY and ensure invalid
environment values cannot raise before Uvicorn starts.
Comment on lines
+125
to
+126
| client = _build_storage_client() | ||
| await client.connect() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Translate client initialization failures to retryable unavailability.
A failure in _build_storage_client() or client.connect() happens before guarded_storage(), bypassing the StorageUnavailableError handler and returning a 500 instead of the required 503 with Retry-After. Wrap connection/setup failures in StorageUnavailableError without caching the failed client.
🤖 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/ztp/api/storage_clients.py` around lines 125 - 126,
Update the storage setup flow around _build_storage_client() and
client.connect() to catch initialization or connection failures and translate
them into StorageUnavailableError, preserving the 503 and Retry-After handling.
Ensure a client that fails setup is not cached, while successful connections
continue through guarded_storage().
Comment on lines
+173
to
+183
| if _object_storage_client is not None: | ||
| try: | ||
| await _object_storage_client.close() | ||
| finally: | ||
| _object_storage_client = None | ||
| for client in _config_store_clients.values(): | ||
| try: | ||
| await client.close() | ||
| except Exception as exc: # noqa: BLE001 - shutdown cleanup is best-effort | ||
| logger.warning("Error closing Config Store client: %s", exc) | ||
| _config_store_clients.clear() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Continue cleanup if object-storage close fails.
If _object_storage_client.close() raises, the exception exits before cached Config Store clients are closed. Catch and log this close error, as already done for Config Store clients, then continue cleanup.
🤖 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/ztp/api/storage_clients.py` around lines 173 - 183,
Update the cleanup flow around _object_storage_client.close() to catch and log
close errors with logger.warning, matching the best-effort handling used for
Config Store clients, while still resetting _object_storage_client to None and
continuing to _config_store_clients cleanup.
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
Makes the ZTP service degrade gracefully when Nautobot is slow or flapping (e.g. during a boot storm or a Nautobot rolling update) instead of crashlooping and amplifying load on a struggling Nautobot.
Motivated by load-testing
kiwi-qa01, where sustained ZTP load / a Nautobot rollout drove the ZTP pods intoCrashLoopBackOff. Root causes:config_manager_devicequery ~2N times (authorize + handler + N configlets).Changes
deploy/helm/templates/ztp.yaml): split liveness/readiness and add astartupProbefor thehttp-api/http-lbcontainers. Liveness is now lenient (timeoutSeconds: 5,periodSeconds: 15,failureThreshold: 6) so a slow downstream degrades instead of crashlooping; traffic shedding is handled in-app.ztp/api/clients.py, wired throughdevice_v1.py+main.pylifespan): one process-wide client with a bounded connection pool → keepalive reuse and a real global connection cap.ztp/nautobot.py): a concurrency semaphore and a shorter request timeout (8s vs 30s). When saturated (or when the breaker is open) it raisesNautobotUnavailableError, surfaced as HTTP 503 +Retry-Afterso devices/ONIE retry.ztp/nautobot.py): short-TTL device-data cache with request coalescing collapses the per-bootconfig_manager_deviceamplification to ~1 query/device/window.ztp/nautobot.py): fails fast for a cooldown after consecutive transient failures (timeouts/connection/5xx), excluding logical 404s.All tunables read from the
[nautobot]INI section with safe fallbacks — no config change required to deploy.Test plan
uv run ruff check+uv run mypyclean on touched filesuv run pytest src/tests/ztp— 90 passedhelm template test . --values values-ci.yamlrenders the new probeskiwi-qa01and re-run the boot-storm sweep to confirm no ZTP crashloop and reduced Nautobot query volumeSummary by CodeRabbit