GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [pipeline-manager] Retry opening the connection to a pipeline for up to 35 s by gz · Pull Request #7868 · feldera/feldera Conversation
|
|
There is a case to be made that we should retry only at the edge to avoid retry amplification -- which is a principle this PR violates. On the other hand the api-server knows best if it makes sense to retry because it can tell if the pod is supposed to be up. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
2ba88d6 to
29b97f3
Compare
…to 35 s Right after a pipeline (re)start the api-server can fail to connect to it: cluster DNS keeps answering with the previous pod's IP for the record's TTL (30 s with kube-dns and NodeLocal DNSCache on GKE), a cached negative answer can outlive the new Service, and a pod can refuse connections for a moment before its server listens. Until now the proxy made one attempt and answered 503 PipelineInteractionConnectFailed, leaving every client to retry. A connect failure means the request was never sent, so repeating it cannot apply it twice. Both proxy paths now retry such failures every 500 ms for up to 35 s, which outlasts the DNS window with an attempt to spare. A streaming request body is shared across attempts and is only replayed while no attempt has read from it. When the proxy gives up, the error says how many attempts it made; when a retry rescues a request, the proxy logs it. Tests: a server that starts listening 1.5 s after the first attempt receives the whole POST body, and a port nothing listens on yields PipelineInteractionConnectFailed only after the budget. Both fail with the retry disabled. Signed-off-by: Gerd Zellweger <[email protected]>
29b97f3 to
180f7b3
Compare
|
The manager-side retry is the right layer: only the manager knows that the request never reached the pipeline, and it can stop when the pipeline stops or moves. But it now stacks with the client retries, because both clients also retry
Neither client sets a read timeout by default, so attempts do not overlap. But a user who sets The clients can't just stop retrying this code, because v0.378.0 already sends it after a single attempt (#7860). My suggestion:
References: The times in the table are calculated from the code, not measured. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Code review of the retry change, as a follow-up to the earlier comment about client retry stacking. I checked these comments against the code at 180f7b3 but did not compile it or run tests. Two comments marked "Not verified" depend on awc behavior or lock timing that I did not confirm.
The most important ones are the 70 s worst case after a cache hit, the 35 s budget for best-effort callers with short timeouts, and permanent connect errors (TLS) that are retried as transient.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| timeout, | ||
| body_size_limit, | ||
| accept_encoding, | ||
| ConnectRetry::DEFAULT, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
After a cache hit, an error removes the cache entry and calls this function again (lines 660-675), and that call also uses ConnectRetry::DEFAULT. When a cached Running pipeline refuses connections, the client therefore waits about 70 s for the 503, not the 35 s the description gives. A load balancer or client with a 60 s timeout cuts the request first. Possible fix: skip the cache-evict retry when the error is a connect failure that already used the budget, or give the second call ConnectRetry::NONE.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| timeout, | ||
| body_size_limit, | ||
| accept_encoding, | ||
| ConnectRetry::DEFAULT, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
All _by_name callers now get the 35 s budget, also callers that pass a short timeout because the call is best effort:
| Caller | Timeout | Effect |
|---|---|---|
fetch_connector_error_stats (pipeline_management.rs:931), run for each pipeline with join_all by the StatusWithConnectors list |
500 ms | One pipeline that refuses connections blocks the web console list for 35-70 s. Only on the fallback path for pipelines with no connector_stats in their status details. |
support bundle (support_data_collector.rs:142) |
per endpoint | Endpoints are fetched one at a time, so an unreachable pipeline can stall the bundle for minutes. |
suspend (pipeline_management.rs:2118) |
120 s | Probably fine. |
Suggestion: let the caller choose the ConnectRetry, or limit the budget by the caller's timeout.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// Whether `error` says the connection to the pipeline was never opened, so the request | ||
| /// was not sent. | ||
| fn is_connect_failure(error: &SendRequestError) -> bool { | ||
| matches!(error, SendRequestError::Connect(connect) if !matches!(connect, ConnectError::Disconnected)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This retries every ConnectError except Disconnected. Some variants are permanent, for example SslError, SslIsNotSupported and H2NotSupported. A TLS configuration error then looks like a 35 s (or 70 s) hang, and it ends in PipelineInteractionConnectFailed, which the SDK and fda retry again. Suggest an allow-list: Timeout, Resolver, NoRecords, Io (refused).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| Err(error) => error, | ||
| }; | ||
| if !is_connect_failure(&error) || !may_retry() || started.elapsed() >= retry.budget { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Not verified: the awc request timeout also covers the connect phase. If the request timeout is shorter than the connector's 5 s timeout, a connect to a dead (stale DNS) address fails as SendRequestError::Timeout, not Connect(Timeout), so it is not retried. Then the stale-DNS case is retried only for callers with a timeout longer than 5 s. A test with a non-routable address and a short timeout would show which way it goes.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// leaves room for an attempt that starts after a 30 s entry expired. | ||
| pub(crate) const DEFAULT: Self = Self { | ||
| budget: Duration::from_secs(35), | ||
| pause: Duration::from_secs(1), |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The pause is 1 s, but the PR description says 500 ms. One of the two needs to change.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| tokio::time::sleep(retry.pause).await; | ||
| // The pipeline may have been stopped or restarted meanwhile: give up with its status | ||
| // error, or follow it to its new location. | ||
| if let Some(current) = recheck().await? |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
? replaces the connect failure with any recheck error. If the cache entry expired (5 s TTL) and the DB read fails for a short time, or the pipeline is briefly not running during an in-place restart, the client gets that error instead of PipelineInteractionConnectFailed. Clients may not retry the replacement error. Consider returning the status error only when the recheck clearly says the pipeline is stopped or unavailable, and otherwise continuing the loop.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| tokio::time::sleep(retry.pause).await; | ||
| // The pipeline may have been stopped or restarted meanwhile: give up with its status | ||
| // error, or follow it to its new location. | ||
| if let Some(current) = recheck().await? |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Not verified: on a cache miss, check_pipeline takes the global db.lock(). Every pipeline change clears the cache, so many concurrent retry loops (the list endpoint with several unreachable pipelines, or many ingress clients during a restart) all queue on that one lock about once per second. This can slow unrelated API requests.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| type Item = Result<Bytes, PayloadError>; | ||
|
|
||
| fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { | ||
| self.was_read.set(true); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
was_read becomes true on every poll, also when the inner payload returns Pending and no bytes were taken. Then one poll that consumed nothing stops all further retries. Setting it only on Poll::Ready(Some(_)) keeps the safety guarantee without that false positive. The practical effect depends on whether awc polls the body before a connect error is reported.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .pipeline_url(&location, endpoint, query_string); | ||
| let timeout = timeout.unwrap_or(Self::PIPELINE_HTTP_REQUEST_TIMEOUT); | ||
| streaming_proxy(client, &url, pipeline_name, &request, body, timeout).await | ||
| let recheck = || async move { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nit: this closure is a copy of the one at line 623, and the two build_request closures repeat the same client.request().timeout().force_close().no_decompress() chain. A helper such as fn recheck_url(&self, tenant_id, name, endpoint, query_string) keeps the two proxy paths from drifting apart.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right approach, after looking at it at a high level. I think that there are refinements in the code that can be done, and I think that we should consider looking at retries more systematically (see the linked issues that I filed) but this is a good start.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.