Skip to content
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

Init commit of linux system tests #1340

Merged
merged 7 commits into from
Jul 27, 2021

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Jul 21, 2021

What does this PR do?

This adds system tests to (most) of the linux data streams, which should help in the future with catching low-hanging bugs.

However, we may want to look into expanding the test architecture somewhat, and I'd like input from @mtojek before this gets merged. The system integration is a bit odd, in that 90% of the code can be covered by pointed at a test file, without any external services, containers, etc. If we want to expand the system tests used in linux and system further, we need some kind of system that populates a test directory inside the agent container with a handful of files that the data streams can read from. I struggled to hack up a solution with what we have now, but decided it wasn't quite worth the ugliness. The system also seems to require test container service...so I had to make a dummy one.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

How to test this PR locally

  • Pull down PR
  • run elastic-package -v test system in the linux directory

@fearful-symmetry fearful-symmetry added the Team:Integrations Label for the Integrations team label Jul 21, 2021
@fearful-symmetry fearful-symmetry self-assigned this Jul 21, 2021
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link

elasticmachine commented Jul 21, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-26T21:40:02.026+0000

  • Duration: 17 min 49 sec

  • Commit: 8657407

Test stats 🧪

Test Results
Failed 0
Passed 25
Skipped 0
Total 25

Trends 🧪

Image of Build Times

Image of Tests

@ruflin ruflin requested a review from exekias July 22, 2021 06:10
@exekias
Copy link

exekias commented Jul 22, 2021

we need some kind of system that populates a test directory inside the agent container with a handful of files that the data streams can read from

Could you elaborate on this? Are you talking about /proc&/sys folders, or is this something else?

@jsoriano
Copy link
Member

we need some kind of system that populates a test directory inside the agent container with a handful of files that the data streams can read from

For system tests of metrics so far I think that we are only checking that metrics are collected without errors, we are not checking their values. For this case it may be also enough with checking that it collects any data, so the actual /proc and /sys filesystems available in the agent container would serve.

For logs you can use ${SERVICE_LOGS_DIR}.

@fearful-symmetry
Copy link
Contributor Author

Could you elaborate on this? Are you talking about /proc&/sys folders, or is this something else?
@exekias

Yes, along with a few other things. Depending on the OS config, some procfs files (such as RAID) won't contain any useful information for the metricset to parse. Some, such as cgroup files, won't exist at all inside a container. I spent some time hacking at the container orchestration that exists now to do that, but it was awfully ugly and I decided not to. At minimum, it would be nice to omit the "auxillary" container that elastic-package test system seems to require, as it's not actually needed here.

@exekias
Copy link

exekias commented Jul 26, 2021

For system tests of metrics so far I think that we are only checking that metrics are collected without errors, we are not checking their values. For this case it may be also enough with checking that it collects any data, so the actual /proc and /sys filesystems available in the agent container would serve.

I'm very much onboard with this for the initial batch of tests (this PR). It won't be testing the full package but many parts of it are still valid.

On top of that (maybe later), we could also have a testdata folder mounted into the agent, the same way we have one for logs, right? This way you could mock a raid file structure and configure the agent to retrieve it, for example.

@mtojek @jsoriano any thoughts on the need for a dummy container when there is no need for it? I'm not particularly concerned with its need by I see @fearful-symmetry point

@mtojek
Copy link
Contributor

mtojek commented Jul 26, 2021

This is a specific use case where you don't need to deploy anything. Temporarily you can use a lightweight/micro/dummy image.

Definitely not a blocker, but could be considered as a feature. Please open an issue in elastic-package for this.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

As you introduce the dependency on ECS, could you please review and adjust also other data streams? I see there are potential candidates, not sure about others:

https://github.com/fearful-symmetry/integrations/blob/linux-system-tests/packages/linux/data_stream/users/fields/ecs.yml
https://github.com/fearful-symmetry/integrations/blob/linux-system-tests/packages/linux/data_stream/socket/fields/ecs.yml

@fearful-symmetry
Copy link
Contributor Author

Alright, added the ECS fields to the other data streasm. @mtojek / @exekias I agree this isn't a blocker, just wanted to make the shortcomings known here. I'll open an issue on elastic-package.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM, but I left one comment regarding error.message to double-check.

@@ -0,0 +1,11 @@
- name: ecs.version
external: ecs
- name: error.message
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we don't add error.message fields. Is there any reason why you added it here? I mean, are there any problems with system tests? If so, then they should be fixed first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that was probably just me copying the ECS fields from other data streams. I can remove it.

@fearful-symmetry fearful-symmetry merged commit 124e69f into elastic:master Jul 27, 2021
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
* init commit of linux system tests

* fix changelog version

* format code

* use import ECS values

* format, again

* add ecs fields to the rest of the data_streams

* remove error ecs fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants