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

Fixes to metricbeat's KVM module #7793

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Jul 30, 2018

Some fixes to the kvm module:

  • Improper error handling caused a panic when connection to libvirtd couldn't be established. Fixes Metricbeats panics in kvm dommemstat metricset #7792.
  • Cleaned up error handling and reporting.
  • The default configuration couldn't possibly work:
    host: [ "localhost" ] results in the module trying to create a network connection without schema. It will always fail with "dial: Unknown network ".
    For a default installation of libvirtd, the only possible method is to use:
    host: [ "unix:///var/run/libvirt/libvirt-sock" ].
    Also some gidance is provided for setting up remote monitoring.

@adriansr adriansr added bug review Metricbeat Metricbeat in progress Pull request is currently in progress. and removed review labels Jul 30, 2018
@adriansr adriansr changed the title Improve error reporting on KVM dommemstat metric Fixes to metricbeat's KVM module Jul 30, 2018
@adriansr adriansr added review and removed in progress Pull request is currently in progress. labels Jul 30, 2018
@adriansr adriansr force-pushed the fix/mb/7792 branch 2 times, most recently from bd91665 to e0fbb1f Compare July 30, 2018 12:57
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.

LGTM, just a comment about the changelog entry

@@ -21,6 +21,8 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]

*Metricbeat*

- Fixed a panic when the kvm module cannot establish a connection to libvirtd. {issue}7792[7792].
Copy link
Member

Choose a reason for hiding this comment

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

Is it a breaking change?

@@ -2,7 +2,10 @@
metricsets: ["dommemstat"]
enabled: true
period: 10s
hosts: ["localhost"]
hosts: ["unix:///var/run/libvirt/libvirt-sock"]
# for remote hosts, setup network access in libvirtd.conf
Copy link
Member

Choose a reason for hiding this comment

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

s/for/For/

hosts: ["localhost"]
hosts: ["unix:///var/run/libvirt/libvirt-sock"]
# for remote hosts, setup network access in libvirtd.conf
# and use the tcp schema:
Copy link
Member

Choose a reason for hiding this comment

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

s/schema/scheme/, right?

}

if len(gotDomainMemoryStats) == 0 {
report.Error(errors.New("no domain memory stats found"))
report.Error(fmt.Errorf("no memory stats for domain %s", d.Name))
Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd prefer errors.Errorf over fmt.Errorf in case we decide to consume the available stack information that it adds to the error.

adriansr added 2 commits July 31, 2018 10:52
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes elastic#7792
The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.
@jsoriano
Copy link
Member

Test failure doesn't seem related.

@jsoriano jsoriano merged commit 76d3949 into elastic:master Jul 31, 2018
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Oct 23, 2018
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes elastic#7792

The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.

(cherry picked from commit 76d3949)
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Oct 23, 2018
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes elastic#7792

The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.

(cherry picked from commit 76d3949)
jsoriano added a commit that referenced this pull request Oct 24, 2018
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes #7792

The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.

Co-authored-by: Adrian Serrano <[email protected]>

(cherry picked from commit 76d3949)
jsoriano added a commit that referenced this pull request Oct 24, 2018
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes #7792

The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.

Co-authored-by: Adrian Serrano <[email protected]>

(cherry picked from commit 76d3949)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Improper error handling caused a panic when connection to libvirtd
couldn't be stablished.

Cleaned up error handling a little bit.

Fixes elastic#7792

The default settings for the kvm module didn't work.

Updated to connect to the unix socket by default, and provide a hint on
how to setup access to libvirtd running on remote hosts.

Co-authored-by: Adrian Serrano <[email protected]>

(cherry picked from commit 6de03f7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metricbeats panics in kvm dommemstat metricset
3 participants