Skip to content

Conversation

@mrodm
Copy link
Contributor

@mrodm mrodm commented May 9, 2023

Closes #1188

This PR adds into system test a new check to look for errors in container logs. These errors could be specified by means of patterns in the code.

A new parameter --profile (or -p) has been added into elastic-package test so it can be retrieved the profile information, required to dump the logs of the services.

This adds a new test result whose Test Name is (<service> logs) under the test type system.

Example of output setting fake patterns:

 $ elastic-package test system -v
...
FAILURE DETAILS:
elastic_package_registry/metrics (elastic-agent logs):
[0] found error "New State ID is AbF6RZN5"
[1] found error "New State ID is kTKFMOWJ"
elastic_package_registry/metrics (package-registry logs):
[0] found error "GET / HTTP/2.0"
[1] found error "GET /package/elastic_package_registry/0.0.6 HTTP/1.1"
[2] found error "GET /package/elastic_package_registry/0.0.6/ HTTP/1.1"
[3] found error "GET /search HTTP/1.1"


╭──────────────────────────┬─────────────┬───────────┬─────────────────────────┬───────────────────────────────────────────────────────────────────────────────────────┬───────────────╮
│ PACKAGE                  │ DATA STREAM │ TEST TYPE │ TEST NAME               │ RESULT                                                                                │  TIME ELAPSED │
├──────────────────────────┼─────────────┼───────────┼─────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┼───────────────┤
│ elastic_package_registry │ metrics     │ system    │ default                 │ PASS                                                                                  │ 22.331858707s │
│ elastic_package_registry │ metrics     │ system    │ (elastic-agent logs)    │ FAIL: test case failed: one or more errors found while examining elastic-agent.log    │    4.287049ms │
│ elastic_package_registry │ metrics     │ system    │ (package-registry logs) │ FAIL: test case failed: one or more errors found while examining package-registry.log │    85.89657ms │
╰──────────────────────────┴─────────────┴───────────┴─────────────────────────┴───────────────────────────────────────────────────────────────────────────────────────┴───────────────╯
--- Test results for package: elastic_package_registry - END   ---
Done

How to test this locally

In a given package, run the following commands:

 $ elastic-package build -v
 # start the stack with the required version (e.g. 8.0.0)
 $ elastic-package stack up -v -d --version <version>
 $ elastic-package test system -v
 $ elastic-package stack down

@mrodm mrodm changed the title Check logs from elastic-agent in system tests WIP - Check logs from elastic-agent in system tests May 9, 2023
@mrodm mrodm force-pushed the check_logs_from_agent_in_tests branch from e4ec44a to b1c8d3f Compare May 9, 2023 14:51
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good in general, added some observations and nitpicking.

Do not store all the messages of docker-compose logs in memory, they are
now processed with a function passed as a parameter. Removed unnecessary
variables.
@mrodm mrodm force-pushed the check_logs_from_agent_in_tests branch from 73e2434 to 9e6d45e Compare May 10, 2023 09:51
cmd.PersistentFlags().BoolP(cobraext.TestCoverageFlagName, "", false, cobraext.TestCoverageFlagDescription)
cmd.PersistentFlags().DurationP(cobraext.DeferCleanupFlagName, "", 0, cobraext.DeferCleanupFlagDescription)
cmd.PersistentFlags().String(cobraext.VariantFlagName, "", cobraext.VariantFlagDescription)
cmd.PersistentFlags().StringP(cobraext.ProfileFlagName, "p", "", fmt.Sprintf(cobraext.ProfileFlagDescription, install.ProfileNameEnvVar))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added profile parameter into elastic-package test command

@mrodm mrodm changed the title WIP - Check logs from elastic-agent in system tests Check logs from elastic-agent in system tests May 11, 2023
@mrodm mrodm marked this pull request as ready for review May 11, 2023 08:43
@mrodm
Copy link
Contributor Author

mrodm commented May 11, 2023

@mrodm I don't think it is a problem, it means that this packages might have to be fixed. Do you know what the problem is? Could it tests that on purpose logs errors?

I don't know the reason of these errors nor the root cause of them

@mrodm mrodm requested a review from jsoriano May 11, 2023 09:58
@jlind23
Copy link
Contributor

jlind23 commented May 11, 2023

As soon as we merge we should ask the teams to look at these. You can already ping them btw

Comment on lines 71 to 82
// logsRegexp{
// includes: regexp.MustCompile("New State ID"),
// excludes: []*regexp.Regexp{
// regexp.MustCompile("is unahorized API key id"),
// },
// },
// logsRegexp{
// includes: regexp.MustCompile("->HEALTHY"),
// excludes: []*regexp.Regexp{
// regexp.MustCompile(`Healthy$`),
// },
// },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be deleted

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, could we add some tests that cover the pattern matching logic?

@mrodm mrodm requested a review from jsoriano May 17, 2023 07:49
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

@jlind23
Copy link
Contributor

jlind23 commented Jun 1, 2023

@mrodm @jsoriano are we good with merging this or is there still some work to do?

@mrodm
Copy link
Contributor Author

mrodm commented Jun 1, 2023

@mrodm @jsoriano are we good with merging this or is there still some work to do?

@jlind23 there is still a final review pending, but I hope it shouldn't be pending too many changes.
But, considering that this PR would be approved, I have some concerns on merging this.

There are still three issues pending to be fixed (one was already fixed):

If this PR is merged, then any attempt to update elastic-package on integrations repository would be blocked (there would be failures) and even future elastic-package releases.

If that following PR in integrations is merged (those failures are ignored), then all those packages affected would be failing and they could not release any new version of the package.

What should we do here? @jsoriano @jlind23

@jlind23
Copy link
Contributor

jlind23 commented Jun 1, 2023

@mrodm in order to move forward can't we mark these 3 failing packages as an exception until it's fixed? If we wait more some new failures can be introduced in the mean time

@mrodm
Copy link
Contributor Author

mrodm commented Jun 1, 2023

@mrodm in order to move forward can't we mark these 3 failing packages as an exception until it's fixed? If we wait more some new failures can be introduced in the mean time

Not sure how to filter that, packages could fail for other reasons too.

An option (not the best one), they could ask us to merge those PRs...

@jsoriano
Copy link
Member

jsoriano commented Jun 1, 2023

@mrodm in order to move forward can't we mark these 3 failing packages as an exception until it's fixed? If we wait more some new failures can be introduced in the mean time

Not sure how to filter that, packages could fail for other reasons too.

An option (not the best one), they could ask us to merge those PRs...

It is possible to skip system tests (see docs). We could add something like this to their test definition when updating elastic-package in integrations:

skip:
  link: https://github.com/elastic/integrations/issues/6178
  reason: "This integration produces events that cannot be indexed"

Each one with its link, and include a comment on these issues about the test being skipped.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mrodm
Copy link
Contributor Author

mrodm commented Jun 1, 2023

It is possible to skip system tests (see docs). We could add something like this to their test definition when updating elastic-package in integrations:

skip:
  link: https://github.com/elastic/integrations/issues/6178
  reason: "This integration produces events that cannot be indexed"

Each one with its link, and include a comment on these issues about the test being skipped.

Thanks @jsoriano !
Then, it is good to be merged now, and we can add those skips in the test system configuration in the required packages in the integrations repository 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Testing] Fail test if there are errors in Agent component logs

4 participants