Skip to content

Exempt server-injected preferences from strict weighted routing check… - #22571

Open
piyushk6130 wants to merge 1 commit into
opensearch-project:mainfrom
piyushk6130:fix/skip-preference-check-for-server-injected-preferences
Open

Exempt server-injected preferences from strict weighted routing check…#22571
piyushk6130 wants to merge 1 commit into
opensearch-project:mainfrom
piyushk6130:fix/skip-preference-check-for-server-injected-preferences

Conversation

@piyushk6130

@piyushk6130 piyushk6130 commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

OperationRouting#searchShards injects a search preference server-side for certain index types when the
caller supplies none: _primary for remote snapshot indices and _primary_first for writable warm
indices (behind WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG). This causes two problems:

Problem 1 — injected preference leaks across indices. The injected preference is assigned to the
caller-supplied preference variable inside the per-shard loop, so in a multi-index search it leaks into
every index processed afterwards. A search spanning a remote snapshot (or warm) index and regular indices
silently routes the regular indices with _primary/_primary_first as well, defeating replica
load-balancing and zone-aware routing for those indices. Since computeTargetedShards returns a
HashSet, which index "wins" is iteration-order dependent, making routing non-deterministic.

Problem 2 — the server rejects its own routing decision. checkPreferenceBasedRoutingAllowed cannot
distinguish the engine's own injected preference from a caller-supplied one. When a restricted preference
is injected while strict weighted shard routing is active (WeightedRoutingMetadata present and
cluster.routing.weighted.strict=true), the server rejects its own routing decision with
PreferenceBasedSearchNotAllowedException (HTTP 403) and the search fails even though the caller never
set a preference. Distributions that restrict PRIMARY_FIRST hit this on every warm-index search on
weighted-routing clusters.

Solution

  • Compute an effective preference per shard instead of mutating the request-level preference, so
    server-injected preferences are scoped to the index they were injected for (fixes Problem 1).
  • Track whether the preference was server-injected and skip the strict weighted routing restriction check
    for server-injected preferences (fixes Problem 2). The check exists to stop callers from bypassing
    weighted shard routing; it should not apply to the engine's own routing decisions. All three server
    injections (_primary for remote snapshot, _primary_first for writable warm, _search_replica for
    search-only replicas) are treated uniformly.
  • Caller-supplied preferences are unaffected: they keep serverInjectedPreference = false and hit the
    restriction check exactly as before.

Testing

  • testServerInjectedPreferenceDoesNotLeakAcrossIndices — reproduces Problem 1; fails on main without
    this fix (AssertionError: All shard copies should be returned for a regular index), passes with it.
  • testStrictWeightedRoutingExemptsOnlyServerInjectedPreferences — pins both sides of the exemption
    directly: a caller-supplied restricted preference still throws
    PreferenceBasedSearchNotAllowedException; the same preference passes when server-injected.
  • testWarmIndexSearchNotBlockedByStrictWeightedRouting — end-to-end: warm index + zero-weight zone +
    cluster.routing.weighted.strict=true + no caller preference succeeds.
  • All existing OperationRoutingTests and SearchWeightedRoutingIT pass; :server:precommit passes.

Note: the earlier gradle-check failure was Netty4HttpRequestSizeLimitIT.testLimitsInFlightRequests, a
known flaky test (#18875) unrelated to this change.

Related Issues

Flaky test seen in CI on this PR: #18875 (unrelated to this change).

Check List

  • Functionality includes testing.
  • API changes companion pull request
    created, if applicable.
  • Public documentation issue/PR
    created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0
license.
For more information on following Developer Certificate of Origin and signing off your commits, please
check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-o
f-origin).

@piyushk6130
piyushk6130 requested a review from a team as a code owner July 27, 2026 05:14
@github-actions

github-actions Bot commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 2f4bc39)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to bf5b882
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Document weighted routing bypass semantics

The serverInjectedPreference flag is only set when the server injects a preference
because the caller supplied none. If the caller explicitly supplies e.g. _primary
for a remote snapshot index, effectivePreference remains the caller value but
serverInjectedPreference stays false, which correctly enforces the strict weighted
routing check. However, ensure this exemption logic isn't unintentionally bypassing
weighted routing checks when the injected preference type (like PRIMARY_FIRST) is in
WEIGHTED_ROUTING_RESTRICTED_PREFERENCES — consider logging or documenting that
server-injected routing intentionally overrides weighted routing constraints for
these index types.

server/src/main/java/org/opensearch/cluster/routing/OperationRouting.java [274-291]

+// Server-injected preferences deliberately bypass strict weighted routing because these index
+// types (remote snapshot, writable warm, search-only replica) have engine-mandated routing.
 if (indexMetadataForShard.isRemoteSnapshot() && (effectivePreference == null || effectivePreference.isEmpty())) {
     effectivePreference = Preference.PRIMARY.type();
     serverInjectedPreference = true;
 }
 
-if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)
-    && indexMetadataForShard.getSettings().getAsBoolean(IndexModule.IS_WARM_INDEX_SETTING.getKey(), false)
-    && (effectivePreference == null || effectivePreference.isEmpty())) {
-    effectivePreference = Preference.PRIMARY_FIRST.type();
-    serverInjectedPreference = true;
-}
-
-if (effectivePreference == null || effectivePreference.isEmpty()) {
-    if (indexMetadataForShard.getNumberOfSearchOnlyReplicas() > 0 && isStrictSearchOnlyShardRouting) {
-        effectivePreference = Preference.SEARCH_REPLICA.type();
-        serverInjectedPreference = true;
-    }
-}
-
Suggestion importance[1-10]: 2

__

Why: The suggestion mainly asks to add a comment documenting the bypass semantics, which is a minor documentation improvement. The existing code already has a comment explaining the scoping, and the checkPreferenceBasedRoutingAllowed method also has a clarifying comment.

Low

Previous suggestions

Suggestions up to commit 301879c
CategorySuggestion                                                                                                                                    Impact
General
Scope exemption to intended injected preferences

The serverInjectedPreference flag currently exempts server-injected preferences from
the strict weighted routing check entirely. However, for SEARCH_REPLICA injected via
isStrictSearchOnlyShardRouting, this may unintentionally bypass weighted routing
constraints that should still apply. Consider whether each injected preference type
should be exempt, or scope the exemption specifically to remote snapshot / warm
index cases.

server/src/main/java/org/opensearch/cluster/routing/OperationRouting.java [279-291]

 if (FeatureFlags.isEnabled(FeatureFlags.WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG)
     && indexMetadataForShard.getSettings().getAsBoolean(IndexModule.IS_WARM_INDEX_SETTING.getKey(), false)
     && (effectivePreference == null || effectivePreference.isEmpty())) {
     effectivePreference = Preference.PRIMARY_FIRST.type();
     serverInjectedPreference = true;
 }
 
 if (effectivePreference == null || effectivePreference.isEmpty()) {
     if (indexMetadataForShard.getNumberOfSearchOnlyReplicas() > 0 && isStrictSearchOnlyShardRouting) {
         effectivePreference = Preference.SEARCH_REPLICA.type();
-        serverInjectedPreference = true;
+        // Intentionally not marking as serverInjectedPreference; strict weighted routing should still apply
     }
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion raises a valid design consideration about whether SEARCH_REPLICA injection should also bypass strict weighted routing, but it's a subjective policy question without clear evidence the current behavior is wrong. Moderate impact.

Low

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 301879c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@piyushk6130
piyushk6130 force-pushed the fix/skip-preference-check-for-server-injected-preferences branch from 301879c to f165103 Compare July 29, 2026 08:34
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f165103

@piyushk6130
piyushk6130 force-pushed the fix/skip-preference-check-for-server-injected-preferences branch from f165103 to bf5b882 Compare July 29, 2026 08:49
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit bf5b882

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for bf5b882: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Piyush Kumar
Exempt server-injected preferences from strict weighted routing check…
2f4bc39 
… and scope them per index

OperationRouting#searchShards injects a search preference server-side for
certain index types when the caller supplies none: _primary for remote
snapshot indices and _primary_first for writable warm indices (behind
WRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG). This causes two problems:

1. The injected preference is assigned to the caller-supplied preference
   variable inside the per-shard loop, so in a multi-index search it leaks
   into every index processed afterwards. A search spanning a remote
   snapshot (or warm) index and regular indices silently routes the regular
   indices with _primary/_primary_first as well, defeating replica
   load-balancing and zone-aware routing for those indices.

2. checkPreferenceBasedRoutingAllowed cannot distinguish the engine's own
   injected preference from a caller-supplied one. When a restricted
   preference is injected while strict weighted shard routing is active
   (WeightedRoutingMetadata present and cluster.routing.weighted.strict),
   the server rejects its own routing decision with
   PreferenceBasedSearchNotAllowedException (HTTP 403) and the search
   fails even though the caller never set a preference. Distributions
   that restrict PRIMARY_FIRST hit this on every warm-index search on
   weighted-routing clusters.

Fix: compute an effective preference per shard instead of mutating the
request-level preference, track whether it was server-injected, and skip
the strict weighted routing restriction check for server-injected
preferences. The check exists to stop callers from bypassing weighted
shard routing; it should not apply to the engine's own routing decisions.

Signed-off-by: Piyush Kumar <piykumab@amazon.com>
@piyushk6130
piyushk6130 force-pushed the fix/skip-preference-check-for-server-injected-preferences branch from bf5b882 to 2f4bc39 Compare July 29, 2026 09:22
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2f4bc39

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 2f4bc39: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@piyushk6130