Skip to content

OSPP : feat(divide): preserve canary partitions during failover retries - #8088

Closed
SouthwestAsiaFloat wants to merge 2 commits into
apache:masterfrom
SouthwestAsiaFloat:ospp-canary-retry
Closed

SouthwestAsiaFloat wants to merge 2 commits into
apache:masterfrom
SouthwestAsiaFloat:ospp-canary-retry

Conversation

@SouthwestAsiaFloat

Copy link
Copy Markdown
Contributor

Enable failover retries for Canary routing while preserving the initially selected partition, including an initial fallback to Stable.

Reuse existing retry settings for default, fixed-delay, and exponential-backoff retries. Each retry selects a healthy, untried node within the same partition; return HTTP 503 when none remains.

Architecture:

flowchart TD
    A["HTTP Request<br/>retryStrategy = failover"] --> B["DividePlugin<br/>Choose Canary or Stable"]
    C["DivideRuleHandle<br/>CanaryConfig + Retry Settings"] -.-> B

    B --> D["Resolve Initial Pool<br/>Apply fallbackPolicy only here"]
    D --> E["Save Partition and Immutable<br/>Label Snapshot in exchange"]
    E --> F["Select First Upstream<br/>Circuit Breaker + Load Balancing"]
    F --> G["HTTP Client<br/>Send Request"]

    G --> H{"Successful?"}
    H -->|Yes| I["Continue Plugin Chain"]
    H -->|No| J{"Retryable Failure<br/>and Retries Remaining?"}

    J -->|No| K["Propagate Error"]
    J -->|Yes| L["Retry Schedule<br/>Default / Fixed / Exponential"]

    L --> M["DefaultRetryStrategy.resend<br/>Refresh Healthy Upstreams"]
    E -. "Reuse Saved Labels" .-> N
    M --> N["Keep Nodes in Selected Partition<br/>Exclude Attempted and Blocked Nodes"]

    N --> O{"Eligible Node Available?"}
    O -->|Yes| P["Load Balance Within Partition"]
    P --> G
    O -->|No| Q["HTTP 503<br/>No Cross-Partition Fallback"]
Loading
  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please coordinate with #8076 before either lands. That PR fixes the same defect — canary failover escaping into the other partition — by carrying the resolved labels in an exchange attribute and filtering the refreshed snapshot inside DefaultRetryStrategy.resend. This PR does the same thing with Constants.SHENYU_CANARY_PARTITION / SHENYU_CANARY_LABELS and additionally replaces the hand-rolled retry loop with Retry + FailoverExhaustedException. Both cannot merge; whoever loses should be closed rather than rebased, and the choice is a maintainer call, not a review verdict.

For what it is worth, this PR covers a case #8076 does not: ExponentialRetryBackoffStrategy previously called response.retryWhen(retrySpec) for failover, i.e. it re-subscribed the same request to the same URI, so retryStrategy=failover with exponential backoff never actually failed over. Routing it through executeFailover is a real fix. If the decision goes to the smaller diff, that hunk should be lifted into it separately.

On the CI: the six red build (shenyu-integrated-test-*) jobs are infrastructure, not this diff. I pulled the http job log: it loops on curl http://localhost:9200/_cat/health?v response 000 until the job is cancelled — the Elasticsearch container never starts. Same lanes are red on unrelated PRs today. A rebase/push will re-trigger them.

Technical notes

  1. Constants.SHENYU_CANARY_PARTITION is written by DividePlugin and only null-checked by resend — its value is never read in production code. The labels carry the actual payload, so this is a boolean disguised as a string. Either drop it, or put it into the FailoverExhaustedException message so a 503 says which partition was exhausted; right now the operator gets CANNOT_FIND_HEALTHY_UPSTREAM_URL_AFTER_FAILOVER with no partition context, which is the one piece of information that would explain the failure.
  2. Replacing the loop with Retry changes retry semantics for every failover user, not only canary ones. Retry.max(n)/fixedDelay(n, ...) give the same attempt count as the old for (i < retryTimes) loop, but that equivalence deserves an explicit test (attempt count with retry=0 and retry=3) rather than being assumed.
  3. The CanaryConfigValidator and RuleServiceTest changes are unrelated to failover retry. Splitting them into their own PR would make both easier to review — this one is already carrying a retry-operator rewrite.
  4. FailoverExhaustedException extending ShenyuException works, but it is control flow signalled through an exception hierarchy that executeFailover then maps back to a 503. A comment at the throw sites saying "this exists to stop the retry chain" would save the next reader a detour.

Happy to re-review once the duplication is resolved and whichever shape is chosen has the attempt-count test.

final String loadBalance = exchange.getAttribute(Constants.LOAD_BALANCE);
//always query the latest available list
final List<Upstream> availableUpstreamList = UpstreamCacheManager.getInstance().findUpstreamListBySelectorId(selectorId);
String partition = exchange.getAttribute(Constants.SHENYU_CANARY_PARTITION);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partition is only ever null-checked; its value is never used. The actual filter input is SHENYU_CANARY_LABELS. So this constant is a flag wearing a string, and the interesting information — which partition was being served — is thrown away.

It would be worth putting partition into the FailoverExhaustedException message, so the resulting 503 says cannot find healthy upstream in partition 'canary' instead of the generic message. That is the first thing an operator would want to know.

Aias00 pushed a commit that referenced this pull request Oct 30, 2026
…8231)

Fixes #8230. Completes the exponential routing correction discussed in #8088 with real-client and virtual-time regression coverage.

Co-authored-by: qiaolezi001 <329931181+qiaolezi001@users.noreply.github.com>
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.

2 participants