Skip performOnPrimary step when executing PublishCheckpoint. #6366

mch2 · 2023-02-18T02:01:57Z

Description

This change fixes broken SegmentReplicationRelocationITs and unmutes them. The cause of the flaky tests is WAIT_UNTIL requests blocking relocation of primary shards because replicas have not refreshed. The issue is that primary shards cannot publish their checkpoints because PublishCheckpointAction is a ReplicationOperation, that first hits the primary and acquires a permit. Given we are blocking operations, the permit is delayed and the request never succeeds. This fixes the issue by ensuring PublishCheckpoint requests are not sent to the primary, and only to replicas within the replication group directly.

Issues Resolved

closes #6065

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2023-02-18T02:25:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11333/
CommitID: 476aabd
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: Marc Handalian <handalm@amazon.com> Fix spotless. Signed-off-by: Marc Handalian <handalm@amazon.com>

github-actions · 2023-02-18T06:25:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermarkWithAutoReleaseEnabled

URL: https://build.ci.opensearch.org/job/gradle-check/11336/
CommitID: 33edf00
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2023-02-18T06:27:02Z

Codecov Report

Merging #6366 (e360577) into main (36f5cfe) will decrease coverage by 0.08%.
The diff coverage is 5.88%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6366      +/-   ##
============================================
- Coverage     70.76%   70.68%   -0.08%     
+ Complexity    59051    58953      -98     
============================================
  Files          4799     4799              
  Lines        282432   282438       +6     
  Branches      40716    40718       +2     
============================================
- Hits         199856   199638     -218     
- Misses        66147    66313     +166     
- Partials      16429    16487      +58

Impacted Files	Coverage Δ
...eplication/checkpoint/PublishCheckpointAction.java	`23.43% <5.88%> (-4.15%)`	⬇️
...g/opensearch/index/analysis/CharFilterFactory.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/opensearch/node/NodeClosedException.java	`50.00% <0.00%> (-50.00%)`	⬇️
...ava/org/opensearch/action/NoSuchNodeException.java	`0.00% <0.00%> (-50.00%)`	⬇️
...a/org/opensearch/tasks/TaskCancelledException.java	`50.00% <0.00%> (-50.00%)`	⬇️
...cluster/coordination/PendingClusterStateStats.java	`20.00% <0.00%> (-48.00%)`	⬇️
.../java/org/opensearch/search/dfs/AggregatedDfs.java	`51.61% <0.00%> (-45.17%)`	⬇️
...ava/org/opensearch/search/dfs/DfsSearchResult.java	`47.87% <0.00%> (-39.37%)`	⬇️
...pensearch/action/ingest/DeletePipelineRequest.java	`31.25% <0.00%> (-37.50%)`	⬇️
...r/src/main/java/org/opensearch/http/HttpUtils.java	`33.33% <0.00%> (-33.34%)`	⬇️
... and 475 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dreamer-89 · 2023-02-20T19:43:12Z

...ernalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationRelocationIT.java

-                .put("index.number_of_replicas", replicaCount)
-                .put("index.refresh_interval", -1)
-        ).get();
+        prepareCreate(INDEX_NAME, Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, replicaCount)).get();


dreamer-89 · 2023-02-20T19:46:18Z

...ernalClusterTest/java/org/opensearch/indices/replication/SegmentReplicationRelocationIT.java

    }

    /**
     * This test verifies happy path when primary shard is relocated newly added node (target) in the cluster. Before
     * relocation and after relocation documents are indexed and documents are verified
     */
-    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/5669")


nit: For completenes perspective, do we need relocation tests for IMMEDIATE & default refresh policy ?

Yes good idea, I've added randomness to select a refresh policy in all of the tests in this class other than the explicit wait_until test.

dreamer-89 · 2023-02-20T19:56:13Z

server/src/main/java/org/opensearch/indices/replication/checkpoint/PublishCheckpointAction.java

@@ -146,12 +146,11 @@ public void handleResponse(ReplicationResponse response) {
                                timer.time()
                            )
                        );
-                        task.setPhase("finished");


Why this setPhase call is removed ? If it is not useful, let's remove same during failure as well (on line 156)

Not intentional, thanks for catching this, it is useful in tracking state in the ReplicationTask.

- Add random refresh policy to reloation ITs. - add back finishing ReplicationTask. Signed-off-by: Marc Handalian <handalm@amazon.com>

dreamer-89

LGTM! Thanks @mch2 for fixing the bug and updating tests here.

github-actions · 2023-02-20T23:23:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationStatsIT.testSegmentReplicationStatsResponse

URL: https://build.ci.opensearch.org/job/gradle-check/11445/
CommitID: e360577
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

opensearch-trigger-bot · 2023-02-20T23:45:21Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6366-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 72314f75807f5eb72d7b178180dda72d92997dfd
# Push it to GitHub
git push --set-upstream origin backport/backport-6366-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6366-to-2.x.

…rch-project#6366) * Skip performOnPrimary step when executing PublishCheckpoint. Signed-off-by: Marc Handalian <handalm@amazon.com> Fix spotless. Signed-off-by: Marc Handalian <handalm@amazon.com> * PR Feedback: - Add random refresh policy to reloation ITs. - add back finishing ReplicationTask. Signed-off-by: Marc Handalian <handalm@amazon.com> --------- Signed-off-by: Marc Handalian <handalm@amazon.com> (cherry picked from commit 72314f7)

…6397)

…rch-project#6366) * Skip performOnPrimary step when executing PublishCheckpoint. Signed-off-by: Marc Handalian <handalm@amazon.com> Fix spotless. Signed-off-by: Marc Handalian <handalm@amazon.com> * PR Feedback: - Add random refresh policy to reloation ITs. - add back finishing ReplicationTask. Signed-off-by: Marc Handalian <handalm@amazon.com> --------- Signed-off-by: Marc Handalian <handalm@amazon.com>

…6366 Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

…estRelocateWhileContinuouslyIndexingAndWaitingForRefresh (#6637) * Trigger Refresh on NRT Engine. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Revert changes made to PublishCheckpointAction in #6366 Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Force flush on new elected primary after relocation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Remove unnecessary assertions Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Adding tests. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Address comments Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix indentation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> --------- Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

…estRelocateWhileContinuouslyIndexingAndWaitingForRefresh (#6637) * Trigger Refresh on NRT Engine. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Revert changes made to PublishCheckpointAction in #6366 Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Force flush on new elected primary after relocation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Remove unnecessary assertions Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Adding tests. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Address comments Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix indentation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> --------- Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> (cherry picked from commit 1e5d913) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…estRelocateWhileContinuouslyIndexingAndWaitingForRefresh (#6637) (#6675) * Trigger Refresh on NRT Engine. * Revert changes made to PublishCheckpointAction in #6366 * Fix failing unit test * Force flush on new elected primary after relocation. * Fix failing unit test. * Remove unnecessary assertions * Adding tests. * Address comments * Fix indentation. --------- (cherry picked from commit 1e5d913) Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…estRelocateWhileContinuouslyIndexingAndWaitingForRefresh (opensearch-project#6637) * Trigger Refresh on NRT Engine. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Revert changes made to PublishCheckpointAction in opensearch-project#6366 Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Force flush on new elected primary after relocation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix failing unit test. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Remove unnecessary assertions Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Adding tests. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Address comments Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> * Fix indentation. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> --------- Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> Signed-off-by: Mingshi Liu <mingshl@amazon.com>

Skip performOnPrimary step when executing PublishCheckpoint.

33edf00

Signed-off-by: Marc Handalian <handalm@amazon.com> Fix spotless. Signed-off-by: Marc Handalian <handalm@amazon.com>

mch2 force-pushed the publishCheckpoint branch from 94377bf to 33edf00 Compare February 18, 2023 05:56

mch2 added the skip-changelog label Feb 18, 2023

mch2 marked this pull request as ready for review February 18, 2023 19:40

mch2 requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners February 18, 2023 19:40

mch2 mentioned this pull request Feb 19, 2023

[BUG] failing IT test : SegmentReplicationRelocationIT #6065

Closed

dreamer-89 reviewed Feb 20, 2023

View reviewed changes

PR Feedback:

e360577

- Add random refresh policy to reloation ITs. - add back finishing ReplicationTask. Signed-off-by: Marc Handalian <handalm@amazon.com>

dreamer-89 approved these changes Feb 20, 2023

View reviewed changes

mch2 merged commit 72314f7 into opensearch-project:main Feb 20, 2023

mch2 deleted the publishCheckpoint branch February 20, 2023 23:44

mch2 added the backport 2.x Backport to 2.x branch label Feb 20, 2023

dreamer-89 mentioned this pull request Feb 21, 2023

[Segment Replication] Fix flaky tests testSegmentReplicationStatsResponse() and testSegmentReplicationStatsWithTimeout() #6268

Merged

6 tasks

mch2 mentioned this pull request Feb 21, 2023

[Backport 2.x] Skip performOnPrimary step when executing PublishCheckpoint. (#6366) #6397

Merged

6 tasks

mch2 added a commit that referenced this pull request Feb 23, 2023

Skip performOnPrimary step when executing PublishCheckpoint. (#6366) (#…

4bb8d24

…6397)

dreamer-89 mentioned this pull request Mar 7, 2023

[BUG][Segment Replication] Stuck Initializing Shard w/ Segment Replication enabled #6084

Closed

mch2 mentioned this pull request Mar 11, 2023

[Segment Replication] Fix flaky test testRelocateWhileContinuouslyIndexingAndWaitingForRefresh #6619

Closed

6 tasks

Rishikesh1159 added a commit to Rishikesh1159/OpenSearch that referenced this pull request Mar 11, 2023

Revert changes made to PublishCheckpointAction in opensearch-project#…

5407a68

…6366 Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>

Rishikesh1159 mentioned this pull request Mar 11, 2023

[Segment Replication] Fix Flaky in Test SegmentReplicationRelocationIT #6637

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip performOnPrimary step when executing PublishCheckpoint. #6366

Skip performOnPrimary step when executing PublishCheckpoint. #6366

mch2 commented Feb 18, 2023

github-actions bot commented Feb 18, 2023

github-actions bot commented Feb 18, 2023

codecov-commenter commented Feb 18, 2023 •

edited

Loading

dreamer-89 Feb 20, 2023

dreamer-89 Feb 20, 2023 •

edited

Loading

mch2 Feb 20, 2023

dreamer-89 Feb 20, 2023

mch2 Feb 20, 2023

dreamer-89 left a comment

github-actions bot commented Feb 20, 2023

opensearch-trigger-bot bot commented Feb 20, 2023

Skip performOnPrimary step when executing PublishCheckpoint. #6366

Skip performOnPrimary step when executing PublishCheckpoint. #6366

Conversation

mch2 commented Feb 18, 2023

Description

Issues Resolved

Check List

github-actions bot commented Feb 18, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 18, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Feb 18, 2023 • edited Loading

Codecov Report

dreamer-89 Feb 20, 2023

Choose a reason for hiding this comment

dreamer-89 Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

mch2 Feb 20, 2023

Choose a reason for hiding this comment

dreamer-89 Feb 20, 2023

Choose a reason for hiding this comment

mch2 Feb 20, 2023

Choose a reason for hiding this comment

dreamer-89 left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Feb 20, 2023

codecov-commenter commented Feb 18, 2023 •

edited

Loading

dreamer-89 Feb 20, 2023 •

edited

Loading