HDDS-7721. Make OM Ratis roles available in /prom endpoint #4140

xBis7 · 2023-01-03T18:24:40Z

What changes were proposed in this pull request?

Ratis roles are created as part of the OMMXBean class which makes them available only for /jmx. This information should be available for both /jmx and /prom endpoints of the OM.

In order to make the info available for the /prom endpoint, this patch adds a new metrics class that holds info about the component registering it, showing to the user if the current OM is a leader or a follower. The info is presented with a gauge(1 for leader, 0 for follower) so that it can be charted.

There will be a follow up PR, with a similar approach for the SCM.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7721

How was this patch tested?

A new test was added under hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithData.java.

This patch was also tested manually in docker clusters for both HA and non-HA like so

in /hadoop-ozone/dist/target/ozone-1.4.0-SNAPSHOT/compose/ozone

❯ docker-compose up --scale datanode=3 -d
  
0.0.0.0:9874/jmx
{
  "name" : "Hadoop:service=OzoneManager,name=OMHAMetrics",
  "modelerType" : "OMHAMetrics",
  "tag.NodeId" : "om1",
  "tag.Hostname" : "7eb57edf7920",
  "OzoneManagerHALeaderState" : 1
},
  
0.0.0.0:9874/prom
# TYPE omha_metrics_ozone_manager_ha_leader_state gauge
omha_metrics_ozone_manager_ha_leader_state{nodeid="om1",hostname="7eb57edf7920"} 1

in /hadoop-ozone/dist/target/ozone-1.4.0-SNAPSHOT/compose/ozone-ha

❯ docker-compose up --scale datanode=3 -d

0.0.0.0:9874/jmx
{
  "name" : "Hadoop:service=OzoneManager,name=OMHAMetrics",
  "modelerType" : "OMHAMetrics",
  "tag.NodeId" : "om1",
  "tag.Hostname" : "om1",
  "OzoneManagerHALeaderState" : 0
},

0.0.0.0:9874/prom
# TYPE omha_metrics_ozone_manager_ha_leader_state gauge
omha_metrics_ozone_manager_ha_leader_state{nodeid="om1",hostname="om1"} 0

kerneltime · 2023-01-03T19:24:31Z

Do you want to include docker-compose.yaml and docker-config changes? I think being able to read the metrics would be useful in general.

xBis7 · 2023-01-03T19:44:31Z

@kerneltime Thanks for the suggestion, I included the config for OM and SCM.

kerneltime · 2023-01-03T20:01:09Z

Question: Why the need to format the metric as

{ HostName: om1 | Node-Id: om1 | Ratis-Port : 9872 | Role: FOLLOWER}  { HostName: om2 | Node-Id: om2 | Ratis-Port : 9872 | Role: LEADER}  { HostName: om3 | Node-Id: om3 | Ratis-Port : 9872 | Role: FOLLOWER}

Would it make sense to break it up into individual values so it can be charted? It could be
leader: true
follower: false
ratis-port:
and so on...
I think Prometheus metrics have a specific scheme to it.

xBis7 · 2023-01-03T21:04:44Z

@kerneltime Thanks for looking into this.

Why the need to format the metric as

I wanted to avoid duplicating code, so I got the same string we are using in OMMXBean.getRatisRoles() from OzoneManager.getRatisRoles().

We can either split it up in new tags or have a Map and then use that for the tag.

Would it make sense to break it up into individual values so it can be charted?

We want to track the leader and if we are presenting info only for the current node, then we would have to go over all of them just to find the leader. If we want to simplify it, we could have a tag with only the info for the leader and skip all the followers.

Do you have any suggestions about the format? This fix is based on this discussion and #3791.

xBis7 · 2023-01-13T15:10:27Z

@kerneltime I updated the patch with the changes you requested. Now OMHAMetrics hold info only for the component registering them. All the info is added as tags instead of a concatenated string and also there is a gauge(1 for leader, 0 for follower) so that the metrics can be charted like you suggested. Can you take another look?

...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithData.java

hadoop-ozone/dist/src/main/compose/ozone-ha/docker-config

xBis7 · 2023-01-20T11:43:27Z

@neils-dev Thanks for the review! I have addressed your comments.

neils-dev · 2023-01-26T04:24:53Z

Thanks @xBis7. I took a look at the prom omha_metrics_ozone_manager_ha_leader_state metric when it is tracked and charted on om leadership transition. I rendered the prometheus endpoint with the prometheus web app and performed failover with 2 om nodes. On failover currently with the extra tag for "state" we get extra traces, in this case 4, one for each state change and gauge change, see - https://github.com/neils-dev/play/blob/main/images/failover_extra_traces.png.

It would be much cleaner to keep the tags to a min and just use the gauge to reflect the leader and changes to the leader. This can be seen when failover is rendered on prometheus with simplified tags, two traces this time, see - https://github.com/neils-dev/play/blob/main/images/failovers_just_gauge.png. Failover is the criss-cross when rendered.

xBis7 · 2023-01-26T15:59:07Z

@neils-dev I've tested it locally and I can see what you are referring to. I will remove state tag from OMHAMetrics. Thanks!

DaveTeng0 · 2023-01-30T09:04:02Z

Hey @xBis7 ! Seemed like some test was failing, please help take a look! Thanks~

xBis7 · 2023-01-30T09:48:24Z

Hey @DaveTeng0, the failure seems unrelated. Check here, I had a green build on the workflow of my fork. A lot of tests are flaky but I don't have write privileges to rerun the failed ones on the PR workflow.

adoroszlai

Thanks @xBis7 for working on this.

...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithData.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHAMetrics.java

xBis7 · 2023-02-01T14:38:46Z

@adoroszlai I've updated the patch, with the changes you requested. Can you please take another look?

adoroszlai

Thanks @xBis7 for updating the patch, LGTM.

If I understand correctly, @neils-dev's comment was also addressed previously in e836659.

xBis7 · 2023-02-01T17:03:36Z

If I understand correctly, @neils-dev's comment was also addressed previously in e836659.

@adoroszlai Yes, it has been addressed.

neils-dev · 2023-02-01T17:51:59Z

Thanks @xBis7 , @adoroszlai, I'm going to take another look.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHAMetrics.java

xBis7 · 2023-02-02T11:52:25Z

@neils-dev I've addressed your comments. Can you please take another look?

DaveTeng0 · 2023-02-13T08:58:28Z

Once @neils-dev takes a final look, this PR would be ready to be merged!

...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithData.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ha/OMHAMetrics.java

xBis7 · 2023-02-16T12:19:49Z

@neils-dev Double checking the leaderId seems unecessary. If for some reason there is no leader or there is an error and leaderId is empty, then the state is set to 0 pointing that the current node is a follower, which will be true.

I hadn't thought about the case where ratis is disabled but I looked into it and tested it. Since, in that scenario there are no leaders or followers, updatePeerList() never gets called and therefore OMHAMetrics are not registered. Checking if ratis is enabled is redundant because if it isn't and the method gets called we will get a NPE at the very first line when calling omRatisServer. I'll remove the check.

xBis7 · 2023-02-16T13:40:24Z

@neils-dev I've added some unit tests, so that we can check the case where we provide an empty leaderId.

neils-dev

Left a few comments. Thanks. Should remove the unintended addition file TestOzoneFsSnapshot.java that came in your latest commit.

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java

neils-dev · 2023-02-21T02:44:41Z

Checking if ratis is enabled is redundant because if it isn't and the method gets called we will get a NPE at the very first line when calling omRatisServer. I'll remove the check.

Thanks for updating that.

xBis7 · 2023-02-21T10:42:41Z

@neils-dev Thanks for the reviews, I've updated the patch. Let me know how it looks.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

xBis7 · 2023-02-21T18:04:00Z

@neils-dev I've updated the patch to address the latest comment.

neils-dev

Thanks @xBis7 . Looks good. Look to merge pending a green CI.

adoroszlai · 2023-02-22T08:00:22Z

Thanks @xBis7 for the patch, @kerneltime, @neils-dev for the review.

adoroszlai · 2023-02-22T12:26:21Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

+    RaftPeer leader = null;
+    try {
+      leader = omRatisServer.getLeader();
+    } catch (IOException ex) {
+      LOG.error("IOException while getting the " +
+          "Ratis server leader.", ex);
+    }
+    if (Objects.nonNull(leader)) {
+      String leaderId = leader.getId().toString();
+
+      // If leaderId is empty, then leader is undefined
+      // and current OM is neither leader nor follower.
+      // OMHAMetrics shouldn't be registered in that case.
+      if (!Strings.isNullOrEmpty(leaderId)) {


@neils-dev @xBis7

I had to update this part of the code after merging the PR due to a conflicting change that had been just merged (HDDS-6743, getLeader() no longer throws IOException).

Upon closer inspection, I think these if-else blocks have a problem. Please correct me if I'm wrong, but if leader is undefined then leader will be null, so the unregistration will not happen. (This seems to have been the case even before HDDS-6743.) If we have any leader information, its id cannot be null.

I think it should be:

if (Objects.nonNull(leader)) { // init } else { // unregister }

@adoroszlai You are right. This was missed because the code was

String leaderId = ""; try{ leader = } catch() { } if (Objects.nonNull(leader)) { } // leaderId could be deliberately left empty down here due to failure to get the leader // after refactoring `String leaderId = "";` was removed.

If we have any leader information, its id cannot be null.

I didn't know that.

How can we handle this now since the code has been merged?

Thanks @xBis7 for checking.

If we have any leader information, its id cannot be null.

I didn't know that.

https://github.com/apache/ratis/blob/27f5a59cc697ce51f1c5ab6d7b6c63eb58390b79/ratis-common/src/main/java/org/apache/ratis/protocol/RaftPeer.java#L175

How can we handle this now since the code has been merged?

HDDS-8009

@adoroszlai Thanks! I'll create a patch shortly for HDDS-8009.

Thanks @adoroszlai for merging and uncovering this change. With getLeader() no longer throwing an exception, we can cleanup the null check and handling of that condition.

xBis7 changed the title ~~Make OM Ratis roles available in /prom endpoint~~ HDDS-7721. Make OM Ratis roles available in /prom endpoint Jan 3, 2023

xBis7 force-pushed the HDDS-7721 branch from 101a3ed to a695800 Compare January 10, 2023 12:42

prashantpogde requested review from kerneltime, mukul1987 and prashantpogde January 19, 2023 17:54

neils-dev reviewed Jan 19, 2023

View reviewed changes

...ne/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithData.java Outdated Show resolved Hide resolved

neils-dev reviewed Jan 19, 2023

View reviewed changes

hadoop-ozone/dist/src/main/compose/ozone-ha/docker-config Outdated Show resolved Hide resolved

kerneltime approved these changes Jan 21, 2023

View reviewed changes

xBis7 force-pushed the HDDS-7721 branch from 0fd1913 to 4c3994f Compare January 26, 2023 16:32

neils-dev added the gr label Jan 26, 2023

adoroszlai requested review from adoroszlai and neils-dev February 1, 2023 11:04

adoroszlai requested changes Feb 1, 2023

View reviewed changes

xBis7 added 6 commits February 1, 2023 13:54

Make OM Ratis roles available in /prom endpoint

492048a

om, scm http-address setup for ozone-ha

12d620b

OM refactor OMHAMetricsInit() call

abf2de1

each OM holds info only about itself - value presented as gauge

b731eea

remove sleep from test

732bd36

removing state tag

e836659

xBis7 force-pushed the HDDS-7721 branch from 4c3994f to e836659 Compare February 1, 2023 11:54

adoroszlai approved these changes Feb 1, 2023

View reviewed changes

neils-dev reviewed Feb 2, 2023

View reviewed changes

xBis7 added 2 commits February 2, 2023 13:51

OMHAMetrics cleanup

4d608b0

Merge remote-tracking branch 'upstream/master' into HDDS-7721

5a4e484

neils-dev reviewed Feb 16, 2023

View reviewed changes

xBis7 added 3 commits February 16, 2023 14:29

cleanup

579778a

Merge remote-tracking branch 'upstream/master' into HDDS-7721

34cf3f3

unit tests

dd60ffc

adoroszlai requested a review from neils-dev February 17, 2023 20:48

neils-dev reviewed Feb 21, 2023

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java Outdated Show resolved Hide resolved

OMHAMetrics not registered if leaderId is empty

ac49ca7

neils-dev reviewed Feb 21, 2023

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Show resolved Hide resolved

if leaderid is empty unregister metrics

3100c35

neils-dev approved these changes Feb 22, 2023

View reviewed changes

adoroszlai merged commit 06e7b1b into apache:master Feb 22, 2023

adoroszlai reviewed Feb 22, 2023

View reviewed changes

xBis7 mentioned this pull request Feb 22, 2023

HDDS-8009. OM HA metrics should be unregistered if leader is not known #4300

Merged

xBis7 mentioned this pull request Mar 8, 2023

HDDS-8035. Intermittent timeout in TestOzoneManagerHAWithData.testOMHAMetrics #4362

Merged

mladjan-gadzic mentioned this pull request Apr 3, 2023

HDDS-8363. Make SCM Ratis roles available in /prom endpoint #4521

Merged

HDDS-7721. Make OM Ratis roles available in /prom endpoint #4140

HDDS-7721. Make OM Ratis roles available in /prom endpoint #4140

Uh oh!

Conversation

xBis7 commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

kerneltime commented Jan 3, 2023

Uh oh!

xBis7 commented Jan 3, 2023

Uh oh!

kerneltime commented Jan 3, 2023

Uh oh!

xBis7 commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xBis7 commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

xBis7 commented Jan 20, 2023

Uh oh!

neils-dev commented Jan 26, 2023

Uh oh!

xBis7 commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveTeng0 commented Jan 30, 2023

Uh oh!

xBis7 commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xBis7 commented Feb 1, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

xBis7 commented Feb 1, 2023

Uh oh!

neils-dev commented Feb 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xBis7 commented Feb 2, 2023

Uh oh!

DaveTeng0 commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xBis7 commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xBis7 commented Feb 16, 2023

Uh oh!

neils-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neils-dev commented Feb 21, 2023

Uh oh!

xBis7 commented Feb 21, 2023

Uh oh!

Uh oh!

xBis7 commented Feb 21, 2023

Uh oh!

neils-dev left a comment

Choose a reason for hiding this comment

xBis7 commented Jan 3, 2023 •

edited

Loading

xBis7 commented Jan 3, 2023 •

edited

Loading

xBis7 commented Jan 26, 2023 •

edited

Loading

xBis7 commented Jan 30, 2023 •

edited

Loading

xBis7 commented Feb 16, 2023 •

edited

Loading