Skip to content

feat(core): add configurable pool import timeout for large pools - #1083

Open
azgms wants to merge 2 commits into
openebs:developfrom
azgms:feat/configurable-pool-import-timeout
Open

feat(core): add configurable pool import timeout for large pools#1083
azgms wants to merge 2 commits into
openebs:developfrom
azgms:feat/configurable-pool-import-timeout

Conversation

@azgms

@azgms azgms commented Apr 15, 2026

Copy link
Copy Markdown

Summary

When an io-engine pod restarts, agent-core re-imports disk pools via ImportPool gRPC. On large pools (e.g. ~200 TiB with many lvols), the SPDK blobstore metadata scan can exceed 60s, hitting the hardcoded timeout cap and causing an infinite timeout-retry loop.

This PR:

  • Fixes the 59s hard cap in timeout_grpc() so operations whose minimum timeout intentionally exceeds 59s are no longer silently clamped (also fixes nvme_reconnect at 62s being capped to 59s)
  • Adds a --pool-import-timeout CLI arg (env: POOL_IMPORT_TIMEOUT) to set an explicit timeout for pool import operations

When --pool-import-timeout is not specified, behavior is unchanged.

Motivation

We run a ~200 TiB Mayastor disk pool across 2 storage nodes. After an io-engine crash, the pool import consistently exceeded the 59s gRPC timeout. Agent-core retried every 10s, flooding the freshly restarted io-engine with import + nexus + replica requests, preventing the import from ever completing. The pool was stuck in Unknown state indefinitely.

Architectural Safety

The 59s cap change is conservative

The cap logic changes from unconditional .min(59s) to conditional: only operations whose op_timeout <= 59s are still capped. This affects exactly two operations today:

  • ImportPool (default 60s): goes from 59s to 60s -- a 1s increase that matches the originally intended pool * 3 calculation
  • nvme_reconnect (default 62s): goes from 59s to 62s -- this was always a bug; the 62s value was explicitly chosen but silently clamped

All other operations (replica, nexus, pool create/destroy, snapshots) have op_timeout <= 30s and remain capped at 59s exactly as before.

The 60s tonic channel timeout is NOT affected

Pool import flows through GrpcContext (client.rs:56-63), which creates its own tonic::transport::Endpoint with .timeout(timeout) set to the result of timeout_grpc(). The separate 60s hardcoded channel timeout in Context::endpoint() (line 145) is only used by the rest-server/csi-to-core-agent path and is not involved.

Node operation serialization

Node operations are serialized via Arc<Mutex> per node (GrpcClientLocked). A longer pool import timeout means other operations on that node queue behind it longer. This is acceptable: during pool import, the pool is in Unknown state, so nexus/replica operations targeting that pool would fail regardless. Letting the import complete is strictly better than the current behavior of timing out and retrying in a 10s loop indefinitely.

No impact on reconciler behavior

The pool reconciler continues to retry imports every reconciliation cycle (default 10s). The only difference is that each attempt now has enough time to succeed on large pools, breaking the infinite timeout-retry loop.

Changes

File Change
grpc/src/context.rs Fix 59s cap, use pool_import(), add 5 unit tests
stor-port/src/transport_api/mod.rs Add pool_import field, accessor, builder
agents/src/bin/core/main.rs Add --pool-import-timeout CLI arg
agents/src/bin/core/node/mod.rs Thread new arg
agents/src/bin/core/node/service.rs Wire to RequestMinTimeout
agents/src/bin/core/node/wrapper.rs Update NodeCommsTimeout::new() call sites
agents/.../snap_rebuild.rs Update NodeCommsTimeout::new() call site

Backward Compatibility

  • Default behavior is preserved when --pool-import-timeout is not set
  • The only side effect is nvme_reconnect (62s) is no longer capped to 59s, which was always a bug
  • --no-min-timeouts continues to bypass all operation-specific timeouts

Test plan

  • Unit tests for timeout_grpc() covering cap behavior (5 tests)
  • cargo check -p agents --bin core passes
  • Manual: set --pool-import-timeout=300s, verify import uses 300s

João Gomes and others added 2 commits April 15, 2026 10:06
The timeout_grpc() function capped all operation timeouts at 59s via
.min(Duration::from_secs(59)). This silently clamped operations whose
minimum timeout intentionally exceeded that cap:

- nvme_reconnect: configured at 62s, was capped to 59s
- ImportPool: computed as pool(20s) * 3 = 60s, was capped to 59s

The cap now only applies when the operation's minimum timeout is <= 59s.
Operations that explicitly declare a higher minimum are no longer clamped.

Adds unit tests for timeout_grpc() covering the cap behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When an io-engine pod restarts, agent-core re-imports pools via the
ImportPool gRPC call. On large pools (e.g. 196 TiB with many lvols),
the SPDK blobstore metadata scan can exceed the default 60s timeout,
causing an infinite timeout-retry loop that prevents pool recovery.

Add a --pool-import-timeout CLI arg (also configurable via
POOL_IMPORT_TIMEOUT env var) that sets an explicit timeout for pool
import operations. When not specified, the existing default behavior
(pool * 3 = 60s) is preserved.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@azgms
azgms requested a review from a team as a code owner April 15, 2026 09:10
@tiagolobocastro

tiagolobocastro commented Apr 15, 2026

Copy link
Copy Markdown
Member

Hey, thanks for the change.
For "historical reasons" we still have a connection limiter to the nodes, so taking a long time will lock up the entire node. So increasing the timeout like this may not be a good idea and might also lead to some issues.

We already have a fix for the long create call here which defaults to 15 minutes. So we allow timeout but then async we check if the pool is created.

Perhaps we need a similar one to the import as well?
Other approaches may be using this but removing the limiter I mentioned or making pool imports a streaming rpc call / or use keep alives.

What do you think?

CC @Abhinandan-Purkait

@Abhinandan-Purkait

Copy link
Copy Markdown
Member

Hey, thanks for the change. For "historical reasons" we still have a connection limiter to the nodes, so taking a long time will lock up the entire node. So increasing the timeout like this may not be a good idea and might also lead to some issues.

We already have a fix for the long create call here which defaults to 15 minutes. So we allow timeout but then async we check if the pool is created.

Perhaps we need a similar one to the import as well? Other approaches may be using this but removing the limiter I mentioned or making pool imports a streaming rpc call / or use keep alives.

What do you think?

CC @Abhinandan-Purkait

@tiagolobocastro as we were discussing, how about a new state + refresh?

@abhilashshetty04

Copy link
Copy Markdown
Member

@azgms , I agree with mimicking behaviour of create_pool as @tiagolobocastro suggested. We can let gRPC timeout and reconciler on subsequent attempts can check if Pool is present without issuing import request again. Possibly set some marker when import gRPC times out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants