-
Notifications
You must be signed in to change notification settings - Fork 625
HDDS-2642. Expose decommission / maintenance metrics via JMX #3781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
b432c98
48f27b0
110c612
8593546
526cd84
3faec5b
f4c9eb5
5a22113
ea64bfa
a8de744
a24ad0f
b13aab0
1232867
6d6b324
536773b
efc2554
3a0efd5
561c368
75ad487
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| */ | ||
| package org.apache.hadoop.hdds.scm.node; | ||
|
|
||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import org.apache.hadoop.hdds.conf.OzoneConfiguration; | ||
| import org.apache.hadoop.hdds.protocol.DatanodeDetails; | ||
| import org.apache.hadoop.hdds.protocol.proto.HddsProtos; | ||
|
|
@@ -38,8 +39,10 @@ | |
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.HashMap; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Queue; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
@@ -74,6 +77,51 @@ public class DatanodeAdminMonitorImpl implements DatanodeAdminMonitor { | |
| private Queue<DatanodeDetails> pendingNodes = new ArrayDeque(); | ||
| private Queue<DatanodeDetails> cancelledNodes = new ArrayDeque(); | ||
| private Set<DatanodeDetails> trackedNodes = new HashSet<>(); | ||
| private NodeDecommissionMetrics metrics; | ||
| private long pipelinesWaitingToClose = 0; | ||
| private long sufficientlyReplicatedContainers = 0; | ||
| private long trackedDecomMaintenance = 0; | ||
| private long trackedRecommission = 0; | ||
| private long unhealthyContainers = 0; | ||
| private long underReplicatedContainers = 0; | ||
|
|
||
| @SuppressFBWarnings(value = "SIC_INNER_SHOULD_BE_STATIC") | ||
| private final class ContainerStateInWorkflow { | ||
| private long sufficientlyReplicated = 0; | ||
| private long unhealthyContainers = 0; | ||
| private long underReplicatedContainers = 0; | ||
| private String host = ""; | ||
|
|
||
| private ContainerStateInWorkflow(String host, | ||
| long sufficientlyReplicated, | ||
|
sodonnel marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we ever pass non-zeros for these values? Perhaps we can just drop these parameters and let them default to zero?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, currently we instantiate zeroing the values then use the setters to update the values. The constructor contains the parameters in case we start initializing with non-zero values. If we don't use it, we can remove and default the parameters to zero. |
||
| long underReplicatedContainers, | ||
| long unhealthyContainers) { | ||
| this.host = host; | ||
| this.sufficientlyReplicated = sufficientlyReplicated; | ||
| this.unhealthyContainers = unhealthyContainers; | ||
| this.underReplicatedContainers = underReplicatedContainers; | ||
| } | ||
|
|
||
| public void setAll(long sufficiently, | ||
| long under, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formatting here seems off again - should either be 4 spaces in from the line above or aligned with the other parameters.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| long unhealthy) { | ||
| sufficientlyReplicated = sufficiently; | ||
| underReplicatedContainers = under; | ||
| unhealthyContainers = unhealthy; | ||
| } | ||
| public void reset() { | ||
| sufficientlyReplicated = 0L; | ||
| underReplicatedContainers = 0L; | ||
| unhealthyContainers = 0L; | ||
| } | ||
|
|
||
| public String getHost() { | ||
| return host; | ||
| } | ||
| } | ||
|
|
||
| private Map<String, ContainerStateInWorkflow> containerStateByHost; | ||
| private Map<String, Long> pipelinesWaitingToCloseByHost; | ||
|
|
||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(DatanodeAdminMonitorImpl.class); | ||
|
|
@@ -90,6 +138,9 @@ public DatanodeAdminMonitorImpl( | |
| this.eventQueue = eventQueue; | ||
| this.nodeManager = nodeManager; | ||
| this.replicationManager = replicationManager; | ||
|
|
||
| containerStateByHost = new HashMap<>(); | ||
| pipelinesWaitingToCloseByHost = new HashMap<>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why split the pipelines into a seperate map? It looks like it would be easier overall to have a pipeline count setter on the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split between replication state and pipelines was for grouping - they are initialized and set in separate parts of the monitor code that resulted in using two separate maps to store the two. Looking to, as suggested, reuse the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Combined all metrics collected by host in monitor to |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -117,6 +168,10 @@ public synchronized void stopMonitoring(DatanodeDetails dn) { | |
| cancelledNodes.add(dn); | ||
| } | ||
|
|
||
| public synchronized void setMetrics(NodeDecommissionMetrics metrics) { | ||
| this.metrics = metrics; | ||
| } | ||
|
|
||
| /** | ||
| * Get the set of nodes which are currently tracked in the decommissioned | ||
| * and maintenance workflow. | ||
|
|
@@ -140,15 +195,18 @@ public synchronized Set<DatanodeDetails> getTrackedNodes() { | |
| public void run() { | ||
| try { | ||
| synchronized (this) { | ||
| trackedRecommission = getCancelledCount(); | ||
| processCancelledNodes(); | ||
| processPendingNodes(); | ||
| trackedDecomMaintenance = getTrackedNodeCount(); | ||
| } | ||
| processTransitioningNodes(); | ||
| if (trackedNodes.size() > 0 || pendingNodes.size() > 0) { | ||
| LOG.info("There are {} nodes tracked for decommission and " + | ||
| "maintenance. {} pending nodes.", | ||
| trackedNodes.size(), pendingNodes.size()); | ||
| } | ||
| setMetricsToGauge(); | ||
| } catch (Exception e) { | ||
| LOG.error("Caught an error in the DatanodeAdminMonitor", e); | ||
| // Intentionally do not re-throw, as if we do the monitor thread | ||
|
|
@@ -168,6 +226,43 @@ public int getTrackedNodeCount() { | |
| return trackedNodes.size(); | ||
| } | ||
|
|
||
| synchronized void setMetricsToGauge() { | ||
| metrics.setTrackedContainersUnhealthyTotal(unhealthyContainers); | ||
| metrics.setTrackedRecommissionNodesTotal(trackedRecommission); | ||
| metrics.setTrackedDecommissioningMaintenanceNodesTotal( | ||
| trackedDecomMaintenance); | ||
| metrics.setTrackedContainersUnderReplicatedTotal( | ||
| underReplicatedContainers); | ||
| metrics.setTrackedContainersSufficientlyReplicatedTotal( | ||
| sufficientlyReplicatedContainers); | ||
| metrics.setTrackedPipelinesWaitingToCloseTotal(pipelinesWaitingToClose); | ||
| for (Map.Entry<String, Long> e : | ||
| pipelinesWaitingToCloseByHost.entrySet()) { | ||
| metrics.metricRecordPipelineWaitingToCloseByHost(e.getKey(), | ||
| e.getValue()); | ||
| } | ||
| for (Map.Entry<String, ContainerStateInWorkflow> e : | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be wrong, but I think there is a bug here. Lets say we put a host to maintenance. It will have some metrics tracked in the ByHost maps. After each pass we reset these maps to have zero counts, but we don't remove the entries from the maps anywhere (unless I have missed it). Then we update the values accordingly. Later the node goes back into service and even though it is removed from the monitor, it will be tracked with zero counts forever. Over time on a long running cluster, we will build up a lot of "by host" metrics with zero values, when they really should be removed. I think the reset will need to remove them from the maps rather than zeroing them, and also when setting the values to the metric gauge, you will need to remove values no longer there from it too. It might be easier to pass a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sodonnel , with the metrics registry it appears that the metrics we track remain in the registry. With this behavior, currently each datanode we add to track remains unless we have an api to remove it from the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huum, looks like you are correct. I wonder what the best approach is here. I don't think its a great user experience if we start with no individual nodes track, and then over time (in a long running SCM) more and more nodes get added for maintenance and decommission and the number builds up all with zero counts. I guess its not a major problem, but it would be nice to resolve it somehow.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #3791 Symious added a tag with a group of metrics in JSON form. For the metrics system, this is just a tag to string, rather than a gauge, but we could group all currently decommissioning / maintenence nodes into a JSON representation to expose the fine grained info. If no nodes are in the workflow, it would just be an empty json object, so nodes can come and go easily. Then you still have your aggregate metrics as they are now. It is unlikely that someone would want to chart an individual DN as they would have to create a new chart for each DN. What do you think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've modified the code to dynamically (without using the helper The host This seems to follow how hadoop handles metrics collected dynamically, however the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how the prom end point works. Its not ideal that it keeps the last value pushed, but I am not sure where that code even comes from!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. We should go forward with using this implementation that works for JMX metrics for completing this PR to expose decommission / maintenance metrics via JMX and open a new jira to look into supporting the prom endpoint. This PR supports metrics tracking the decommission and maintenance workflow both with aggregated counts and DN host specific counts. A jira will be filed to track prom endpoint behavior for the metrics. What do you think? |
||
| containerStateByHost.entrySet()) { | ||
| metrics.metricRecordOfReplicationByHost(e.getKey(), | ||
| e.getValue().sufficientlyReplicated, | ||
| e.getValue().underReplicatedContainers, | ||
| e.getValue().unhealthyContainers); | ||
| } | ||
| } | ||
|
|
||
| void resetContainerMetrics() { | ||
| pipelinesWaitingToClose = 0; | ||
| sufficientlyReplicatedContainers = 0; | ||
| unhealthyContainers = 0; | ||
| underReplicatedContainers = 0; | ||
|
|
||
| for (Map.Entry<String, ContainerStateInWorkflow> e : | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment, to reset things, you need to iterate this map and clear all values. Then iterate it again and remove any entries that are no longer tracked. What if, you just clear the map, which is a one liner. Then make ContainerStateInWorkflow a public inner class, and pass the That way reset becomes a lot easier too. Also, if we add another metric for a host, we don't need to add another parameter to the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, thanks, I was looking to couple the i.) there needs to be separate stores for numbers collected for the metrics from the monitor and numbers stored in the ii.) With the two separate stores, we need to know which hosts stored are currently in the workflow and which are out of the workflow and stale. Thus the check in the monitor code to collect those hosts that are stale and reporting that to the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the implementation as it stands now, protect against incomplete intermediate metrics? The metrics are snapshot via the call to getMetrics, but the metrics are set over several calls from the Decommission monitor to the metrics class and there is no synchronisation. Could we not set Probably, getMetrics() and any setters need synchronized, and even then you need to set everything in a single synchronized call. We can build up a Replace them all on the next call and then we don't need to worry about expiring individual nodes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On each scheduled run of the monitor, the implementation captures the current workflow state completely prior to flushing the metric update to the
Yes, I currently have been coding something just like that based on your earlier comment. With this, the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The code in It makes multiple calls to To make it consistent, you need to synchronize in the metrics object and set ALL the metrics in a single synchronized call.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Will do. At least try to keep possible inconsistency to a minimum. We won't report metrics that show one value in one sample, that clear, then go back to value due to our sampling from the monitor.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something we should look to add to the code, adding in a single call to the metrics object for all collected metrics to update? Modify
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Latest commit contains the modifications we are discussing -
Each iteration in the monitor collects snapshot of node metrics in workflow within threadLocal variables.
The |
||
| containerStateByHost.entrySet()) { | ||
| e.getValue().reset(); | ||
| } | ||
| pipelinesWaitingToCloseByHost.replaceAll((k, v) -> 0L); | ||
| } | ||
|
|
||
| private void processCancelledNodes() { | ||
| while (!cancelledNodes.isEmpty()) { | ||
| DatanodeDetails dn = cancelledNodes.poll(); | ||
|
|
@@ -188,7 +283,9 @@ private void processPendingNodes() { | |
| } | ||
|
|
||
| private void processTransitioningNodes() { | ||
| resetContainerMetrics(); | ||
| Iterator<DatanodeDetails> iterator = trackedNodes.iterator(); | ||
|
|
||
| while (iterator.hasNext()) { | ||
| DatanodeDetails dn = iterator.next(); | ||
| try { | ||
|
|
@@ -278,6 +375,9 @@ private boolean checkPipelinesClosedOnNode(DatanodeDetails dn) | |
| } else { | ||
| LOG.info("Waiting for pipelines to close for {}. There are {} " + | ||
| "pipelines", dn, pipelines.size()); | ||
| pipelinesWaitingToCloseByHost.put(dn.getHostName(), | ||
| (long) pipelines.size()); | ||
| pipelinesWaitingToClose += pipelines.size(); | ||
| return false; | ||
| } | ||
| } | ||
|
|
@@ -327,6 +427,17 @@ private boolean checkContainersReplicatedOnNode(DatanodeDetails dn) | |
| LOG.info("{} has {} sufficientlyReplicated, {} underReplicated and {} " + | ||
| "unhealthy containers", | ||
| dn, sufficientlyReplicated, underReplicated, unhealthy); | ||
| containerStateByHost.computeIfAbsent(dn.getHostName(), | ||
| hostID -> new ContainerStateInWorkflow(hostID, | ||
|
sodonnel marked this conversation as resolved.
Outdated
|
||
| 0L, | ||
| 0L, | ||
| 0L) | ||
| ).setAll(sufficientlyReplicated, | ||
| underReplicated, | ||
| unhealthy); | ||
| sufficientlyReplicatedContainers += sufficientlyReplicated; | ||
| underReplicatedContainers += underReplicated; | ||
| unhealthyContainers += unhealthy; | ||
| if (LOG.isDebugEnabled() && underReplicatedIDs.size() < 10000 && | ||
| unhealthyIDs.size() < 10000) { | ||
| LOG.debug("{} has {} underReplicated [{}] and {} unhealthy [{}] " + | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than suppress the FB warning, can this class be made
private final static class ...? I am not an expert in this area, but usually inner classes I've seen are static. The difference between static and non static inner classes seems to be that "non static" inner classes can directly access the enclosing classes instance variables and methods.A static inner class cannot directly access the enclosing classes methods. It has to do it via an object reference.
In this case, the inner class is a simple wrapper around a set of variables and does not need to access the enclosing methods class, and therefore can be static I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Removed the suppress annotation and properly converted instead to
static final nested classfrom thefinal inner class.As the inner class does not refer to the outer class instance, it should indeed be a static nested class.