Skip to content

Rollover: scope checkBlock to write index only, skipping non-write alias members - #21838

Merged
andrross merged 4 commits into
opensearch-project:mainfrom
mishail:ms/21629-rollover-fix-oss
Jul 1, 2026
Merged

Rollover: scope checkBlock to write index only, skipping non-write alias members#21838
andrross merged 4 commits into
opensearch-project:mainfrom
mishail:ms/21629-rollover-fix-oss

Conversation

@mishail

@mishail mishail commented May 26, 2026

Copy link
Copy Markdown
Contributor

Disclaimer: parts of this PR were generated by LLM

Description

The rollover precheck in TransportRolloverAction.checkBlock resolved the rollover target to every alias member and aborted on any METADATA_WRITE block on any of them. That means a non-write alias member carrying such a block — for example a cross-cluster-replication follower's index-replication block — kills otherwise-valid rollovers against a sibling write index. The same shape applies to data-stream rollovers when an older backing carries a metadata-write block.

This PR scopes the precheck to the single write index via IndexAbstraction.getWriteIndex() — the same path MetadataRolloverService uses to perform the rollover — so the precheck and the actual rollover agree on which index matters. The same code path handles alias and data-stream targets and works regardless of which plugin placed the block.

The block check itself still goes through ClusterBlocks.indicesWithRemoteSnapshotBlockedException(added by #16483) — only the input changes from "all alias members" to "the single write index". The helper's existing carve-out for remote_snapshot indices (skip the abort unless the user has set index.blocks.read_only, index.blocks.metadata, or index.blocks.read_only_allow_delete) therefore continues to apply, so searchable-snapshot indices keep working.

ClusterBlocks.indicesWithRemoteSnapshotBlockedException itself is unchanged; TransportIndicesAliasesAction still uses it for alias-modification blocking and is unaffected.

Related Issues

Resolves #21629

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.

Signed-off-by: Mikhail Stepura <mstepura@apple.com>
@mishail
mishail requested a review from a team as a code owner May 26, 2026 20:23
@github-actions github-actions Bot added bug Something isn't working labels May 26, 2026
@github-actions

github-actions Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 83fed8f)

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

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null-safety for metadata access

Add null-safety check for state.metadata() before accessing getIndicesLookup(). If
the cluster state metadata is null, this will throw a NullPointerException. Consider
adding a guard clause to return null early if metadata is unavailable.

server/src/main/java/org/opensearch/action/admin/indices/rollover/TransportRolloverAction.java [130-137]

+if (state.metadata() == null) {
+    return null;
+}
 IndexAbstraction indexAbstraction = state.metadata().getIndicesLookup().get(request.getRolloverTarget());
 if (indexAbstraction == null) {
     return null;
 }
 IndexMetadata writeIndex = indexAbstraction.getWriteIndex();
 if (writeIndex == null) {
     return null;
 }
Suggestion importance[1-10]: 2

__

Why: While adding null-safety checks is generally good practice, ClusterState.metadata() is highly unlikely to return null in OpenSearch's architecture. The ClusterState object always contains metadata, and this would represent a fundamental system failure. The suggestion addresses a theoretical edge case that doesn't occur in practice, making it low-impact defensive programming rather than fixing an actual issue.

Low

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for bca10e0: 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?

Signed-off-by: Mikhail Stepura <mstepura@apple.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 78dfd3e

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 78dfd3e: SUCCESS

@codecov

codecov Bot commented May 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.48%. Comparing base (097bf13) to head (83fed8f).

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21838      +/-   ##
============================================
- Coverage     73.50%   73.48%   -0.03%     
+ Complexity    75572    75532      -40     
============================================
  Files          6034     6034              
  Lines        342604   342606       +2     
  Branches      49279    49281       +2     
============================================
- Hits         251826   251751      -75     
- Misses        70758    70862     +104     
+ Partials      20020    19993      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 27f18eb

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 27f18eb: 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?

@mishail mishail changed the title Scope checkBlock to write index only, skipping non-write alias members Rollover: scope checkBlock to write index only, skipping non-write alias members May 27, 2026
gingeekrishna added a commit to gingeekrishna/OpenSearch that referenced this pull request May 27, 2026
…changes)

Build 78356 and 78421 failed. Verified no remaining references to
deleted modules exist anywhere in the codebase. PR opensearch-project#21838 (unrelated)
shows the same failure/pass/failure pattern in builds 78353/78354/78380,
confirming this is systemic CI flakiness.

Signed-off-by: Radhakrishnan Pachyappan <gingeekrishna@gmail.com>
@mishail mishail closed this May 28, 2026
@mishail mishail reopened this May 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 27f18eb

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 27f18eb: 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?

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 83fed8f

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 83fed8f: SUCCESS

@mishail

mishail commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@andrross it's been a month already. Are there anything wrong with these changes?

@andrross
andrross merged commit 8d2b041 into opensearch-project:main Jul 1, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Rollover precheck overscans alias members; should check only the write index

2 participants