scottchiefbaker / dool

Python3 compatible fork of dstat
GNU General Public License v3.0
332 stars 64 forks source link

kvm: show VM name in the `most-exensive` column #47

Closed KJ7LNW closed 1 year ago

KJ7LNW commented 1 year ago
SUMMARY

Return the VM name in calls to getnamebypid

ISSUE TYPE

About 10 years ago I wrote a simple patch for Dag's dstat to show the virtual machine name in the most-expensive process column. We only ever used this internally.

I have not yet looked at dool code to see if this patch is even compatible, because first I want to ask if this is something that would be of interest.

If so:

--- a/usr/bin/dstat 2009-11-24 01:30:11.000000000 -0800
+++ b/usr/bin/dstat 2014-11-07 10:20:09.719148833 -0800
@@ -1946,6 +1946,12 @@
         return os.path.basename(name)
     return name

+def index_containing_substring(the_list, substring):
+   for i, s in enumerate(the_list):
+       if substring in s:
+           return i
+   return -1
+
 def getnamebypid(pid, name):
     ret = None
     try:
@@ -1956,6 +1962,10 @@
         if ret.startswith('-'):
             ret = basename(cmdline[-2])
             if ret.startswith('-'): raise
+        if any("qemu" in s for s in cmdline):
+            idx = index_containing_substring(cmdline, '-name')
+            if idx >= 0:
+                ret = cmdline[idx+1]
         if not ret: raise
     except:
         ret = basename(name)
OS / ENVIRONMENT

Linux

scottchiefbaker commented 1 year ago

Yes I would be interested in this. First make sure the code ports over to Dool (should be simple). Once we have proof of concept running on Dool we can figure out where to place it. I'm thinking a plug-in?

KJ7LNW commented 1 year ago

Here is a working patch.

How would you fold it in as a plugin so it gets used by options like --top-bio and --top-cpu?

diff --git a/dool b/dool
index bfd97cf..7f8b661 100755
--- a/dool
+++ b/dool
@@ -2106,6 +2106,12 @@ def basename(name):
         return os.path.basename(name)
     return name

+def index_containing_substring(the_list, substring):
+    for i, s in enumerate(the_list):
+        if substring in s:
+            return i
+    return -1
+
 def getnamebypid(pid, name):
     "Return the name of a process by taking best guesses and exclusion"
     ret = None
@@ -2123,6 +2129,14 @@ def getnamebypid(pid, name):
         if ret.startswith('-'):
             ret = basename(cmdline[-2])
             if ret.startswith('-'): raise
+        if "qemu" in cmdline[0]:
+            idx = index_containing_substring(cmdline, '-name')
+            if idx >= 0:
+                # Match "foo" from "guest=foo,otherstuff=bar", or match "foo"
+                # for older libvirts that do not have var=val formatted
+                # content.
+                m = re.match("(?:^[^=]+=)?([^,]+)", cmdline[idx+1])
+                ret = m.group(1)
         if not ret: raise
     except:
         ret = basename(name)
scottchiefbaker commented 1 year ago

What hypervisor are you running that this matches against? I'm running Proxmox and I see qemu processes like so:

root       52293 58.1  1.9 12817400 10557092 ?   Sl   Apr12 122227:05 /usr/bin/kvm -id 213 -name TestDHCPServer,debug-threads=on -no-shutdown...
KJ7LNW commented 1 year ago

Interesting. We are using vanilla libvirt+KVM on Oracle Linux 9. It looks like this:

/usr/libexec/qemu-kvm -name guest=test.ewheeler.net,debug-threads=on ...

Maybe this should be the regex:

>>> print(re.match("^(?:guest=)?([^,=]+)", "foo,bar=baz").group(1))
foo
>>> print(re.match("^(?:guest=)?([^,=]+)", "guest=foo,bar=baz").group(1))
foo
>>> print(re.match("^(?:guest=)?([^,=]+)", "foo").group(1))
foo
scottchiefbaker commented 1 year ago

Checkout the code I'm working on in #48. It's a slightly modified version of what you had.

KJ7LNW commented 1 year ago

That looks great! Should self.width be configurable?

scottchiefbaker commented 1 year ago

@KJ7LNW I'm not sure the best approach to the width. I toyed around with the idea of calculating the longest process name and using that. That doesn't work if your process name is ls but your longest process is 40 chars long. You'll end up with a lot of extra whitespace. I'm inclined to just leave it hardcoded at 20 for now until someone can come up with a better idea.

Does the code in #48 capture the name for you? If it works for you I'll land the PR.

scottchiefbaker commented 1 year ago

I went ahead and landed the code in #48. Give it a test and if it doesn't work let me know.

I'm prepping for a new release by the end of the work.