HDDS-6447. Refine SCM handling of unhealthy container replicas.

[errose28]

What changes were proposed in this pull request?

This PR continues Hanisha's work from #3258, although it makes some changes to the rules proposed there.

Updates how replication manager (RM) deals with quasi closed and unhealthy replicas for Ratis containers only. Currently all unhealthy containers are deleted. It is possible that the unhealthy container still has mostly good data, just with a few corrupted blocks, and that we will have the ability to recover unhealthy containers in the future. For this reason, this PR proposes changing the replication manager to abide by the following rules:

• If the container is closed:

  • If all replicas are unhealthy, they should be replicated like healthy containers.
    • This means the system should prioritize having 3 copies available, and delete extra copies if over replication occurs.
    • The unhealthy replicas to keep should be prioritized on highest BCSID.
  • If only some of the replicas are unhealthy:
    • In iteration 1, RM should replicate the healthy replicas and ignore the unhealthy replicas.
    • In iteration 2, RM should delete the unhealthy replicas.

• If the container is not yet closed:

  • Open containers remain excluded from RM.
  • If all replicas are unhealthy:
    • 3 replicas should be preserved. These will be chosen based on unique BCSID.
  • If there is a mix of unhealthy and quasi closed replicas:
    • In iteration 1, RM should replicate the quasi closed containers so that there are 3 replicas, ignoring the unhealthy replicas.
      • Since SCM currently has no way of knowing whether a replica is unhealthy due to a few block corruptions or complete container corruption/volume loss, unhealthy replicas should not count towards the container's durability.
    • In iteration 2, RM should delete the unhealthy containers whose BCSIDs are already represented in the healthy replicas.
      • In the future if unhealthy replicas can be recovered, there is potential to close these containers.

What is the link to the Apache JIRA

HDDS-6447

How was this patch tested?

Test were added and updated in TestLegacyReplicationManager. To aid in reviewing, tests in that class were grouped in to nested classes based on functionality. Tests in the UnstableReplicas class concern these changes, and all should be reviewed for expected behavior even if they do not show up in the diff. Tests in other classes were relocated with slight modification as necessary. For reviewers of this file I would recommend verifying that all tests in the original TestLegacyReplicationManager are still present and passing, as the diff for this refactor is quite messy.

[errose28]

Hi @adoroszlai @sodonnel @nandakumar131 if you have time I would appreciate some feedback on the RM changes proposed in the description here before reviewing of the code starts.

[sodonnel]

This can only be QUASI_CLOSED containers, which cannot be force closed (not enough unique origin IDs), right?

[errose28]

This part of the code has changed since this review. Sorry I did not address it at the time, but I encourage you to check out the new version and post any remaining questions.

[siddhantsangwan]

Would this stream be sorted in ascending order and end up removing the lowest BCSIDs from candidates?

[errose28]

Right. This is now fixed and the comparator is reversed in the current code. See deleteExcessLowestBcsIDs

[sodonnel]

Looking at the suggestion for CLOSED container:

If all replicas are unhealthy, they should be replicated like healthy containers.

This is difficult / impossible for EC. For EC we need to reconstruct the container by reading the entire contents from a quorum of other containers and generating the missing data. If all containers are legitimately unhealthy, with some sort of read problem, we are going to get errors reading some of the blocks and be unable to reconstruct the data. It would likely be possible to do a partial reconstruction, but that comes with significant complexity too, as some block might be missing from one of the containers in the group, which would trip up the clients trying to read it.

For now, what we have done with EC is say that an UNHEALTHY replica is much like a missing one. If we have one or more, we treat the container as under replicated and try to fix it. If we also exclude UNHEALTHY from the over-replication handling, and treat them as if they are already gone. That way, over replication will not remove them.

Only if the container is neither over or under replicated will we remove the unhealthy replicas.

The problem we are left with, is that if too many are unhealthy, we cannot do a reconstruction and hence cannot fix the problem. Then we will not replicate them either, as we cannot really do that, and it is possible to lose some of the unhealthy replicas over time due to disk failures etc.

[siddhantsangwan]

@errose28 I'm still reviewing LegacyReplicationManager but changes in other classes look okay.

[siddhantsangwan]

I think this logic will not update this container's under replicated state in the report if it's recoverable and those maps are full

[errose28]

Good catch. I fixed this in fe2fd7e by having a dedicated method to generate the replication related parts of the report. Having them mixed with the handlers for these cases was messy.

[siddhantsangwan]

A side effect of this change is that Containers such as {Container State: CLOSED, Replicas: CLOSED, CLOSING, CLOSING} would be called under replicated. I saw that the handler will try to replicate only if closing these replicas won't achieve sufficient replication. Do you think it can be confusing to call such a container under replicated?

For EC in the new RM, we're treating UNHEALTHY replicas as "not there" since they're unavailable, CLOSED as candidates for replicating, and replicas in other states as available but cannot be replicated.

In ECContainerReplicaCount:

      /*
      Remove UNHEALTHY replicas because they are unavailable. They could be a
      reason for under replication but should not be a reason for over
      replication.

      For example, consider the following set of replicas for an EC 3-2
      container:
      Replica Index 1: Closed
      Replica Index 2: Closed
      Replica Index 3: Closed, Unhealthy (2 replicas for this index)
      Replica Index 4: Unhealthy
      Replica Index 5: Closed

      This is a case of under replication because index 4 is unavailable. Index
      3 is not considered over replicated because its second copy is unhealthy.
      */

[errose28]

Thanks for the detailed review @siddhantsangwan. I finished all previously WIP tests and they are covering more cases around closing containers with unhealthy replicas and saving BCSIDs like you mentioned.

A side effect of this change is that Containers such as {Container State: CLOSED, Replicas: CLOSED, CLOSING, CLOSING} would be called under replicated. I saw that the handler will try to replicate only if closing these replicas won't achieve sufficient replication. Do you think it can be confusing to call such a container under replicated?

Yes even though the code is doing the right thing by trying to close them, we should probably not report these as under replicated. I will fix the report during my day tomorrow.

[siddhantsangwan]

Yes even though the code is doing the right thing by trying to close them, we should probably not report these as under replicated. I will fix the report during my day tomorrow.

Another related point is whether RatisContainerReplicaCount should see all replicas that don't match with the container's state or only replicas that have UNHEALTHY state as "unhealthy". This affects the behaviour of isSufficientlyReplicated in that class.

[errose28]

Another related point is whether RatisContainerReplicaCount should see all replicas that don't match with the container's state or only replicas that have UNHEALTHY state as "unhealthy". This affects the behaviour of isSufficientlyReplicated in that class.

Right. Currently I am not counting the unhealthy replicas towards sufficient replication. This was done to minimize the changes to RatisContainerReplicaCount since this class is shared by the old and new RM. It seems this is causing test failures in the recently merged TestRatisOverReplicationHandler though.

Ultimately I think what needs to happen is RatisContainerReplicaCount gets refactored to more explicitly deal with unhealthy replicas. However I don't think we should do this while two RM implementations are using the same class for different unhealthy replica handling. I think we should leave RatisContainerReplicaCount as is in this PR and ignore the failures in TestRatisOverReplicationHandler. When unhealthy replica handling is ported to the new RM and we are ready to turn off the old RM, we can refactor RatisContainerReplicaCount.

[siddhantsangwan]

Right. Currently I am not counting the unhealthy replicas towards sufficient replication.

My concern is that it will affect all classes that use the isSufficientlyReplicated api. Another approach is to have a new method such as isSufficientlyReplicatedConsideringHealthy that only considers healthy replicas (as in replicas that match container state). This method can be used in legacy rm for the current changes.

[errose28]

@siddhantsangwan I managed to preserve the original functionality of RatisContainerReplicaCount by extending the class to make the minor change needed for the legacy RM in this PR. The child class can be deleted when the two RMs are reconciled. See changes in this range

[siddhantsangwan]

@errose28 The final changes look great. Thanks for working on this!