You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "xBis7 (via GitHub)" <gi...@apache.org> on 2023/11/21 17:50:18 UTC
[PR] HDDS-9645. Recon doesn't exclude out-of-service nodes when checking for healthy … [ozone]
xBis7 opened a new pull request, #5651:
URL: https://github.com/apache/ozone/pull/5651
…containers
## What changes were proposed in this pull request?
Datanodes that are `DECOMMISSIONING`, `DECOMMISSIONED`, `ENTERING_MAINTENANCE` or `IN_MAINTENANCE`, are considered out-of-service. SCM doesn't count their replicas as healthy and available when checking for under-replicated or over-replicated containers.
Recon should do the same and filter out-of-service nodes because
1. their replicas aren't actually available
2. it needs to be consistent with SCM
This patch modifies the `ContainerHealthStatus` constructor to achieve the desired behavior.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9645
## How was this patch tested?
New unit tests were added. This patch was also tested manually. The steps for manual testing can be found on the jira's description.
Green CI builds on my fork:
https://github.com/xBis7/ozone/actions/runs/6944974103
https://github.com/xBis7/ozone/actions/runs/6945013128
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1842700875
> Yea, it is going to be tricky to fix this in Recon, without duplicating much of the logic in SCM for this scenario.
I was looking into that, but right now I think the best solution would be to reuse the `RatisContainerReplicaCount` class from SCM and let Recon get `under-replication` or `over-replication` from there.
For example, `ContainerHealthStatus#isOverReplicated()`
```diff
public boolean isOverReplicated() {
- return replicaDelta < 0;
+ return ratisContainerReplicaCount.isOverReplicated();
}
```
No pending replica operations will be considered for Recon but it will be almost in sync with the SCM and any SCM changes for calculating `over-replication` or `under-replication` will be carried over to Recon without further changes.
@sodonnel What do you think?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1845226355
@devmadhuu Thanks for the comments.
> isMissing() still considers total replicas irrespective of healthy/unhealthy. Shouldn't we consider only HealthyReplicas here ?
I changed that because I think it's a discrepancy. Missing container for the SCM means an empty replica list, regardless of healthy/unhealthy. If a container has only 1 replica and that replica is unhealthy, it won't be considered missing for SCM but Recon would be reporting it as such.
I got that from these places in the code
* that's where `ReplicationManager` marks a container missing [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/ClosingContainerHandler.java#L74-L78), the container replica list has ALL known replicas.
* that's where `LegacyReplicationManager` marks a container missing [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java#L535-L539) and [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java#L1834-L1837).
Please correct me if I misunderstood and the list checked contain only healthy containers.
> And we had this JIRA [HDDS-9633](https://issues.apache.org/jira/browse/HDDS-9633) to sync Recon container stats with RM container report.
I wasn't aware of that. Thanks.
> Is this PR covering all ?
This is covering `over-replication`, `under-replication` and `missing`. I think we can get rid of `replicaDelta` in Recon entirely and reuse SCM code for all calculations. I didn't want to move ahead with it because the scope will grow out of hand. We will have to modify all the Recon tests for containers.
I also haven't change anything about `mis-replication`. I have a PR open for an issue related to that.
> Can you add test matching admin CLI RM container report numbers matching for various states with Recon showing container stats or matching.
Yes, let me look into an integration test or a robot test. I don't know which is easier at the moment. We should be able to use `curl` to get the data from Recon UI.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1425198642
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
Review Comment:
I know, that's why I renamed the original method to `isProperlyReplicated()` and now every method in Recon, calling `isHealthy()`, now calls `isProperlyReplicated()`.
It's part of the problem, that Recon uses the same method naming as the SCM but has a different meaning for most of them.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1857240460
> @devmadhuu Thanks for the comments.
>
> > isMissing() still considers total replicas irrespective of healthy/unhealthy. Shouldn't we consider only HealthyReplicas here ?
>
> I changed that because I think it's a discrepancy. Missing container for the SCM means an empty replica list, regardless of healthy/unhealthy. If a container has only 1 replica and that replica is unhealthy, it won't be considered missing for SCM but Recon would be reporting it as such. I got that from these places in the code
>
> * `ReplicationManager` marks a container missing [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/ClosingContainerHandler.java#L74-L78), the container replica list has ALL known replicas.
>
> * `LegacyReplicationManager` marks a container missing [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java#L535-L539) and [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java#L1834-L1837).
>
>
> Please correct me if I misunderstood and the checked list contain only healthy containers.
>
> > And we had this JIRA [HDDS-9633](https://issues.apache.org/jira/browse/HDDS-9633) to sync Recon container stats with RM container report.
>
> I wasn't aware of that. Thanks.
>
> > Is this PR covering all ?
>
> This is covering `over-replication`, `under-replication` and `missing`. I think we can get rid of `replicaDelta` in Recon entirely and reuse SCM code for all calculations. I didn't want to move ahead with it because the scope will grow out of hand. We will have to modify all the Recon tests for containers.
>
> I also haven't changed anything about `mis-replication`. I have a PR open for an issue related to that.
>
> > Can you add test matching admin CLI RM container report numbers matching for various states with Recon showing container stats or matching.
>
> Yes, let me look into an integration test or a robot test. I don't know which is easier at the moment. We should be able to use `curl` to get the data from Recon UI.
@xBis7 thanks for continue working on this, As I understand Recon also was considering the container as missing when all 3 replicas are zero, so both SCM and Recon were treating missing container as same. You check the original code of Recon ContainerHealthStatus#isMissing method where Recon was using numReplicas == 0 match..
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1857669374
@devmadhuu Here is a summary of the changes in this PR:
Recon and SCM are using the same naming and terminology regarding unhealthy containers. The code in Recon was quite old and following a very simple process of identifying each kind of unhealthy container, compared to the SCM. Recon was ignoring the existence of offline nodes (decommission, maintenance) and was also providing a different explanation for some terminologies.
The goal of this PR, is to make Recon container page consistent with the ReplicationManager `admin container report`. To avoid duplicating the code or its logic, I reused the same methods that the ReplicationManager is using for identifying under-replication, over-replication etc. With these changes, Recon is using the same algorithms as the RM for unhealthy containers. Every change in the RM is carried over to Recon as well.
I think it's important to deal with common terminology, in the same manner, to be consistent and avoid confusion. I'm referring to the way Recon defines healthy and missing containers.
* healthy
* RM
* the container is closed or quasi-closed and all replicas are in the same state
* Recon
* the container isn't over-replicated or under-replicated or mis-replicated
* I didn't change the code but I renamed the method and all of its references. This is actually referring to proper replication and not what RM means by healthy.
* missing
* RM
* 0 replicas, no information or reference at all. An `admin container info` call from the CLI, returns empty for all fields. If a container has only 1 unhealthy replica and we can get at least some info from it, like pipeline ID, then the container isn't missing.
* Recon
* `ContainerHealthStatus` was getting a list of all the replicas and then filtering it to get just the healthy ones. Then checking the healthy replica list and if there are no healthy replicas, then the container is missing.
* I modified it to use the initial replica list, for checking for missing.
We can revert the healthy and missing changes to reduce the scope of this PR.
I've added an integration test, that places some nodes in decommission or maintenance and makes a container go missing. The test, compares the RM `admin container report` to Recon's response in every step and makes sure that the numbers are the same and that RM's sample of containers exist in Recon's list of unhealthy containers.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406275350
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
It is "OK" for a maintenance replica to be offline. The definition of maintenance is that one or two replicas out of 3 can be offline and the container is still considered healthy, so I am not sure if it is correct to just assume a maintenance copy is offline. If there was a scenario where all 3 of the replicas of a container were in maintenance, with no other available copies, then that would be under-replicated.
This leads to another question - what if a container has 3 replicas from alive nodes:
IN_SERVICE
IN_SERVICE
IN_MAINTENANCE
Then the IN_MAINTENANCE node gets shutdown, so there are only the 2 IN_SERVICE nodes left. This is not considered under-replicated and RM will take no action on this container, as it believes that the IN_MAINTENANCE copy will return.
SCM keeps track of replicas that are for Maintenance nodes and counts them as still being available so it can make decisions on this sort of thing.
The concern I have is that Recon shows different counts for under-replicated than the RM report and it can cause confusion to users.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1845249604
@devmadhuu https://issues.apache.org/jira/browse/HDDS-9633 That was our initial issue and while investigating we narrowed it down to Recon ignoring out-of-service nodes.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1441689482
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
+ }
+
+ public boolean isSufficientlyReplicated() {
+ return containerReplicaCount.isSufficientlyReplicated();
+ }
+
+ public boolean isProperlyReplicated() {
Review Comment:
Long-term, the best approach would be to remove all delta calculations from this class and rely entirely on `RatisContainerReplicaCount` and `ECContainerReplicaCount`.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1883592438
@sodonnel @sumitagrawl Can you take another look on this PR? Any comments on the approach?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406876671
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
As far as I understand, a node doesn’t go offline until its replicas have been copied to another node. While ENTERING_MAINTENANCE or DECOMMISSIONING container replicas are added or removed as needed to maintain proper replication. The container will be under-replicated until copies have been made and the node successfully becomes offline.
Once that is done, the container is correctly replicated, has 3 healthy and available replicas and 1 offline. SCM doesn’t report any under-replicated or over-replicated containers but Recon
- for master, counts 1 over-replicated because it sees 4 replicas (no distinction between online - offline).
- for this patch, 0 count.
When the offline datanode is stopped, SCM doesn’t count unhealthy containers and
- for master, Recon no longer counts 1 over-replicated container.
- for this patch, no change in Recon.
When having 3 healthy replicas and 3 nodes, and 1 of them prepares to go offline, the container is temporarily under-replicated until a replica copy is created. Once that is done, then the container is considered properly replicated and the node goes offline. There should no longer be an issue of under-replication.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1847090958
> However, what about EC containers? They will have the same problems, and there is an EcContainerReplicaCount class too. I see in getMinimumRequiredReplicaNum you are checking for EC or Ratis, so you also need to check for EC or Ratis and create the correct ReplicaCount object for EC or Ratis.
@sodonnel That's a valid point, let me look further into it. The way Recon is designed right now, it doesn't check for EC in any place and all the calculations are for Ratis. Also, I think there is no under-replication concept for EC, right?
> Another question - what does Recon do to the container replicas it knows about when a maintenance node goes offline? Does it purge them from memory, or keep them?
> What does recon do when an IN_SERVICE node goes dead, does it keep the replicas in memory marked as dead or does it just remove them like SCM does?
Both scenarios, dead node and a maintenance node going offline, are handled in the `DeadNodeHandler` and `ReconDeadNodeHandler` accordingly.
SCM doesn't remove replicas belonging to maintenance nodes, as I see [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DeadNodeHandler.java#L88). Recon doesn't care about maintenance or the node state in general. `ReconDeadNodeHandler` triggers a health check which gets the dead node's containers from ReconSCM and processes them. If the container is over or under replicated, it will be added in the unhealthy container records. If the container is marked as DELETED in the SCM, Recon will mark it as DELETED in its container map. There are no other actions, therefore the replicas are kept in memory.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1847169088
@sodonnel I've updated the patch for EC. BTW, I'm working on an integration test like @devmadhuu suggested.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1905703954
@devmadhuu I deliberately changed the meaning in some of Recon's methods, to make it consistent with the ReplicationManager. If @sodonnel and @sumitagrawl don't have an objection to the changes, then there isn't something pending.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406497574
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
For Recon, what would it display if a node is in maintenance and then stopped, so there are only 2 replicas online? Will it show 2 available and 3 expected, highlighting it as under replicated, or will it be seen as OK?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1425201711
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -87,19 +117,19 @@ public boolean isDeleted() {
}
public boolean isOverReplicated() {
- return replicaDelta < 0;
+ return containerReplicaCount.isOverReplicated();
}
public boolean isUnderReplicated() {
- return !isMissing() && replicaDelta > 0;
+ return !isMissing() && containerReplicaCount.isUnderReplicated();
Review Comment:
@sodonnel By removing `isUnderReplicated()` from the interface, this should be
```suggestion
return !isMissing() && !containerReplicaCount.isSufficientlyReplicated();
```
right?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1425198642
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
Review Comment:
I know, that's why I renamed the original method to `isProperlyReplicated()` and every method in Recon, calling `isHealthy()`, now calls `isProperlyReplicated()`.
It's part of the problem, that Recon uses the same method naming as the SCM but has a different meaning for most of them.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1441647606
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
+ }
+
+ public boolean isSufficientlyReplicated() {
+ return containerReplicaCount.isSufficientlyReplicated();
+ }
+
+ public boolean isProperlyReplicated() {
Review Comment:
isSufficientlyReplicated is where delta is 0 or over-replication, and isProperlyReplicated is delta 0 (no under/over replication) and no misReplication.
I think over-replication can be considered as healthy only (may not need check delta=0).
When perfectly replicated (delta=0), in that case need check for misReplication
We can rename method as isHealtyReplication() with above 2 condition, as sufficently replication also means properly replication.
Can see some comment in code related to this point,
org.apache.hadoop.hdds.scm.SCMCommonPlacementPolicy#validateContainerPlacement
> // There are scenarios where there could be excessive replicas on a rack due to nodes
> // in decommission or maintenance or over-replication in general.
> // In these cases, ReplicationManager shouldn't report mis-replication.
> // The original intention of mis-replication was to indicate that there isn't enough rack tolerance.
> // In case of over-replication, this isn't an issue.
> // Mis-replication should be an issue once over-replication is fixed, not before.
> // Adjust the maximum number of replicas per rack to allow containers
> // with excessive replicas to not be reported as mis-replicated.
> maxReplicasPerRack += Math.max(0, dns.size() - replicas);
> return new ContainerPlacementStatusDefault(
> currentRackCount.size(), requiredRacks, numRacks, maxReplicasPerRack,
> currentRackCount);
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1913252506
Thanks @xBis7 for the patch, @devmadhuu, @sodonnel, @sumitagrawl for the review.
I left a few minor suggestions for future changes.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1821389947
@devmadhuu Can you please take a look at this 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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1910326167
@sumitagrawl Let me explain
> But Recon defines all states where we don't have the correct number of replicas as unhealthy, so that the containers can appear in the container-endpoint.
Healthy has a different meaning for Recon. Recon recognizes 5 states
1. Over-replication
2. Under-replication
3. Mis-replication
4. Missing
5. Correct number of replicas
The comment from the `SCMCommonPlacementPolicy` says that we shouldn't have to worry about over-replication because none of the above states means unhealthy for SCM. This is what an unhealthy container is for SCM, check [here](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ContainerReplicaCount.java#L58-L74).
Recon says that if the container is in State 5 then it's healthy and if it's in any other state, I'll present it in the UI. The meaning here is different. If we change it the scope of the changes will grow out of hand.
> > So that we can say that a container is properly replicated if it's neither under-replicated nor mis-replicated.
>
> ok, You mean those container will be part of below category in Recon UI, right?
With this change, over-replicated containers won't appear in the UI. This is from master, if an `over-replicated` container is considered healthy, then this check [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java#L316-L318) will return and the container will never be added in the unhealthy containers record [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java#L323).
We need the container to be added in the unhealthy containers record, because it's used for the UI. Check [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java#L423) which is where we distinguish over-replication. In that method, it first checks if the container is healthy [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java#L429) and then checks for over-replication [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java#L475).
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1905640621
@xBis7 - Thanks for the addressing the comments. Any comments left ? What is pending ?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406338314
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
Like you said, for SCM, healthy replicas are also the ones that are offline [getHealthyReplicaCount()](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java#L193-L196).
When the SCM checks for over-replication or under-replication, it gets only the available replicas [getAvailableReplicas()](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java#L215-L221) based mostly on the `healthyReplicaCount` field.
The `healthyReplicaCount` doesn't count replicas from nodes in decommission or maintenance [countReplicas()](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java#L171).
```
this.healthyReplicas = healthyReplicas
```
I know that it says `healthyReplicas` but it should hold just the available replicas in order to be consistent with the SCM.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406876671
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
As far as I understand, a node doesn’t go offline until its replicas have been copied to another node. While ENTERING_MAINTENANCE or DECOMMISSIONING container replicas are added or removed as needed to maintain proper replication. The container will be under-replicated until copies have been made and the node successfully becomes offline.
Once that is done, the container is correctly replicated, has 3 healthy and available replicas and 1 offline. SCM doesn’t report any under-replicated or over-replicated containers but Recon
- for master, counts 1 over-replicated because it sees 4 replicas (no distinction between online - offline).
- for this patch, 0 count.
When the offline datanode is stopped, SCM doesn’t count unhealthy containers and
- for master, Recon no longer counts 1 over-replicated container.
- for this patch, no change in Recon.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
As far as I understand, a node doesn’t go offline until its replicas have been copied to another node. While ENTERING_MAINTENANCE or DECOMMISSIONING container replicas are added or removed as needed to maintain proper replication. The container will be under-replicated until copies have been made and the node successfully becomes offline.
Once that is done, the container is correctly replicated, has 3 healthy and available replicas and 1 offline. SCM doesn’t report any under-replicated or over-replicated containers but Recon
- for master, counts 1 over-replicated because it sees 4 replicas (no distinction between online - offline).
- for this patch, 0 count.
When the offline datanode is stopped, SCM doesn’t count unhealthy containers and
- for master, Recon no longer counts 1 over-replicated container.
- for this patch, no change in Recon.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1864299847
> @devmadhuu Here is a summary of the changes in this PR:
>
> Recon and SCM are using the same naming and terminology regarding unhealthy containers. The code in Recon was quite old and following a very simple process of identifying each kind of unhealthy container, compared to the SCM. Recon was ignoring the existence of offline nodes (decommission, maintenance) and was also providing a different explanation for some terminologies.
>
> The goal of this PR, is to make Recon container page consistent with the ReplicationManager `admin container report`. To avoid duplicating the code or its logic, I reused the same methods that the ReplicationManager is using for identifying under-replication, over-replication etc. With these changes, Recon is using the same algorithms as the RM for unhealthy containers. Every change in the RM is carried over to Recon as well.
>
> I think it's important to deal with common terminology, in the same manner, to be consistent and avoid confusion. I'm referring to the way Recon defines healthy and missing containers.
>
> * healthy
>
> * RM
>
> * the container is closed or quasi-closed and all replicas are in the same state
> * Recon
>
> * the container isn't over-replicated or under-replicated or mis-replicated
> * I didn't change the code but I renamed the method and all of its references. This is actually referring to proper replication and not what RM means by healthy.
>
> * missing
>
> * RM
>
> * 0 replicas, no information or reference at all. An `admin container info` call from the CLI, returns empty for all fields. If a container has only 1 unhealthy replica and we can get at least some info from it, like pipeline ID, then the container isn't missing.
> * Recon
>
> * `ContainerHealthStatus` was getting a list of all the replicas and then filtering it to get just the healthy ones. Then checking the healthy replica list and if there are no healthy replicas, then the container is missing.
> * I modified it to use the initial replica list, healthy and unhealthy.
>
>
> We can revert the healthy and missing changes to reduce the scope of this PR.
>
> I've added an integration test, that places some nodes in decommission or maintenance and makes a container go missing. The test, compares the RM `admin container report` to Recon's response in every step and makes sure that the numbers are the same and that RM's sample of containers exist in Recon's list of unhealthy containers.
Thanks @xBis7 for your effort on this patch. I think this looks much better in current state and regarding EC handling of containers, this can be taken up separately.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1827721612
Thanks @devmadhuu @sumitagrawl for the reviews. I'll give it some time if @dombizita @sodonnel have any comments before merging.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1839300428
@sodonnel After your comments regarding maintenance, I started looking more into it. We need to filter decommissioned nodes but not the ones in maintenance.
Let's say that we have 3 replicas
IN_SERVICE
IN_SERVICE
IN_SERVICE
and then we put 1 in maintenance
IN_SERVICE
IN_SERVICE
IN_MAINTENANCE
we have 2 healthy replicas and no copy is created.
If we put one more in maintenance, then we will have 1 healthy replica and therefore 1 copy is made to meet the replication requirements.
IN_SERVICE
IN_SERVICE
IN_MAINTENANCE
IN_MAINTENANCE
For SCM there is no issue, for Recon the container is over-replicated as it has 4/3 replicas.
The container will be over-replicated when both nodes are out of maintenance but until then, the SCM correctly ignores the excessive replicas.
If we filter the nodes in maintenance, Recon will report under-replication even with 2 healthy replicas, which is worse because that means potential data loss.
I'll temporarily convert this PR into a draft while trying to figure out a way to be consistent with SCM regarding maintenance nodes.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1441688656
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
+ }
+
+ public boolean isSufficientlyReplicated() {
+ return containerReplicaCount.isSufficientlyReplicated();
+ }
+
+ public boolean isProperlyReplicated() {
Review Comment:
@sumitagrawl I couldn't figure out how to call this method. The original name was `isHealthy` and I just wanted to change that, to avoid having a different meaning here than the SCM has.
> We can rename method as isHealtyReplication() with above 2 condition,
That makes sense to me. I'll rename it.
> Can see some comment in code related to this point,
org.apache.hadoop.hdds.scm.SCMCommonPlacementPolicy#validateContainerPlacement
I added that comment in a previous PR, to explain that when having `over-replication`, we shouldn't also have `mis-replication`. Although, `over-replication` is an unhealthy state, `mis-replication` refers to not having enough racks or having an uneven replica distribution per rack. Having too many replicas in a rack, isn't an issue of `mis-replication` but just `over-replication`.
> I think over-replication can be considered as healthy only (may not need check delta=0).
With the above comment, it doesn't mean that when a container is over-replicated then it's entirely healthy, we still need to fix over-replication(disk space issue, etc.).
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1850152564
While working on the integration test, I created an EC container and then compared SCM to Recon. I found out that Recon-SCM doesn't store EC containers. I filed this jira for the issue
https://issues.apache.org/jira/browse/HDDS-9898
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1846963602
> I've updated ContainerHealthStatus to use RatisContainerReplicaCount for calculating over-replication or under-replication.
This seems like a good idea. However, what about EC containers? They will have the same problems, and there is an EcContainerReplicaCount class too. I see in `getMinimumRequiredReplicaNum` you are checking for EC or Ratis, so you also need to check for EC or Ratis and create the correct ReplicaCount object for EC or Ratis.
Another question - what does Recon do to the container replicas it knows about when a maintenance node goes offline? Does it purge them from memory, or keep them?
What does recon do when an IN_SERVICE node goes dead, does it keep the replicas in memory marked as dead or does it just remove them like SCM does?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1905820430
If @devmadhuu is happy I am happy for this to be committed.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5651:
URL: https://github.com/apache/ozone/pull/5651
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1857241372
@xBis7 there is too much of discussion in this pR, it would be helpful if you summarize the final changes in a para here ?what changed in Recon compared to SCM ?
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1842661905
Yea, it is going to be tricky to fix this in Recon, without duplicating much of the logic in SCM for this scenario. In SCM there is some logic to avoid removing replicas on the dead node event when the replica is in maintenance, and then RM sees it as still present.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1407450174
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
I didn't realize that It's different for MAINTENANCE. The case in the above comment is for DECOMMISSIONING. For MAINTENANCE, no copy is made.
On master, Recon doesn't show an error neither before nor after the datanode is stopped.
With this patch, Recon shows the container as under-replicated while it's in maintenance and continues to do so after the datanode is stopped.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406497574
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
For Recon, what would it display if a node is in maintenance and then stopped, so there are only 2 replicas online? Will it show 2 available and 3 expected, highlighting it as under replicated, or will it be seen as OK? In this case, no extra copy will be made on the cluster, so recon will never get to see 3 replicas.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1908135761
>> I think over-replication can be considered as healthy only (may not need check delta=0).
> With the above comment, it doesn't mean that when a container is over-replicated then it's entirely healthy, we still need to > fix over-replication(disk space issue, etc.).
This is only difference wrt org.apache.hadoop.hdds.scm.SCMCommonPlacementPolicy#validateContainerPlacement having description of the behavior. https://github.com/apache/ozone/pull/5651#discussion_r1441647606
@sodonnel what is your opinion for this? over-replication IMO does not make container as unhealty.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1468501845
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeUtil.java:
##########
@@ -0,0 +1,102 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.hdds.scm.node;
+
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.ozone.test.GenericTestUtils;
+import org.junit.jupiter.api.Assertions;
+
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Utility class with helper methods for testing node state and status.
+ */
+public final class TestNodeUtil {
Review Comment:
nit: Ozone's naming convention is:
* `Test*` for tests
* `*TestUtil` (or similar) for test helpers which do not have executable tests themselves
So ideally this should be `NodeTestUtil`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAndAdminContainerCLI.java:
##########
@@ -0,0 +1,485 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.recon;
+
+import com.google.common.base.Strings;
+import org.apache.hadoop.hdds.HddsConfigKeys;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState;
+import org.apache.hadoop.hdds.scm.ScmConfigKeys;
+import org.apache.hadoop.hdds.scm.ScmUtils;
+import org.apache.hadoop.hdds.scm.cli.ContainerOperationClient;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport.HealthState;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.node.NodeManager;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineManager;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineNotFoundException;
+import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
+import org.apache.hadoop.ozone.TestDataUtil;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.container.TestHelper;
+import org.apache.hadoop.ozone.om.helpers.BucketLayout;
+import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
+import org.apache.hadoop.ozone.recon.api.types.UnhealthyContainerMetadata;
+import org.apache.hadoop.ozone.recon.api.types.UnhealthyContainersResponse;
+import org.apache.hadoop.ozone.recon.scm.ReconNodeManager;
+import org.apache.hadoop.ozone.recon.scm.ReconStorageContainerManagerFacade;
+import org.apache.hadoop.ozone.recon.spi.ReconContainerMetadataManager;
+import org.apache.hadoop.ozone.recon.tasks.ReconTaskConfig;
+import org.apache.hadoop.hdds.scm.node.TestNodeUtil;
+import org.apache.ozone.test.GenericTestUtils;
+import org.apache.ozone.test.LambdaTestUtils;
+import org.hadoop.ozone.recon.schema.ContainerSchemaDefinition.UnHealthyContainerStates;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.slf4j.event.Level;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Collections.emptyMap;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_COMMAND_STATUS_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_HEARTBEAT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_NODE_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_PIPELINE_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_RECON_HEARTBEAT_INTERVAL;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.IN_SERVICE;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_DATANODE_ADMIN_MONITOR_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_DEADNODE_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_STALENODE_INTERVAL;
+import static org.apache.hadoop.ozone.recon.ReconServerConfigKeys.OZONE_RECON_OM_SNAPSHOT_TASK_INTERVAL_DELAY;
+
+/**
+ * Integration tests for ensuring Recon's consistency
+ * with the "ozone admin container" CLI.
+ */
+@Timeout(300)
+public class TestReconAndAdminContainerCLI {
+
+ private static final OzoneConfiguration CONF = new OzoneConfiguration();
+ private static ScmClient scmClient;
+ private static MiniOzoneCluster cluster;
+ private static NodeManager scmNodeManager;
+ private static long containerIdR3;
+ private static OzoneBucket ozoneBucket;
+ private static ContainerManager scmContainerManager;
+ private static ContainerManager reconContainerManager;
+
+ private static Stream<Arguments> outOfServiceNodeStateArgs() {
+ return Stream.of(
+ Arguments.of(NodeOperationalState.ENTERING_MAINTENANCE,
+ NodeOperationalState.IN_MAINTENANCE, true),
+ Arguments.of(NodeOperationalState.DECOMMISSIONING,
+ NodeOperationalState.DECOMMISSIONED, false)
+ );
+ }
+
+ @BeforeAll
+ public static void init() throws Exception {
+ setupConfigKeys();
+ cluster = MiniOzoneCluster.newBuilder(CONF)
+ .setNumDatanodes(5)
+ .includeRecon(true)
+ .build();
+ cluster.waitForClusterToBeReady();
+ GenericTestUtils.setLogLevel(ReconNodeManager.LOG, Level.DEBUG);
+
+ scmClient = new ContainerOperationClient(CONF);
+ StorageContainerManager scm = cluster.getStorageContainerManager();
+ PipelineManager scmPipelineManager = scm.getPipelineManager();
+ scmContainerManager = scm.getContainerManager();
+ scmNodeManager = scm.getScmNodeManager();
+
+ ReconStorageContainerManagerFacade reconScm =
+ (ReconStorageContainerManagerFacade)
+ cluster.getReconServer().getReconStorageContainerManager();
+ PipelineManager reconPipelineManager = reconScm.getPipelineManager();
+ reconContainerManager = reconScm.getContainerManager();
+
+ LambdaTestUtils.await(60000, 5000,
+ () -> (reconPipelineManager.getPipelines().size() >= 4));
+
+ // Verify that Recon has all the pipelines from SCM.
+ scmPipelineManager.getPipelines().forEach(p -> {
+ try {
+ Assertions.assertNotNull(reconPipelineManager.getPipeline(p.getId()));
Review Comment:
nit: please use static imports for assertions
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAndAdminContainerCLI.java:
##########
@@ -0,0 +1,485 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.recon;
+
+import com.google.common.base.Strings;
+import org.apache.hadoop.hdds.HddsConfigKeys;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState;
+import org.apache.hadoop.hdds.scm.ScmConfigKeys;
+import org.apache.hadoop.hdds.scm.ScmUtils;
+import org.apache.hadoop.hdds.scm.cli.ContainerOperationClient;
+import org.apache.hadoop.hdds.scm.client.ScmClient;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport.HealthState;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.node.NodeManager;
+import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineManager;
+import org.apache.hadoop.hdds.scm.pipeline.PipelineNotFoundException;
+import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.OzoneConfigKeys;
+import org.apache.hadoop.ozone.TestDataUtil;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.container.TestHelper;
+import org.apache.hadoop.ozone.om.helpers.BucketLayout;
+import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
+import org.apache.hadoop.ozone.recon.api.types.UnhealthyContainerMetadata;
+import org.apache.hadoop.ozone.recon.api.types.UnhealthyContainersResponse;
+import org.apache.hadoop.ozone.recon.scm.ReconNodeManager;
+import org.apache.hadoop.ozone.recon.scm.ReconStorageContainerManagerFacade;
+import org.apache.hadoop.ozone.recon.spi.ReconContainerMetadataManager;
+import org.apache.hadoop.ozone.recon.tasks.ReconTaskConfig;
+import org.apache.hadoop.hdds.scm.node.TestNodeUtil;
+import org.apache.ozone.test.GenericTestUtils;
+import org.apache.ozone.test.LambdaTestUtils;
+import org.hadoop.ozone.recon.schema.ContainerSchemaDefinition.UnHealthyContainerStates;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.slf4j.event.Level;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Collections.emptyMap;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_COMMAND_STATUS_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_CONTAINER_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_HEARTBEAT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_NODE_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_PIPELINE_REPORT_INTERVAL;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_RECON_HEARTBEAT_INTERVAL;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.IN_SERVICE;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_DATANODE_ADMIN_MONITOR_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_DEADNODE_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL;
+import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_STALENODE_INTERVAL;
+import static org.apache.hadoop.ozone.recon.ReconServerConfigKeys.OZONE_RECON_OM_SNAPSHOT_TASK_INTERVAL_DELAY;
+
+/**
+ * Integration tests for ensuring Recon's consistency
+ * with the "ozone admin container" CLI.
+ */
+@Timeout(300)
+public class TestReconAndAdminContainerCLI {
+
+ private static final OzoneConfiguration CONF = new OzoneConfiguration();
+ private static ScmClient scmClient;
+ private static MiniOzoneCluster cluster;
+ private static NodeManager scmNodeManager;
+ private static long containerIdR3;
+ private static OzoneBucket ozoneBucket;
+ private static ContainerManager scmContainerManager;
+ private static ContainerManager reconContainerManager;
+
+ private static Stream<Arguments> outOfServiceNodeStateArgs() {
+ return Stream.of(
+ Arguments.of(NodeOperationalState.ENTERING_MAINTENANCE,
+ NodeOperationalState.IN_MAINTENANCE, true),
+ Arguments.of(NodeOperationalState.DECOMMISSIONING,
+ NodeOperationalState.DECOMMISSIONED, false)
+ );
+ }
+
+ @BeforeAll
+ public static void init() throws Exception {
+ setupConfigKeys();
+ cluster = MiniOzoneCluster.newBuilder(CONF)
+ .setNumDatanodes(5)
+ .includeRecon(true)
+ .build();
+ cluster.waitForClusterToBeReady();
+ GenericTestUtils.setLogLevel(ReconNodeManager.LOG, Level.DEBUG);
+
+ scmClient = new ContainerOperationClient(CONF);
+ StorageContainerManager scm = cluster.getStorageContainerManager();
+ PipelineManager scmPipelineManager = scm.getPipelineManager();
+ scmContainerManager = scm.getContainerManager();
+ scmNodeManager = scm.getScmNodeManager();
+
+ ReconStorageContainerManagerFacade reconScm =
+ (ReconStorageContainerManagerFacade)
+ cluster.getReconServer().getReconStorageContainerManager();
+ PipelineManager reconPipelineManager = reconScm.getPipelineManager();
+ reconContainerManager = reconScm.getContainerManager();
+
+ LambdaTestUtils.await(60000, 5000,
+ () -> (reconPipelineManager.getPipelines().size() >= 4));
+
+ // Verify that Recon has all the pipelines from SCM.
+ scmPipelineManager.getPipelines().forEach(p -> {
+ try {
+ Assertions.assertNotNull(reconPipelineManager.getPipeline(p.getId()));
+ } catch (PipelineNotFoundException e) {
+ Assertions.fail();
+ }
+ });
+
+ Assertions.assertTrue(scmContainerManager.getContainers().isEmpty());
Review Comment:
nit: please use AssertJ for better failure message (e.g. it shows items of the list if unexpectedly found to be non-empty)
```java
import static org.assertj.core.api.Assertions.assertThat;
...
assertThat(scmContainerManager.getContainers()).isEmpty();
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconEndpointUtil.java:
##########
@@ -0,0 +1,185 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.recon;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Strings;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.server.http.HttpConfig;
+import org.apache.hadoop.hdfs.web.URLConnectionFactory;
+import org.apache.hadoop.ozone.recon.api.types.UnhealthyContainersResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.InputStream;
+import java.net.ConnectException;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+
+import static java.net.HttpURLConnection.HTTP_CREATED;
+import static java.net.HttpURLConnection.HTTP_OK;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_ADDRESS_DEFAULT;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_ADDRESS_KEY;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_HTTPS_ADDRESS_DEFAULT;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_HTTPS_ADDRESS_KEY;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_HTTP_ADDRESS_DEFAULT;
+import static org.apache.hadoop.hdds.recon.ReconConfigKeys.OZONE_RECON_HTTP_ADDRESS_KEY;
+import static org.apache.hadoop.hdds.server.http.HttpConfig.getHttpPolicy;
+import static org.apache.hadoop.http.HttpServer2.HTTPS_SCHEME;
+import static org.apache.hadoop.http.HttpServer2.HTTP_SCHEME;
+
+/**
+ * Utility class, used by integration tests,
+ * for getting responses from Recon Endpoints.
+ */
+public final class TestReconEndpointUtil {
Review Comment:
`ReconEndpointTestUtil`
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1909417065
> So that we can say that a container is properly replicated if it's neither under-replicated nor mis-replicated.
ok, You mean those container will be part of below category in Recon UI, right?
If so, I am fine with the change.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1908446893
@sumitagrawl Healthy has a different meaning for the ReplicationManager and for Recon and so it's tricky to define it. For RM, an over-replicated container isn't unhealthy. But Recon defines all states where we don't have the correct number of replicas as unhealthy, so that the containers can appear in the container-endpoint.
If I understand correctly, you are suggesting to change this
```java
public boolean isProperlyReplicated() {
return replicaDelta == 0 && !isMisReplicated();
}
```
to this
```java
public boolean isProperlyReplicated() {
return replicaDelta >= 0 && !isMisReplicated();
}
```
Is that correct?
So that we can say that a container is properly replicated if it's neither under-replicated nor mis-replicated.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1406306274
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -48,8 +48,12 @@ public class ContainerHealthStatus {
int repFactor = container.getReplicationConfig().getRequiredNodes();
this.healthyReplicas = healthyReplicas
.stream()
- .filter(r -> !r.getState()
- .equals((ContainerReplicaProto.State.UNHEALTHY)))
+ // Filter unhealthy replicas and
+ // replicas belonging to out-of-service nodes.
+ .filter(r ->
+ (!r.getDatanodeDetails().isDecommissioned() &&
+ !r.getDatanodeDetails().isMaintenance() &&
Review Comment:
> It is "OK" for a maintenance replica to be offline
But the issue here was that Recon was still counting it as online.
> The definition of maintenance is that one or two replicas out of 3 can be offline and the container is still considered healthy, so I am not sure if it is correct to just assume a maintenance copy is offline.
Here Recon was displaying the replicas in maintenance as unhealthy and in the over-replicated column.
> The concern I have is that Recon shows different counts for under-replicated than the RM report and it can cause confusion to users.
SCM container report doesn't count replicas that are in decommission or maintenance while Recon was counting them as unhealthy. That's the purpose of this patch, to make Recon consistent with SCM.
@sodonnel Check the example in the jira description. For example, I have 5 datanodes and 3 replicas. If 2 of the replica nodes go into decommission or maintenance, then 2 new replicas will be created in other nodes. In total, in the container info we will have 5 replicas but 2 of them will be offline and the SCM container report won't print any unhealthy replicas. Recon on the other hand, will also count the 2 offline replicas as online and will display that the container is over-replicated.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1843586540
While trying to make Recon consistent with the SCM, I ended up duplicating the code. I've updated `ContainerHealthStatus` to use `RatisContainerReplicaCount` for calculating over-replication or under-replication. This is the same class making these calculations in the SCM. With this approach, every change in how the SCM checks for over or under replication will be carried over to Recon without further modifications.
I've also made a few other small changes to eliminate discrepancies with the SCM. For example, a container is missing when it has 0 replicas in the system, regardless of healthy or unhealthy replicas. Recon was marking a container as missing if it had 0 `healthy` replicas.
I've updated the existing tests and they are passing and I've also added new unit tests. I've also got a couple green CIs on my fork, so there isn't anything broken with these changes.
This approach is more extensive than the original one but it ensures that Recon stays in sync with SCM. @devmadhuu @sumitagrawl @sodonnel @adoroszlai Can you please take a look?
There could be a follow up PR for making more methods in `ContainerHealthStatus` call their equivalents from `RatisContainerReplicaCount` and cleaning up the tests.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1425183102
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ContainerReplicaCount.java:
##########
@@ -49,6 +49,8 @@ public interface ContainerReplicaCount {
boolean isSufficientlyReplicatedForOffline(DatanodeDetails datanode,
NodeManager nodeManager);
+ boolean isUnderReplicated();
Review Comment:
I am not sure we really need another method on the interface which is just `!isSufficientlyReplicated()` - we can just call `!isSufficientlyReplicated()` without adding more to the interface.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #5651:
URL: https://github.com/apache/ozone/pull/5651#discussion_r1425186208
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java:
##########
@@ -78,6 +100,14 @@ public int getReplicationFactor() {
}
public boolean isHealthy() {
+ return containerReplicaCount.isHealthy();
Review Comment:
This is changing the meaning here. RepCount isHealthy is checking for the container being closed / quasi-closed and all replicas match that. The original isHealthy here was about not under / over rep and also not mis-rep.
--
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-9645. Recon doesn't exclude out-of-service nodes when checking for healthy containers [ozone]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #5651:
URL: https://github.com/apache/ozone/pull/5651#issuecomment-1854416225
@devmadhuu I've added the integration test.
To make sure that the test isn't flaky, I created a workflow in another branch, that runs the test on repeat. Each workflow runs the test 10 times. I got 3 green CIs.
https://github.com/xBis7/ozone/actions/runs/7197563498/job/19604976469#step:6:4035
https://github.com/xBis7/ozone/actions/runs/7197941719/job/19606235980#step:6:4035
https://github.com/xBis7/ozone/actions/runs/7197993201/job/19606410429#step:6:4035
--
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