You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "siddhantsangwan (via GitHub)" <gi...@apache.org> on 2023/09/27 17:48:33 UTC
[GitHub] [ozone] siddhantsangwan opened a new pull request, #5369: HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning
siddhantsangwan opened a new pull request, #5369:
URL: https://github.com/apache/ozone/pull/5369
## What changes were proposed in this pull request?
Mix of quasi-closed and unhealthy replicas blocks decommission even if sufficiently replicated.
a. Caused when only some of the replicas hit the error during write.
b. Can be fixed by removing this check:
```
if (!replicaSet.isHealthy()) {
if (LOG.isDebugEnabled()) {
unhealthyIDs.add(cid);
}
if (unhealthy < CONTAINER_DETAILS_LOGGING_LIMIT
```
However, simply removing that check is not a complete solution. We need to try and preserve any UNHEALTHY replicas that have the greatest Sequence ID. https://issues.apache.org/jira/browse/HDDS-9321 takes care of the Legacy Replication Manager side of things to preserve such UNHEALTHY replicas. This jira focuses on the decommissioning side.
This PR already includes changes from #5366 so the reviewers can see the bigger picture and so that tests can be written. We should merge that PR before this one.
Changes:
1. Implemented `isSufficientlyReplicatedForOffline` in `LegacyRatisContainerReplicaCount`. The implementation checks `getVulnerableUnhealthyReplicas` for any UNHEALTHY replicas that need to saved.
2. Implemented `isHealthyEnoughForOffline` in `LegacyRatisContainerReplicaCount`. This method does not check for UNHEALTHY replicas like `isHealthy()`.
3. `DatanodeAdminMonitorImpl` now uses the `isHealthyEnoughForOffline` API instead of `isHealthy()` so this check does not block decommissioning. Of course, decommissioning still remains blocked until `isSufficientlyReplicatedForOffline` is satisfied and vulnerable UNHEALTHY replicas have been replicated.
This PR only deals with the LegacyReplicationManager + decommissioning flow. I've added a defensive check for "hdds.scm.replication.enable.legacy" in the decommissioning code.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9354
## How was this patch tested?
Added unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5369:
URL: https://github.com/apache/ozone/pull/5369#discussion_r1342698458
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyRatisContainerReplicaCount.java:
##########
@@ -59,4 +65,34 @@ protected int healthyReplicaCountAdapter() {
public int getUnhealthyReplicaCountAdapter() {
return getMisMatchedReplicaCount();
}
+
+ @Override
+ public boolean isHealthyEnoughForOffline() {
+ LifeCycleState containerState = getContainer().getState();
+ return (containerState == LifeCycleState.CLOSED
+ || containerState == LifeCycleState.QUASI_CLOSED)
+ && getReplicas().stream()
+ .filter(r -> r.getDatanodeDetails().getPersistedOpState() == IN_SERVICE)
+ .filter(r -> r.getState() !=
+ ContainerReplicaProto.State.UNHEALTHY)
+ .allMatch(r -> ReplicationManager.compareState(
Review Comment:
allMatch:
> If the stream is empty then true is returned and the predicate is not evaluated.
What is the intention if there are only out of service nodes or all are unhealthy? Do we expect this to return true or false?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on PR #5369:
URL: https://github.com/apache/ozone/pull/5369#issuecomment-1744685219
@sodonnel Thanks for the review. Merging master into this branch is giving me problems, so I'm closing this PR and just creating a new one - https://github.com/apache/ozone/pull/5385. It has no new changes, except addressing your review comment for `LegacyRatisContainerReplicaCount`. Let's continue working on this jira in the new PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #5369:
URL: https://github.com/apache/ozone/pull/5369#discussion_r1343550271
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyRatisContainerReplicaCount.java:
##########
@@ -59,4 +65,34 @@ protected int healthyReplicaCountAdapter() {
public int getUnhealthyReplicaCountAdapter() {
return getMisMatchedReplicaCount();
}
+
+ @Override
+ public boolean isHealthyEnoughForOffline() {
+ LifeCycleState containerState = getContainer().getState();
+ return (containerState == LifeCycleState.CLOSED
+ || containerState == LifeCycleState.QUASI_CLOSED)
+ && getReplicas().stream()
+ .filter(r -> r.getDatanodeDetails().getPersistedOpState() == IN_SERVICE)
+ .filter(r -> r.getState() !=
+ ContainerReplicaProto.State.UNHEALTHY)
+ .allMatch(r -> ReplicationManager.compareState(
Review Comment:
When all are UNHEALTHY, then I think it's fine to return true, because RM will still try to maintain replication factor replicas of that container and we don't want all UNHEALTHY blocking decommissioning.
Not so sure about the case when all are out of service. Such a case is unexpected because if RM is working properly, it shouldn't allow this to happen in the first place. I think the user of this class should use the other API, `isSufficientlyReplicatedForOffline`, to guard against this case. It seems to be out of scope for `isHealthyEnoughForOffline`, since this method is concerned with the health state of the replicas, while the other one checks for number of replicas. We can still return false from this method just to be defensive, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5369:
URL: https://github.com/apache/ozone/pull/5369#issuecomment-1743053779
Had a look at this - I have one comment and it needs a rebase after we committed the previous change before I review further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning [ozone]
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan closed pull request #5369: HDDS-9354. LegacyReplicationManager: Unhealthy replicas of a sufficiently replicated container can block decommissioning
URL: https://github.com/apache/ozone/pull/5369
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org