[WIP] Collect domain memory stat from KVM #6265

amandahla · 2018-02-02T14:32:25Z

Just to start discussion about the implementation.
It collects the same output from the command "virsh dommemstat ".

Reference: #4402

elasticmachine · 2018-02-02T14:32:26Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2018-02-02T14:32:33Z

metricbeat/module/kvm/domain_memory/domain_memory.go

@@ -0,0 +1,129 @@
+package domain_memory


don't use an underscore in package name

ruflin · 2018-02-05T23:21:50Z

@amandahla Do you know what happens in the background / lib? Does it talk to an API or executes a binary?

amandahla · 2018-02-07T11:16:43Z

I understand that the go-libvirt make a local ("unix", "/var/run/libvirt/libvirt-sock") or RPC ("tcp", "192.168.1.12:16509") connection to the libvirt daemon. Then we can make the same thing that the virsh command does.
But there is a warning about RPC connections here: "The libvirt project strongly recommends against talking to the RPC interface directly. They consider it to be a private implementation detail with the possibility of being entirely rearchitected in the future."

ruflin · 2018-02-09T00:02:21Z

I would prefer if we don't execute any binaries if possible so I'm tempted to go the RPC direction for now. @andrewkroh Thouhgs?

amandahla · 2018-02-09T10:59:46Z

Sorry @ruflin but I understand that the library connects to the libvirt daemon without executing any binaries. I used the virsh command only as a reference.
Did I get it wrong? :|

andrewkroh · 2018-02-09T17:38:27Z

the library connects to the libvirt daemon without executing any binaries

👍 Good. At some point in the near future we will likely add a seccomp filter that prohibits execs (#5213).

ruflin · 2018-02-27T06:31:47Z

@amandahla I wonder what the next steps are you had in mind here? Seems like the PR is pretty close to be complete?

amandahla · 2018-03-08T13:31:54Z

@ruflin I think that I only need to write the tests. Is there anything else?

amandahla · 2018-03-08T17:51:31Z

Thinking about it...Should I use "Fetch(report mb.ReporterV2)"? I didn't find something like "mbtest.NewEventFetcher" for this to write the test.

andrewkroh · 2018-03-08T20:22:16Z

Sorry, the missing test utility was an oversight on my part.

Similar to NewReportingMetricSet there should be a NewReportingMetricSetV2 helper in mb/testing. Would you want to add the new test helper?

houndci-bot · 2018-03-19T18:50:41Z

metricbeat/mb/testing/modules.go

-	r.events = append(r.events, event)
-	return true
-}
+func ReportingFetchV2(metricSet mb.ReportingMetricSetV2) []mb.Event {


exported function ReportingFetchV2 should have comment or be unexported

amandahla · 2018-03-19T18:55:02Z

@andrewkroh I'm not sure if I did right but here it is :)

andrewkroh · 2018-03-19T19:36:04Z

I ended up needing this too for testing the perfmon metricset and added NewReportingMetricSetV2 and ReportingFetchV2 in d4eac97. I was going to leave a note here after it merged and I totally forgot. Can you see if what I added works for your testing.

amandahla · 2018-03-19T21:11:26Z

@andrewkroh Great, thanks! I didn't saw that. Now I removed what I did.

ruflin · 2018-03-21T07:57:14Z

@amandahla Is this PR ready for review?

ruflin · 2018-03-21T07:57:19Z

jenkins, test it

amandahla · 2018-03-21T12:08:50Z

@ruflin Yes, is ready.

jsoriano

I have added some comments. Please add also an entry to the changelog, and if possible a data.json. Thanks for this new module!

jsoriano · 2018-03-21T19:19:48Z

NOTICE.txt

+--------------------------------------------------------------------
+Dependency: github.com/digitalocean/go-libvirt
+Revision: 59d541f19311883ad82708651353009fb207d8a9
+License type (autodetected): UNKNOWN


@ruflin @andrewkroh can this be manually corrected?

@jsoriano No. Do you know the license type? We should update our script to detect the right one.

It seems to be Apache

The file that checks all the licenses is here: https://github.com/elastic/beats/blob/master/dev-tools/generate_notice.py Probably need some regexp / contains adjustements.

@tsg @monicasarbu FYI

These markdown formatted licenses seem to come from here: https://github.com/IQAndreas/markdown-licenses in the README it says: licenses are read by humans, not computers :)

I have created a PR to add this license #6632

So much about assumption who is going to be your user :-) #6632 got merged.

jsoriano · 2018-03-21T19:23:40Z

metricbeat/docs/modules/kvm.asciidoc

+----
+metricbeat.modules:
+- module: kvm
+  metricsets: ["memory"]


Metricset name is memory or dommemstat? Maybe memory is a good name, out of implementation details :) but for the rest of the PR it seems to be dommemstat.

I changed from "memory" to "dommemstat" to be consistent with the virsh command parameters to domain monitoring. It seems that I forgot to change this file :|

jsoriano · 2018-03-21T19:27:37Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+	cfgwarn.Experimental("The kvm dommemstat metricset is experimental.")
+
+	config := struct{}{}
+	if err := base.Module().UnpackConfig(&config); err != nil {


No needed if there is no custom configuration.

@amandahla could you also remove this?

jsoriano · 2018-03-21T19:33:20Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+		// when running tests, a mock Libvirt server is used
+		c = libvirttest.New()
+	} else {
+		c, err = net.DialTimeout(m.separatedURI[0], m.separatedURI[1], 2*time.Second)


Timeout could be obtained from configuration as in https://github.com/elastic/beats/blob/master/metricbeat/module/munin/node/node.go#L45

jsoriano · 2018-03-21T19:47:34Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+
+	return &MetricSet{
+		BaseMetricSet: base,
+		separatedURI:  strings.Split(base.HostData().URI, "://"),


Could we have here a safer parsing? Maybe using url.Parse. If protocol is not present in the host configuration, m.separatedURI[1] will panic on Fetch.

jsoriano · 2018-03-21T19:56:09Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+		// when running tests, a mock Libvirt server is used
+		c = libvirttest.New()
+	} else {
+		c, err = net.DialTimeout(m.separatedURI[0], m.separatedURI[1], 2*time.Second)


@andrewkroh @ruflin we might want to offer some kind of high-level "Dialer" that handles connections using info in BaseMetricSet. I guess lots of modules support unix/tcp and/or have to parse urls, handle with tls, timeouts configs... wdyt? do we already have something similar?

We have a http helper which contains the base metricset: https://github.com/elastic/beats/blob/master/metricbeat/helper/http.go#L19 Perhaps we can do in the future something similar?

jsoriano · 2018-03-21T19:58:55Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+		c, err = net.DialTimeout(m.separatedURI[0], m.separatedURI[1], 2*time.Second)
+		if err != nil {
+			report.Error(err)
+		}


defer c.Close()?

jsoriano · 2018-03-21T20:00:53Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+	}
+
+	for _, d := range domains {
+		l.DomainMemoryStats(d, 8, 0)


Is it needed?

Actually no, my mistake. :|

jsoriano · 2018-03-21T20:01:39Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+	for _, d := range domains {
+		l.DomainMemoryStats(d, 8, 0)
+
+		gotDomainMemoryStats, err := l.DomainMemoryStats(d, 8, 0)


What are these 8 an 0? Would it make sense to set them from constants?

8 -> Maximum stats that you want
0 -> Flags that you can pass
Actually it would be better to change to 11 since we have 11 possible stats and to set constants.

ruflin · 2018-03-22T14:04:40Z

metricbeat/module/kvm/dommemstat/_meta/fields.yml

@@ -0,0 +1,9 @@
+- name: dommemstat


We should complete the fields.yml here and have a system tests that validates that all fields are documented.

@amandahla could you complete this fields file?

jsoriano

It is fine to refer the issue in the commit message, but could you add additional information there about the change? Thanks!

jsoriano · 2018-03-28T17:30:08Z

metricbeat/metricbeat.reference.yml

@@ -317,6 +317,14 @@ metricbeat.modules:
  metricsets:
    - event

+#--------------------------------- kvm Module --------------------------------
+- module: kvm
+  metricsets: ["memory"]


memory still here, you may need to run make update again.

jsoriano · 2018-03-28T17:30:55Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+
+	u, err := url.Parse(m.Host())
+	if err != nil {
+		report.Error(err)


This parsing could be done in New so it is done only once. If it's kept here add a return after reporting the Error so it doesn't continue.

Moved to New, thanks!

jsoriano

Thanks for your changes. It mostly LGTM, there are a couple of comments that I think should be addressed before merging (deleting the empty custom config and filling the fields file), others can probably be done in future PRs as the module is going to be merged as experimental.

jsoriano · 2018-04-09T19:09:57Z

metricbeat/module/kvm/dommemstat/dommemstat.go

+	cfgwarn.Experimental("The kvm dommemstat metricset is experimental.")
+
+	config := struct{}{}
+	if err := base.Module().UnpackConfig(&config); err != nil {


@amandahla could you also remove this?

jsoriano · 2018-04-09T19:12:40Z

metricbeat/module/kvm/_meta/config.yml

+  hosts: ["localhost"]
+
+  # Timeout to connect to Libvirt server
+  #timeout: 1s


@ruflin we will have to add a reference config and default metricsets, as this module is experimental I think this can be left for a future PR

jsoriano · 2018-04-09T19:15:35Z

metricbeat/module/kvm/dommemstat/_meta/fields.yml

@@ -0,0 +1,9 @@
+- name: dommemstat


@amandahla could you complete this fields file?

ruflin · 2018-04-18T15:09:53Z

@amandahla Any chance you find some time to push this forward in the near future? Would be nice to have the code in. Let us know if we can help somehow.

amandahla · 2018-04-18T17:29:36Z

Sorry for the delay! Finally the commit with the changes asked.

jsoriano · 2018-04-18T22:42:38Z

jenkins, test this please

ruflin · 2018-04-19T12:23:39Z

metricbeat/modules.d/kvm.yml.disabled

@@ -0,0 +1,8 @@
+- module: kvm
+  metricsets: ["dommemstat"]
+  enabled: false


@exekias If this module is going to be enabled, will the enabled config be overwritten?

nope, I think we should remove the enabled setting here

@jsoriano I suggest we merge the PR and to a follow up PR to adjust the config and config reference file to our most recent logic.

jsoriano · 2018-04-19T16:14:28Z

Merged, thanks @amandahla !

houndci-bot reviewed Feb 2, 2018

View reviewed changes

ruflin added module review Metricbeat Metricbeat labels Feb 3, 2018

houndci-bot reviewed Mar 19, 2018

View reviewed changes

jsoriano reviewed Mar 21, 2018

View reviewed changes

ruflin reviewed Mar 22, 2018

View reviewed changes

jsoriano mentioned this pull request Mar 22, 2018

Add markdown-formatted Apache 2 title fo notice generator #6632

Merged

jsoriano requested changes Mar 28, 2018

View reviewed changes

jsoriano reviewed Apr 9, 2018

View reviewed changes

Collect domain memory stat from KVM (#4402)

b222403

jsoriano approved these changes Apr 18, 2018

View reviewed changes

ruflin reviewed Apr 19, 2018

View reviewed changes

jsoriano merged commit ccd8a31 into elastic:master Apr 19, 2018

This was referenced Apr 19, 2018

Metricbeat: Add reference config and default metricset to kvm module #6900

Merged

[Feature] - Metricbeat for libvirt/KVM #4402

Closed

[WIP] Collect domain memory stat from KVM #6265

[WIP] Collect domain memory stat from KVM #6265

Conversation

amandahla commented Feb 2, 2018 • edited Loading

elasticmachine commented Feb 2, 2018

Choose a reason for hiding this comment

ruflin commented Feb 5, 2018

amandahla commented Feb 7, 2018

ruflin commented Feb 9, 2018

amandahla commented Feb 9, 2018

andrewkroh commented Feb 9, 2018

ruflin commented Feb 27, 2018

amandahla commented Mar 8, 2018

amandahla commented Mar 8, 2018

andrewkroh commented Mar 8, 2018

Choose a reason for hiding this comment

amandahla commented Mar 19, 2018

andrewkroh commented Mar 19, 2018

amandahla commented Mar 19, 2018

ruflin commented Mar 21, 2018

ruflin commented Mar 21, 2018

amandahla commented Mar 21, 2018

jsoriano left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Mar 21, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Mar 21, 2018 • edited Loading

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

jsoriano left a 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

jsoriano left a 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

ruflin commented Apr 18, 2018

amandahla commented Apr 18, 2018

jsoriano commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 19, 2018

amandahla commented Feb 2, 2018 •

edited

Loading

jsoriano Mar 21, 2018 •

edited

Loading

jsoriano Mar 21, 2018 •

edited

Loading