[Bugfix] Fix NPE in ReplicaShardAllocator (#13993)

[DaniilRoman]

Description

The PR fixes the NPE in ReplicaShardAllocator when we can't get an active primary shard.

Related Issues

Resolves #13993

Check List

• Functionality includes testing.

Flaky test

• [BUG] org.opensearch.indices.IndicesRequestCacheIT.testCacheCleanupOnEqualStalenessAndThreshold is flaky #13503

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]

❌ Gradle check result for b18d572: 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]

❌ Gradle check result for e2fbc24: 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]

❌ Gradle check result for 4063a06: 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]

❕ Gradle check result for d0cd50c: UNSTABLE

• TEST FAILURES:

      1 org.opensearch.index.ShardIndexingPressureSettingsIT.testShardIndexingPressureEnforcedEnabledDisabledSetting

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.93%. Comparing base (4abcf39) to head (b4e9a85).
Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14385      +/-   ##
============================================
+ Coverage     71.91%   71.93%   +0.02%     
+ Complexity    62516    62493      -23     
============================================
  Files          5145     5145              
  Lines        293307   293309       +2     
  Branches      42407    42407              
============================================
+ Hits         210919   211003      +84     
+ Misses        65040    64953      -87     
- Partials      17348    17353       +5     

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

[github-actions]

❌ Gradle check result for f96ce27: 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]

✅ Gradle check result for ff59718: SUCCESS

[DaniilRoman]

Hi team 👋
Is there anything I need to improve to get an approval? 🙏

[dblock]

This looks good, but I am not super familiar with this part of the code, maybe @andrross or @mch2 can double-check this?

@DaniilRoman This fixes a bug, so add to CHANGELOG.

[DaniilRoman]

@dblock thank you for looking at 🙏 I added it to CHANGELOG and I see I need to add a label backport 2.x as well, but I can't do it by my own?

[github-actions]

❌ Gradle check result for 47d96c2: 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]

✅ Gradle check result for b4e9a85: SUCCESS

[dblock]

Looks innocent enough to merge.

[dblock]

Thanks for fixing this @DaniilRoman, backport is running.

[Bukhtawar]

@DaniilRoman do we know why this assertion had to be removed. @dblock In general I am not of fan of removing assertions, they exist in the system for a reason, we shouldn't let our guards down.

[dbwiddis]

@Bukhtawar the assertion has been replaced by an explicit null check, returning null instead of throwing an exception. Either way the code will not proceed past this point. There is a previous return null at line 98, so it seems null is already an expected return value.

[Bukhtawar]

@dbwiddis If I am understanding this correct there are two things

1. the method cancelExistingRecoveryForBetterMatch is expected to return a Runnable to denote some work to cancel existing recovery. A null as a method return type denotes no work is supposed to be done for cancelling.
2. the invariant that says for a given shard(replica) it's corresponding primary should be non-null, since the code path is supposed to kick in for an initialising replica copy which can only be initialising once it's corresponding primary has been assigned see the decider

So there are two different things that exist and equating them IMO is incorrect. Removing the assertion essentially says at this point of execution a primary shard can be null when the system was designed ensuring that it shouldn't?

[dbwiddis]

So there are two different things that exist and equating them IMO is incorrect. Removing the assertion essentially says at this point of execution a primary shard can be null when the system was designed ensuring that it shouldn't?

I haven't dug into the logic, but from the face of it, the previous situation is that the assertion causes an exception, and the exception isn't handled.

I've no objection to reverting this change if you think the exception should be caught and addressed elsewhere.

[Bukhtawar]

I haven't dug into the logic, but from the face of it, the previous situation is that the assertion causes an exception, and the exception isn't handled.

Unfortunately assertions don't run outside tests and the stack trace isn't an assertion tripping. Even if an assertion is failing the right thing to do is investigate why it is failing rather than burying the problem, by removing the assertion(invariant) that was added

[dbwiddis]

#10254 didn't "fix" or modify the behavior of this method, I had just refactored ...

So I see looking again... sorry. The first time I was just looking for a line number. But now I'm really confused. In order for an NPE on line 108 on null primaryShard it would have had to pass the assertions on 106 and 107.

OpenSearch/server/src/main/java/org/opensearch/gateway/ReplicaShardAllocator.java

Lines 106 to 108 in 12115d1

	assert primaryShard != null : "the replica shard can be allocated on at least one node, so there must be an active primary";
	assert primaryShard.currentNodeId() != null;
	final DiscoveryNode primaryNode = allocation.nodes().get(primaryShard.currentNodeId());

Is this just undefined/unpredictable behavior during JVM shutdown (node drop)?

In any case, seems a revert of this PR is the right action at this point.

[Bukhtawar]

@Bukhtawar that's not the case with the assert keyword as it's used here. It throws an AssertionError.
As an Error it's not normally caught by exception catch blocks unless you're specifically looking for it, and by the traditional interpretation of Error means that it's an unrecoverable situation and the entire JVM should be shut down.

Sorry I don't think this is the correct understanding of how assert works. AssertionErrors are only configured for tests using a -ea JVM flag and in this case I don't think an AssertionError was triggered. Please feel free to read up more on this here

In any case, seems a revert of this PR is the right action at this point

Thanks for your understanding.

[dbwiddis]

Got it, and thanks for educating me.

So wouldn't an appropriate fix for the NPE then to both put the assert back and null check to avoid the exception? That seems odd but I've never really used assertions like this so at this point I really don't know the best approach.

[andrross]

So wouldn't an appropriate fix for the NPE then to both put the assert back and null check to avoid the exception?

@dbwiddis The appropriate fix is to figure out why the invariant is being violated and fix that bug. i.e. if no primary exists, then there can be no recovery and this method should not be called. The previous behavior of having the code fail with an NPE is preferable to ignoring a situation that is not intended to be possible.

Slightly off topic, but in general I really dislike using the assert keyword. Instead of assert primaryShard != null : "message" I almost always prefer something like Objects.requireNonNull(primaryShard, "message") so that it deterministically fails in all situations. Only if the code is super performance critical and if the assertion check is computationally intensive do I think the assert keyword makes sense.

[dbwiddis]

The appropriate fix is to figure out why the invariant is being violated and fix that bug.

@andrross In general I agree.

In reality, I read the issue reporting this bug and it indicates it's happening during a node drop. Which means the JVM is shutting down for <insert reason here>. Which means undefined/unpredictable behavior.

TLDR: I'm not sure there is a bug, and the symptom may be a result of a JVM being shut down causing a node drop as a result of some totally unrelated thing.

There's not enough information in the issue to infer more.