Add supported-versions and update docker-compose for logstash module #29440

matschaffer · 2021-12-15T06:15:00Z

What does this PR do?

Updates logstash module tests to run on more recent and multiple logstash versions.

Why is it important?

The version embedded in docker-compose was very old (7.12)

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.~~

How to test this PR locally

cd metricbeat
MODULE=logstash mage goIntegTest pythonIntegTest

Related issues

Fixes: #30202

mergify · 2021-12-15T06:15:19Z

This pull request does not have a backport label. Could you fix it @matschaffer? 🙏
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 · 2021-12-15T06:53:12Z

💚 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-07T02:27:35.664+0000
Duration: 90 min 33 sec

Test stats 🧪

Test	Results
Failed	0
Passed	9442
Skipped	2516
Total	11958

💚 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!)

matschaffer · 2021-12-15T06:54:26Z

Ah.... there's no 8.1.0 but there should be a 8.1.0-SNAPSHOT. Is there any established pattern for how these versions should be set?

ruflin · 2021-12-15T07:20:34Z

metricbeat/module/logstash/docker-compose.yml

@@ -2,22 +2,22 @@ version: '2.3'

 services:
  logstash:
-    image: docker.elastic.co/integrations-ci/beats-logstash:${LOGSTASH_VERSION:-7.12.0}-1
+    image: docker.elastic.co/integrations-ci/beats-logstash:${LOGSTASH_VERSION:-8.1.0}-1


As this is the module collecting stats, I wonder if the tests should really be against a snapshot / unreleased version or rather a released version?

Can anyone confirm if this image definitely even exists? https://container-library.elastic.co/r/integrations-ci/beats-logstash gives me a 404 even when signed in via elastic okta.

I think this is built from the local Dockerfile: https://github.com/elastic/beats/blob/master/metricbeat/module/logstash/_meta/Dockerfile See the path to _meta here: https://github.com/elastic/beats/blob/master/metricbeat/module/logstash/docker-compose.yml#L7

Ah, I see. I guess the image line just gives it a local name then. Never used both build & image in the same file I don't think.

matschaffer · 2021-12-20T01:48:56Z

With -SNAPSHOT docker compose up seems to work at least. No idea if this will work in the integration harness or not. Hoping CI can tell us something about that.

ruflin · 2021-12-20T08:40:18Z

I expect 8.1.0-SNAPSHOT to work. But I wonder if this is really is what we want to test against for the module part. For all the other services, we test against stable releases, for example for mysql or redis. We test against snapshot elasticsearch releases for the output part to make sure to detect issues as early as possible. But as it has caused too many flaky tests we now have a script in place that updates to a specific hash and only merges if it goes green. For Logstash, we could either test against a stable release or also follow the out hash update approach. On the -SNAPSHOT approach I would like to push back because of the flakyness on CI.

In case the decision is made for the hash approach I'm sure @v1v is happy to help out.

v1v · 2021-12-20T08:56:01Z

For Logstash, we could either test against a stable release or also follow the out hash update approach

+1 for using the SNAPSHOT pinned hash approach. As Nicolas mentioned, this is the only way we can ensure the Beats project is not affected directly by breaking changes from the upstream but using PRs that get updated when a new SNAPSHOT is published. Those PRs are tagged with https://github.com/elastic/beats/pulls?q=is%3Apr+is%3Aopen+label%3Abuild-monitoring

The automation relies on https://github.com/elastic/beats/blob/master/.ci/bump-stack-version.sh so if LogStash is needed, can you please add the required sed/commands to update those files accordingly? For the record, VERSION is the same for each component in the ElasticStack, so the artifacts-api entrypoint is the one that provided those details

On the other hand, I've no clue about the docker registry : docker.elastic.co/integrations-ci/beats-elasticsearch and docker.elastic.co/integrations-ci/beats-logstash

matschaffer · 2022-01-04T06:36:16Z

Thanks @v1v and happy new year! Picking this back up. I'd say the 8.1.0-SNAPSHOT in the PR so far is right, but there's still the question of if bump-stack-version.sh should modify these files.

It doesn't look like the script makes any changes to metricbeat/module/* so far, but maybe it should?

What's still not clear to me is in what testing cycle metricbeat/module/logstash/docker-compose.yml gets used. Anyone know?

ruflin · 2022-01-10T09:11:36Z

Each module is tested as part of the integration and system tests. For the setup the above mentioned docker-compose file is used. This is tested on each PR or if by now we have some checks in place to see which files are modified, it will be run each time that the logstash module files are modified. But I think it is run on each PR currently.

jsoriano · 2022-01-10T14:57:38Z

For context, these modules are tested with released versions of the stack, recently we added a step to the release process to update the versions in metricbeat/docker-compose.yml and x-pack/metricbeat/docker-compose.yml after each release. These updates are done by an internal script, maybe these files for the logstash module should have been added there too. These files are [at the moment] different to the files used for each beat testing environments, that use snapshots that are updated with the process described in #29440 (comment).

If we want to follow the same approach as for the other stack monitoring modules, we should update the internal release script to update also the logstash files.

If we want to use tagged snapshots, we can go in the direction of updating .ci/bump-stack-version.sh, but we should also move there the update of the other metricbeat files, to avoid having different methods to update similar files.

elasticmachine · 2022-01-19T19:05:55Z

Pinging @elastic/stack-monitoring (Stack monitoring)

elasticmachine · 2022-01-19T19:05:55Z

Pinging @elastic/integrations (Team:Integrations)

jasonrhodes · 2022-01-26T15:11:27Z

Is this still necessary, @ruflin / @matschaffer ? I am reading through this ticket's comments and I don't follow what this change is in regards to. If it's not currently mergeable with a review from the Beats team, can we have a short meeting to chat about this so Mat and I can better understand what the request is?

matschaffer · 2022-01-26T23:44:44Z

Yeah, I still haven't wrapped my head around how these files are used. Figured I'd come around to it after 8.0 is stablized.

ruflin · 2022-01-27T10:33:10Z

Jaime and I have provided quite a few details on how this is used and where. What bits are missing or not clear. Happy to chat on a meeting if needed but async works too.

If we go for testing a stable release, this is very easy and quick to approve. If we go for a snapshot release which can make the full beats build unstable, we need to have more discussion. What version the integrations should test against is a decision your team @jasonrhodes should drive, happy to assist during the decision.

jasonrhodes · 2022-01-27T13:11:29Z

Ok yeah, I think this is another casualty of the gaps between ownership. I'll schedule a meeting so we can attempt to get on the same page.

jasonrhodes · 2022-01-28T14:13:26Z

@robbavey / @elastic/logstash can you take a look here and let me know if you have any input? I'm not sure if your team is aware of these tests at all, but I want to make sure we don't do something you didn't expect here.

…ack-versions

matschaffer · 2022-02-03T08:14:55Z

@v1v I think I hit a snag here and will need to know more about what calls bump-stack-version.sh

I'm fairly certain what we actually want is for metricbeat 8.x to be able to monitor (cc @robbavey and @jsoriano to confirm this guess).

But it looks like the 7.16 snapshot updates go straight to the https://github.com/elastic/beats/pull/30106/files branch.

For 8.x to test against 7.x, we'll need something that will run against the master branch when new releases of logstash 7.x are produced.

matschaffer · 2022-02-03T08:16:51Z

We could also merge this PR as is since at least we've got the basic pattern going. It appears all the pytests are skipped anyway.

We at least get some basic coverage from the golang tests, but they only support one version.

jsoriano · 2022-02-03T09:30:19Z

Hm... so MODULE=logstash mage pythonIntegTest uses the docker-compose file but MODULE=logstash mage goIntegTest doesn't.

Go tests in the logstash module do use compose, here for example.

Seems like the golang tests clean up pretty quickly whereas the pytests leave the containers running.

Umm, if I remember correctly it is the other way around, go tests leave the service containers running, pytests remove them on teardown (here).

Could it be that you run golang tests first and then the python ones? Maybe this way the scenario for the golang tests was still running.

It may happen that compose mixes up the scenarios when go and python tests are run without the supported versions file (because they share the same compose configuration and compose project name). Maybe if you were adding the supported versions file you saw things behave differently, with supported versions each version is started (and stopped) as a different compose project.

Regarding the update to bump-stack-version.sh, as you mention the needings here are slightly different, maybe we need a different automation here.

We could also merge this PR as is since at least we've got the basic pattern going. It appears all the pytests are skipped anyway.
We at least get some basic coverage from the golang tests, but they only support one version.

+1 to this, next thing would probably be to fix the tests, before further automation to update the versions.

jsoriano · 2022-02-03T09:31:38Z

metricbeat/module/logstash/_meta/supported-versions.yml

@@ -0,0 +1,3 @@
+variants:
+  - LOGSTASH_VERSION: 8.1.0-aa69d697-SNAPSHOT
+  - LOGSTASH_VERSION: 7.17.0-fd761817-SNAPSHOT


Consider including some released version too, to ensure compatibility with older versions, and to have a reference to compare with if upgrading a snapshot makes the test fail.

Yeah, makes sense. For now I'll add 7.17 and 8.0.0-rc2 I guess (cc @robbavey for cross check).

I have no idea how we'll keep those updated but maybe @v1v has some ideas and we have some time to figure it out.

The special case here are the snapshot versions, so I think we should have something like the bump stack versions approach.

The non-snapshot versions can be maintained manually, as done for the rest of modules, when we find some particularity in an specific version, or when we consider that we don't need testing for an specific versions anymore.

jsoriano · 2022-02-03T09:33:31Z

metricbeat/module/logstash/test_logstash.py

@@ -10,6 +10,7 @@
 import urllib.request


+@metricbeat.parameterized_with_supported_versions
 class Test(metricbeat.BaseTest):


Maybe we can skip the whole class till tests are fixed, otherwise this is probably going to be starting the containers for nothing in the CI pipelines.

yeah, makes sense to me

Makese the pytest output clearer ('s' for skip, not '.' for pass). And might avoid launching extra containers for no reason.

This avoids container launches.

matschaffer · 2022-02-04T03:03:05Z

metricbeat/module/logstash/test_logstash.py

@@ -20,8 +22,6 @@ def test_node(self):
        """
        logstash node metricset test
        """
-        unittest.skip('Skipping this test to check documented fields. We will unskip once we know which fields can be deleted')


Note that these tests (and the class) already have a top-level skip, so just removing the redundant lines here (especially since if we use this approach the test looks like a pass).

matschaffer · 2022-02-04T03:03:54Z

Okay, I think this is ready for revew/merge. @jsoriano maybe you can give it a ✅ and we can handle other changes as part of #30146

ruflin · 2022-02-04T07:10:47Z

metricbeat/module/logstash/_meta/supported-versions.yml

+  - LOGSTASH_VERSION: 8.1.0-aa69d697-SNAPSHOT
+  - LOGSTASH_VERSION: 8.0.0-rc2
+  - LOGSTASH_VERSION: 7.17.1-aa300c4a-SNAPSHOT
+  - LOGSTASH_VERSION: 7.17.0


I like to see that we test multiple variants. Lets make sure we do the same for the other stack modules.

Cool. That's the plan I laid out in #30146

…k-versions

jsoriano

👍 thanks for taking care of this @matschaffer! Waiting for the follow up fixing the skipped tests 🙂

matschaffer · 2022-02-07T14:27:17Z

@jsoriano are you able to merge this? I get "The base branch restricts merging to authorized users. " on this end

matschaffer · 2022-02-07T14:32:33Z

Or @ruflin perhaps? Guessing we might need to add @elastic/infra-monitoring-ui (or at least @elastic/stack-monitoring ) to the beats repo with write access.

robbavey

Apologies for the late review - was out the back end of last week. LGTM

…29440) Updates logstash module tests to run on more recent and multiple logstash versions. Completely skip logstash python tests to avoid running compose scenarios for skipped tests. (cherry picked from commit 478a5ce) # Conflicts: # metricbeat/module/logstash/docker-compose.yml

jsoriano · 2022-02-08T10:47:35Z

Merged, and backporting this to 8.1 and 8.0 branches.

…29440) Updates logstash module tests to run on more recent and multiple logstash versions. Completely skip logstash python tests to avoid running compose scenarios for skipped tests. (cherry picked from commit 478a5ce)

…29440) (#30269) Updates logstash module tests to run on more recent and multiple logstash versions. Completely skip logstash python tests to avoid running compose scenarios for skipped tests. (cherry picked from commit 478a5ce) Co-authored-by: Mat Schaffer <[email protected]>

…se for logstash module (#30268) Updates logstash module tests to run on more recent and multiple logstash versions. Completely skip logstash python tests to avoid running compose scenarios for skipped tests. (cherry picked from commit 478a5ce) It's just a _little_ odd to have 8.1 being tested by metricbeat 8.0. But I think this will help keep backports easier. In theory it could also help ensure 8.0 metricbeat updates don't break support for 8.1 logstash monitoring. Though such changes are probably unlikely. Co-authored-by: Mat Schaffer <[email protected]>

Match logstash docker-compose versions to stack branch versions

8747b0f

matschaffer requested review from jsoriano, ruflin and kaiyan-sheng December 15, 2021 06:15

matschaffer self-assigned this Dec 15, 2021

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 15, 2021

matschaffer requested a review from jasonrhodes December 15, 2021 06:15

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Dec 15, 2021

ruflin reviewed Dec 15, 2021

View reviewed changes

Switch to snapshot versions since they exist

bebac88

jsoriano added Feature:Stack Monitoring Team:Integrations Label for the Integrations team labels Jan 19, 2022

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

Merge remote-tracking branch 'upstream/master' into docker-compose-st…

f04aef7

…ack-versions

jsoriano reviewed Feb 3, 2022

View reviewed changes

Add 7/8 released tags, bump 7 snapshot to 7.17.1

3c2d27d

matschaffer mentioned this pull request Feb 4, 2022

Update supported-versions.yml and docker-compose.yml for stack monitoring modules as part of bump-stack-version.sh #30207

Closed

matschaffer linked an issue Feb 4, 2022 that may be closed by this pull request

[Stack Monitoring] Add supported-versions testing to logstash module #30202

Closed

matschaffer changed the title ~~Match logstash docker-compose versions to stack branch versions~~ Add supported-versions and update docker-compose for logstash module Feb 4, 2022

Move skips to top

f1ea273

Makese the pytest output clearer ('s' for skip, not '.' for pass). And might avoid launching extra containers for no reason.

matschaffer requested a review from robbavey February 4, 2022 02:59

Actually skip the whole class

8f80b0b

This avoids container launches.

matschaffer commented Feb 4, 2022

View reviewed changes

ruflin reviewed Feb 4, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into docker-compose-stac…

bc0fa6c

…k-versions

jsoriano approved these changes Feb 7, 2022

View reviewed changes

robbavey approved these changes Feb 7, 2022

View reviewed changes

jsoriano merged commit 478a5ce into elastic:main Feb 8, 2022

jsoriano added backport-v8.0.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Feb 8, 2022

mergify bot mentioned this pull request Feb 8, 2022

[8.0](backport #29440) Add supported-versions and update docker-compose for logstash module #30268

Merged

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

mergify bot mentioned this pull request Feb 8, 2022

[8.1](backport #29440) Add supported-versions and update docker-compose for logstash module #30269

Merged

Add supported-versions and update docker-compose for logstash module #29440

Add supported-versions and update docker-compose for logstash module #29440

Conversation

matschaffer commented Dec 15, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

mergify bot commented Dec 15, 2021

elasticmachine commented Dec 15, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

matschaffer commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matschaffer commented Dec 20, 2021

ruflin commented Dec 20, 2021

v1v commented Dec 20, 2021

matschaffer commented Jan 4, 2022

ruflin commented Jan 10, 2022

jsoriano commented Jan 10, 2022

elasticmachine commented Jan 19, 2022

elasticmachine commented Jan 19, 2022

jasonrhodes commented Jan 26, 2022

matschaffer commented Jan 26, 2022

ruflin commented Jan 27, 2022

jasonrhodes commented Jan 27, 2022

jasonrhodes commented Jan 28, 2022

matschaffer commented Feb 3, 2022

matschaffer commented Feb 3, 2022

jsoriano commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matschaffer commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

matschaffer commented Feb 7, 2022

matschaffer commented Feb 7, 2022

robbavey left a comment

Choose a reason for hiding this comment

jsoriano commented Feb 8, 2022

matschaffer commented Dec 15, 2021 •

edited

Loading

elasticmachine commented Dec 15, 2021 •

edited by jenkins-beats-ci bot

Loading