ZooKeeper module: Adapt to ZooKeeper 3.6+ `mntr` response fields' changes. by pfcoperez · Pull Request #30068 · elastic/beats

pfcoperez · 2022-01-27T17:34:21Z

What does this PR do?

This PR tries to tackle the compatibility issues that arise from trying to use the ZooKeeper metrics beats module with recent versions of this service: 3.6 and 3.7 majors.

Since ZooKeeper 3.6, some response to mntr 4lw commands requests have changed their implicit type from integers to floats. Fields like zk_avg_latency, zk_min_latency and zk_max_latency are some examples of this.

Additionally, zk_followers has been replaced by zk_learners.

This PR adds compatibility with these changes while not breaking pre ZK 3.6 ones.

Why is it important?

It is key for Elastic cloud operations to have these changes in beats. This can surely be extrapolated to other users which might wonder why certain fields are not longer ingested after having upgraded to any of the most recent ZooKeeper majors.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

cd metricbeat
MODULE=zookeeper mage -v pythonIntegTest

Related issues

Closes: #30066

Use cases

Screenshots

Logs

mergify · 2022-01-27T17:35:09Z

This pull request does not have a backport label. Could you fix it @pfcoperez? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2022-01-27T18:20:06Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-02-10T14:24:05.944+0000
Duration: 83 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7351
Skipped	1977
Total	9328

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2022-01-28T09:36:45Z

Pinging @elastic/siem (Team:SIEM)

elasticmachine · 2022-01-28T10:05:24Z

Pinging @elastic/integrations (Team:Integrations)

ruflin · 2022-01-28T10:05:54Z

I assume this will need backport to 8.0 and 7.17, not 7.12?

metricbeat/module/zookeeper/mntr/data.go

sayden · 2022-01-28T10:19:47Z

7.12 too so Cloud can do an progressive upgrade

sayden · 2022-01-28T10:41:27Z

All right I'll have to generate a switch in the code flow depending on the zookeeper version

jsoriano

7.12 too so Cloud can do an progressive upgrade

If backported to 7.12, it should be backported first to all the branches in between (7.13, 7.14...), to avoid someone using the fixed 7.12 version finding the problem again when upgrading to one of the others.

In any case remember that very probably there is not going to be any other 7.12 release, so this would only fix the issue for custom builds.

metricbeat/module/zookeeper/mntr/data.go

ruflin · 2022-01-31T08:18:06Z

There will not be any future 7.12 release. Even if we backport it, it never becomes available as a release. To get the change, Cloud has to go to 7.16, likely 7.17. @pfcoperez Is this an issue?

pfcoperez · 2022-01-31T12:20:17Z

There will not be any future 7.12 release. Even if we backport it, it never becomes available as a release. To get the change, Cloud has to go to 7.16, likely 7.17. @pfcoperez Is this an issue?

I think this might be an issue for ECE, would it be possible to do a minor official release?

pfcoperez · 2022-01-31T12:21:32Z

I'd like to include a run of the ITs with ZK 3.7 (in addition to the current 3.5). Would you mind pointing me into the steps to do it? 🙇

ruflin · 2022-02-02T07:57:59Z

Here is the compose file that spins up the environment: https://github.com/elastic/beats/blob/master/metricbeat/module/zookeeper/docker-compose.yml and the docker file: https://github.com/elastic/beats/blob/master/metricbeat/module/zookeeper/_meta/Dockerfile Interesting enough, it already has a variable for the version. @jsoriano I remember some services we tests multiple versions, do you have an example as reference?

jsoriano · 2022-02-02T11:37:32Z

I remember some services we tests multiple versions, do you have an example as reference?

Yep, we have support in the python tests. Take a look for example to the mysql or redis modules. You need these things:

Add the @metricbeat.parameterized_with_supported_versions decorator to the test class in test_zookeeper.py.
Parameterize the version or other variants in the docker compose file for zookeeper. I think that this is already done, but consider reviewing default values there, as they are the ones used by Go tests.
Add a _meta/supported-versions.yml file, with each one of the versions to test, take a look to the redis or mysql ones as example.

You can then run the python tests for this module with MODULE=zookeeper mage -v pythonIntegTest.

ruflin · 2022-02-02T12:15:49Z

@sayden As a follow up, we should also update the compatibility of the zookeeper package: https://github.com/elastic/integrations/tree/main/packages/zookeeper

pfcoperez · 2022-02-02T14:05:10Z

I remember some services we tests multiple versions, do you have an example as reference?

Yep, we have support in the python tests. Take a look for example to the mysql or redis modules. You need these things:

* Add the `@metricbeat.parameterized_with_supported_versions` decorator to the test class in `test_zookeeper.py`.

* Parameterize the version or other variants in the docker compose file for zookeeper. I think that this is already done, but consider reviewing default values there, as they are the ones used by Go tests.

* Add a `_meta/supported-versions.yml` file, with each one of the versions to test, take a look to the [redis](https://github.com/elastic/beats/blob/dd24764c4a9ff57b52d0289f9647fee8a5c9358e/metricbeat/module/redis/_meta/supported-versions.yml) or [mysql](https://github.com/elastic/beats/blob/dd24764c4a9ff57b52d0289f9647fee8a5c9358e/metricbeat/module/mysql/_meta/supported-versions.yml) ones as example.

You can then run the python tests for this module with MODULE=zookeeper mage -v pythonIntegTest.

Thank you @jsoriano ! I'll try to do it. I'll get back to you if I don't manage to.

pfcoperez · 2022-02-03T16:01:27Z

@jsoriano Thanks to your indications, in 8c9c62cbbf1f39d766b68349454d31ffb6d831eb, I've added the validation against ZooKeeper 3.6 and 3.7, in addition to the current 3.5.

📝 Note that I've changed the source image for ZooKeeper to the official ones: https://hub.docker.com/_/zookeeper Is that OK?

sayden

All right! Thank you very much for the help!

pfcoperez · 2022-02-09T14:33:13Z

@jsoriano @sayden Thanks a lot for your thorough reviews and insightful help.

In d034056 I've added UTs, may I ask you to please take a look? If you find them OK i'll just merge.

jsoriano

Thanks for adding unit tests! I have added some suggestions there, and I will leave the final word to @sayden 🙂

metricbeat/module/zookeeper/mntr/mntr_test.go

…ding them from test data files

jsoriano

LGTM

jsoriano · 2022-02-10T14:23:14Z

/test

sayden

Thanks for all the effort!

…nges. (#30068) (cherry picked from commit 3a311f1)

…r` response fields' changes. (#30360) (cherry picked from commit 3a311f1) Co-authored-by: Pablo Francisco Pérez Hidalgo <contact@pablofranciscoperez.info> Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

…nges. (#30068) (#30359) (cherry picked from commit 3a311f1) Co-authored-by: Pablo Francisco Pérez Hidalgo <contact@pablofranciscoperez.info> Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

…tr` response fields' changes. (#30358) (cherry picked from commit 3a311f1) Co-authored-by: Pablo Francisco Pérez Hidalgo <contact@pablofranciscoperez.info> Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

…-29710 * '8.1' of github.com:elastic/beats: (51 commits) refactor pushDockerImages (#30414) (#30624) ci: add windows-2022 in the extended meta-stage (#30528) (#30630) Curate k8s testing versions to only keep the actively maintained (#30619) (#30625) [8.1](backport #30355) Add Beats upgrade docs for 8.0 (#30612) Remove references to gcp from the Functionbeat docs (#30579) (#30609) x-pack/auditbeat/module/system/socket: defend against exec with zero arguments (#30586) (#30597) [MySQL Enterprise] Adding default paths values to manifest.yml (#30598) (#30604) metricbeat - fix elasticsearch and kibana integration tests failures in 8.0 (#30566) (#30594) Install gawk as a replacement for mawk in Docker containers. (#30452) (#30465) [Filebeat] Remove RecordedFuture dataset from Threat Intel module (#30564) (#30568) Adjust the documentation of `backoff` options in filestream input (#30552) (#30557) packetbeat/beater: help the GC clean up the Npcap installer if it's not used (#30513) (#30546) Osquerybeat: Add install verification for osquerybeat (#30388) (#30404) Update docker/distribution to 2.8.0 (#30462) (#30540) Add `parsers` examples to `filestream` reference configuration (#30529) (#30537) [8.1](backport #30068) ZooKeeper module: Adapt to ZooKeeper 3.6+ `mntr` response fields' changes. (#30360) [8.1](backport #30512) Switch skip to use `CI` (#30525) Forward-port 8.0.1 changelog to 8.1 (#30517) packetbeat/beater: don't attempt to install npcap when already installed (#30509) (#30511) Add drop and explicit tests to avoid duplicate ingest of elasticsearch logs (#30440) (#30488) ...

pfcoperez added the bug label Jan 27, 2022

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 27, 2022

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 27, 2022

pfcoperez added backport-v7.12.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Jan 27, 2022

mtojek added the Team:SIEM label Jan 28, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 28, 2022

ruflin added the Team:Integrations Label for the Integrations team label Jan 28, 2022

sayden self-requested a review January 28, 2022 10:19

mtojek reviewed Jan 28, 2022

View reviewed changes

metricbeat/module/zookeeper/mntr/data.go Outdated Show resolved Hide resolved

jsoriano added backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.0.0 Automated backport with mergify labels Jan 28, 2022

jsoriano reviewed Jan 28, 2022

View reviewed changes

metricbeat/module/zookeeper/mntr/data.go Outdated Show resolved Hide resolved

pfcoperez removed the backport-v7.12.0 Automated backport with mergify label Feb 2, 2022

sayden mentioned this pull request Feb 3, 2022

Upgrade compatibility of Zookeeper to 3.7 elastic/integrations#2639

Closed

mergify bot assigned pfcoperez Feb 8, 2022

sayden approved these changes Feb 9, 2022

View reviewed changes

PR Comments: Make code more idiomatic

9b40f02

mntr unit tests

d034056

pfcoperez force-pushed the fix/30066/zookeeper/recent_versions_compatibility branch from b8bee51 to d034056 Compare February 9, 2022 14:39

Lint style changes

93fde37

jsoriano reviewed Feb 9, 2022

View reviewed changes

metricbeat/module/zookeeper/mntr/mntr_test.go Outdated Show resolved Hide resolved

metricbeat/module/zookeeper/mntr/mntr_test.go Show resolved Hide resolved

metricbeat/module/zookeeper/mntr/mntr_test.go Outdated Show resolved Hide resolved

pfcoperez added 3 commits February 10, 2022 10:59

PR comments: UT, Test mntr samples against expected values

77212ad

PR Suggestions: Make use t.Run to group all tests for a given version

45b9b24

PR Suggestions: Embed mntr samples as string variable contents by loa…

e4c96c7

…ding them from test data files

jsoriano reviewed Feb 10, 2022

View reviewed changes

jsoriano requested a review from sayden February 10, 2022 12:48

jsoriano added the backport-v8.1.0 Automated backport with mergify label Feb 10, 2022

jsoriano assigned sayden Feb 10, 2022

sayden approved these changes Feb 11, 2022

View reviewed changes

sayden merged commit 3a311f1 into elastic:main Feb 11, 2022

mergify bot pushed a commit that referenced this pull request Feb 11, 2022

ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' cha…

893342f

…nges. (#30068) (cherry picked from commit 3a311f1)

mergify bot mentioned this pull request Feb 11, 2022

[7.17](backport #30068) ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' changes. #30358

Merged

mergify bot pushed a commit that referenced this pull request Feb 11, 2022

ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' cha…

069db49

…nges. (#30068) (cherry picked from commit 3a311f1)

mergify bot mentioned this pull request Feb 11, 2022

[8.0](backport #30068) ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' changes. #30359

Merged

mergify bot pushed a commit that referenced this pull request Feb 11, 2022

ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' cha…

cb38c51

…nges. (#30068) (cherry picked from commit 3a311f1)

mergify bot mentioned this pull request Feb 11, 2022

[8.1](backport #30068) ZooKeeper module: Adapt to ZooKeeper 3.6+ mntr response fields' changes. #30360

Merged

jsoriano mentioned this pull request Feb 22, 2022

[Release] Update test environments for 8.0.1 #28659

Merged

pfcoperez deleted the fix/30066/zookeeper/recent_versions_compatibility branch March 8, 2022 10:50

Conversation

pfcoperez commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

mergify bot commented Jan 27, 2022

Uh oh!

elasticmachine commented Jan 27, 2022 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

elasticmachine commented Jan 28, 2022

Uh oh!

elasticmachine commented Jan 28, 2022

Uh oh!

ruflin commented Jan 28, 2022

Uh oh!

Uh oh!

sayden commented Jan 28, 2022

Uh oh!

sayden commented Jan 28, 2022

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruflin commented Jan 31, 2022

Uh oh!

pfcoperez commented Jan 31, 2022

Uh oh!

pfcoperez commented Jan 31, 2022

Uh oh!

ruflin commented Feb 2, 2022

Uh oh!

jsoriano commented Feb 2, 2022

Uh oh!

ruflin commented Feb 2, 2022

Uh oh!

pfcoperez commented Feb 2, 2022

Uh oh!

pfcoperez commented Feb 3, 2022

Uh oh!

sayden left a comment

Choose a reason for hiding this comment

Uh oh!

pfcoperez commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

jsoriano commented Feb 10, 2022

Uh oh!

sayden left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pfcoperez commented Jan 27, 2022 •

edited

Loading

elasticmachine commented Jan 27, 2022 •

edited by jenkins-beats-ci bot

Loading

pfcoperez commented Feb 9, 2022 •

edited

Loading