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

Add RHEL/Centos support to the system/users metricset #16902

Merged

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Mar 9, 2020

What does this PR do?

This PR is a simultaneous bugfix/enhancement that addresses both #16757 and #16753. Rather than use the dbus ListUnitsByPatterns method, we make an introspection call to dbus, look at the methods available to us, and then return a function pointer to make a call dependent on what is available. The goal is to make systemd do as much of the work is possible, only falling back to filtering methods within metricbeat as needed. The filtering behavior implemented here is designed to emulate what happens on the dbus end via ListUnitsByPatterns. This also exposes pattern_filter since it was a 2-line change, and should be useful.

Why is it important?

Currently, this metricset doesn't work on Centos7/RHEL7, due to the ListUnitsByPatterns method being too new.

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

Author's Checklist

  • Additonal testing needs to be done on centos and RHEL.

How to test this PR locally

  • Pull down and build the branch on centos 7.
  • Run this metricset. Optionally, add some pattern_filter or state_filter values.

Related issues

Closes #16757 and #16753

@fearful-symmetry fearful-symmetry requested a review from a team March 9, 2020 14:42
@fearful-symmetry fearful-symmetry self-assigned this Mar 9, 2020
@fearful-symmetry fearful-symmetry changed the title Services rhel support Add RHEL/Centos support to the system/users metricset Mar 9, 2020
@andresrc andresrc added the Team:Services (Deprecated) Label for the former Integrations-Services team label Mar 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@andresrc andresrc requested a review from a team March 9, 2020 15:18
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.

Nice, thanks! It will need some changelog entries, for the fix and for the new option.

Do you think it would be possible to add some integration tests? I would be ok with leaving this for a future change as it can be tricky for systemd, I will add the test-plan label by now.

metricbeat/module/system/service/dbus.go Outdated Show resolved Hide resolved

//call "introspect" on the systemd1 path to see what ListUnit* methods are available
obj := conn.Object("org.freedesktop.systemd1", dbusRaw.ObjectPath("/org/freedesktop/systemd1"))
err = obj.Call("org.freedesktop.DBus.Introspectable.Introspect", 0).Store(&props)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting!

@jsoriano jsoriano added the test-plan Add this PR to be manual test plan label Mar 10, 2020
@fearful-symmetry
Copy link
Contributor Author

@jsoriano Testing this isn't particularly easy--we don't have any OS level integration tests, so I'm relying on testing parsers.

Co-Authored-By: Jaime Soriano Pastor <jaime.soriano@elastic.co>
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.

Intake is failing in CI, for the rest it LGTM.

@@ -112,6 +112,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Fix detection and logging of some error cases with light modules. {pull}14706[14706]
- Fix imports after PR was merged before rebase. {pull}16756[16756]
- Add dashboard for `redisenterprise` module. {pull}16752[16752]
- Dynamically choose a method for the system/service metricset to support older linux distros. {pull}16902[16902]
Copy link
Member

Choose a reason for hiding this comment

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

For a moment I thought that it should say system/users metricset, but is the title of the issue what is wrong. Remember this when forging the commit message! 🙂

@fearful-symmetry fearful-symmetry merged commit 1d36da7 into elastic:master Mar 12, 2020
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Mar 12, 2020
* major refactor to support different systemd version

* format and updates

* update ref docs

* update ref, again

* add newline

* Fix error string

Co-Authored-By: Jaime Soriano Pastor <jaime.soriano@elastic.co>

* add changelog entry

* make update

* add build target

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
(cherry picked from commit 1d36da7)
fearful-symmetry added a commit that referenced this pull request Mar 16, 2020
* major refactor to support different systemd version

* format and updates

* update ref docs

* update ref, again

* add newline

* Fix error string

Co-Authored-By: Jaime Soriano Pastor <jaime.soriano@elastic.co>

* add changelog entry

* make update

* add build target

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
(cherry picked from commit 1d36da7)
@andresrc andresrc added test-plan-added This PR has been added to the test plan and removed [zube]: Done labels Mar 16, 2020
vooon added a commit to sardinasystems/sensu-go-systemd-check that referenced this pull request Apr 3, 2020
@urso urso added the test-plan-ok This PR passed manual testing label Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Metricbeat Metricbeat Team:Services (Deprecated) Label for the former Integrations-Services team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan test-plan-ok This PR passed manual testing v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat] Support systemd version from RHEL 7 for system service metricset
5 participants