kvm plugins improvements #186

duritong · 2012-09-12T11:20:31Z

I added a few improvements to the kvm plugins:

Make most of them also work on RedHat based systems
Add an additional kvm_net_rh Plugin that works on RedHat bases systems. It is more or less a copy of the existing kvm_net plugin, which does not work and which needed to many changes which I'm not sure would have broken things further.

ssm · 2014-10-05T19:36:40Z

Closing and opening old pull request, to have it tested by travis-ci.

ssm · 2014-10-05T22:34:46Z

plugins/virtualization/kvm_net_rh

+    res = {}
+    macs_to_inf = find_macs_to_inf()
+    interfaces = {}
+    for e in Popen('cat /proc/net/dev | awk \'{ print $1  ":" $9 }\'', shell=True, stdout=PIPE).communicate()[0].split('\n'):


This line seems to contain a bug, which should be fixed before we can merge.

% cat /proc/net/dev | awk '{ print $1 ":" $9 }' Inter-|: face:multicast|bytes eth0::0 eth1::5246 virbr0::0 lo::0

The format on my box is:

% cat /proc/net/dev Inter-| Receive | Transmit face |bytes packets errs drop fifo frame compressed multicast|bytes packets errs drop fifo colls carrier compressed eth0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 eth1: 3833806033 3143411 0 0 0 0 0 5246 168429641 1939069 0 0 0 0 0 0 virbr0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 lo: 86071620 477326 0 0 0 0 0 0 86071620 477326 0 0 0 0 0 0

...which looks more human-readable than strictly parsable.

On a reasonably modern linux kernel, perhaps what you can find with "find /sys/class/net/*/statistics/" may be more useful.

sumpfralle · 2016-10-21T00:11:50Z

@duritong: are you still interested in this pull request? What do you think about the issues above?

On RedHat the binary that is used is called qemu-kvm not kvm.

duritong · 2016-12-01T10:17:12Z

So I reworked the whole stuff and I hope problems to be solved by now

sumpfralle

Thank you for working on these plugins!

I added some suggestions.

Regarding the red-hat-specific(?) plugin: what is the difference between this one and the general kvm_net plugin?
Maybe you could merge these into a single plugin handling both cases? Working with code copies is (as you already noticed with the other plugins) a burden, that should be avoided, if possible.

sumpfralle · 2016-12-04T15:16:08Z

plugins/virtualization/kvm_cpu

+    else:
+        kvm = Popen("which kvm", shell=True, stdout=PIPE)
+        kvm.communicate()
+        return not bool(kvm.returncode)


I would propose to phrase this clearer (instead of relying on the coincidence of the implicit interger/boolean conversion):
return kvm.returncode == 0

I didn't change the original behavior. Just added one that fits more for RHEL/CentOS.
In general this part is from my point of view problematic in 2 cases:

which lookups are expensive

we're not only looking at kvm.

I can change it in general, but as I said the intend of my PR was not to change the original behavior. But I can come up with general improvements, if people would like to see that.

sumpfralle · 2016-12-04T15:18:17Z

plugins/virtualization/kvm_cpu

 def list_pids():
    ''' Find the pid of kvm processes
    @return a list of pids from running kvm
    '''
-    pid = Popen("pidof qemu-system-x86_64", shell=True, stdout=PIPE)
+    pid = Popen("pidof qemu-kvm qemu-system-x86_64 kvm", shell=True, stdout=PIPE)


Does this list of names lack "qemu-system-i386"?
Other architectures are probably not available due to kvm being bound to intel/amd - right?

As mentioned above I did not change the original behavior. So I just kept qemu-system-x86_64 as it was the current behavior. I can add though other architectures as well.

sumpfralle · 2016-12-04T15:18:43Z

plugins/virtualization/kvm_io

+    else:
+        kvm = Popen("which kvm", shell=True, stdout=PIPE)
+        kvm.communicate()
+        return not bool(kvm.returncode)


same as above

sumpfralle · 2016-12-04T15:18:59Z

plugins/virtualization/kvm_io

 def list_pids():
    ''' Find the pid of kvm processes
    @return a list of pids from running kvm
    '''
-    pid = Popen("pidof qemu-system-x86_64", shell=True, stdout=PIPE)
+    pid = Popen("pidof qemu-kvm qemu-system-x86_64 kvm", shell=True, stdout=PIPE)


same as above

sumpfralle · 2016-12-04T15:19:14Z

plugins/virtualization/kvm_mem

+    else:
+        kvm = Popen("which kvm", shell=True, stdout=PIPE)
+        kvm.communicate()
+        return not bool(kvm.returncode)


same as above

sumpfralle · 2016-12-04T15:19:23Z

plugins/virtualization/kvm_mem

 def list_pids():
    ''' Find the pid of kvm processes
    @return a list of pids from running kvm
    '''
-    pid = Popen("pidof qemu-system-x86_64", shell=True, stdout=PIPE)
+    pid = Popen("pidof qemu-kvm qemu-system-x86_64 kvm", shell=True, stdout=PIPE)


same as above

duritong · 2016-12-06T18:55:06Z

Regarding the RH specific plugin: I have only rhel based kvm boxes and there were some significant differences, I wasn't able to test on debian based kvm boxes. So either someone needs to test it or we should keep it as seperate plugins.

I agree though, that a unified plugin would be better, but given I don't have access to debian based kvm boxes I can't really verify my changes.

sumpfralle · 2016-12-07T05:42:36Z

I would be happy, if you could change the bits necessary for your environment and also fix/improve more details along the way (in separate commits). Most of the plugins do not have a fixed maintainer and cannot be easily tested by the repository maintainers - thus it is usually highly welcome to combine fixes with general cleanups.

Regarding Debian/Red Hat: I have access to Debian systems with kvm and I am fluent in python. Thus I would be happy to test and fix (if necessary) the plugins, as soon as you included the Red Hat specific parts.

sumpfralle · 2017-01-05T04:41:05Z

@duritong: what do you think?

duritong · 2017-01-06T10:17:09Z

I'll try to work on it, december was a bit a busy month for me.

sumpfralle · 2017-01-27T22:39:20Z

@duritong: just a friendly (and patient) reminder ...

sumpfralle · 2018-03-09T03:10:35Z

@duritong: ping?

sumpfralle · 2019-07-21T22:05:58Z

Ping?

github-actions · 2021-04-17T02:23:03Z

Stale pull request message

ssm added the enhancement label May 21, 2014

ssm added the new plugin label Oct 5, 2014

ssm closed this Oct 5, 2014

ssm reopened this Oct 5, 2014

ssm reviewed Oct 5, 2014
View reviewed changes

ssm added the todo: needs more work label May 5, 2015

duritong added 2 commits December 1, 2016 11:14

add a kvm_net plugin that works on redhat based systems

bd28292

Make kvm plugins also work on RedHat based systems

f1bee82

On RedHat the binary that is used is called qemu-kvm not kvm.

duritong force-pushed the master branch from 9f08810 to f1bee82 Compare December 1, 2016 10:16

sumpfralle requested changes Dec 4, 2016

View reviewed changes

github-actions bot added the no-pr-activity label Apr 17, 2021

github-actions bot closed this Apr 25, 2021

steveschnepp reopened this Apr 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvm plugins improvements #186

kvm plugins improvements #186

duritong commented Sep 12, 2012

ssm commented Oct 5, 2014

ssm Oct 5, 2014

sumpfralle commented Oct 21, 2016

duritong commented Dec 1, 2016

sumpfralle left a comment

sumpfralle Dec 4, 2016

duritong Dec 6, 2016

sumpfralle Dec 4, 2016

duritong Dec 6, 2016

sumpfralle Dec 4, 2016

sumpfralle Dec 4, 2016

sumpfralle Dec 4, 2016

sumpfralle Dec 4, 2016

duritong commented Dec 6, 2016

sumpfralle commented Dec 7, 2016

sumpfralle commented Jan 5, 2017

duritong commented Jan 6, 2017

sumpfralle commented Jan 27, 2017

sumpfralle commented Mar 9, 2018

sumpfralle commented Jul 21, 2019

github-actions bot commented Apr 17, 2021

kvm plugins improvements #186

Are you sure you want to change the base?

kvm plugins improvements #186

Conversation

duritong commented Sep 12, 2012

ssm commented Oct 5, 2014

Choose a reason for hiding this comment

sumpfralle commented Oct 21, 2016

duritong commented Dec 1, 2016

sumpfralle 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

duritong commented Dec 6, 2016

sumpfralle commented Dec 7, 2016

sumpfralle commented Jan 5, 2017

duritong commented Jan 6, 2017

sumpfralle commented Jan 27, 2017

sumpfralle commented Mar 9, 2018

sumpfralle commented Jul 21, 2019

github-actions bot commented Apr 17, 2021