* [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records
2020-03-31 20:00 [PATCH 0/3] tools/kvm_stat: add logfile support Stefan Raspl
@ 2020-03-31 20:00 ` Stefan Raspl
2020-03-31 21:45 ` Paolo Bonzini
2020-03-31 20:00 ` [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file Stefan Raspl
2020-03-31 20:00 ` [PATCH 3/3] tools/kvm_stat: add sample systemd unit file Stefan Raspl
2 siblings, 1 reply; 10+ messages in thread
From: Stefan Raspl @ 2020-03-31 20:00 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: Stefan Raspl <raspl@de.ibm.com>
When running in logging mode, skip records with all zeros (=empty records)
to preserve space when logging to files.
Signed-off-by: Stefan Raspl <raspl@linux.ibm.com>
---
tools/kvm/kvm_stat/kvm_stat | 47 +++++++++++++++++++++++++--------
tools/kvm/kvm_stat/kvm_stat.txt | 4 +++
2 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 7fe767bd2625..54000ac508f9 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1488,7 +1488,8 @@ def batch(stats):
class StdFormat(object):
- def __init__(self, keys):
+ def __init__(self, keys, skip_zero_records):
+ self._skip_zero_records = skip_zero_records
self._banner = ''
for key in keys:
self._banner += key.split(' ')[0] + ' '
@@ -1496,16 +1497,21 @@ class StdFormat(object):
def get_banner(self):
return self._banner
- @staticmethod
- def get_statline(keys, s):
+ def get_statline(self, keys, s):
res = ''
+ non_zero = False
for key in keys:
+ if s[key].delta != 0:
+ non_zero = True
res += ' %9d' % s[key].delta
+ if self._skip_zero_records and not non_zero:
+ return ''
return res
class CSVFormat(object):
- def __init__(self, keys):
+ def __init__(self, keys, skip_zero_records):
+ self._skip_zero_records = skip_zero_records
self._banner = 'timestamp'
self._banner += reduce(lambda res, key: "{},{!s}".format(res,
key.split(' ')[0]), keys, '')
@@ -1513,8 +1519,14 @@ class CSVFormat(object):
def get_banner(self):
return self._banner
- @staticmethod
- def get_statline(keys, s):
+ def get_statline(self, keys, s):
+ if self._skip_zero_records:
+ non_zero = False
+ for key in keys:
+ if s[key].delta != 0:
+ non_zero = True
+ if self._skip_zero_records and not non_zero:
+ return ''
return reduce(lambda res, key: "{},{!s}".format(res, s[key].delta),
keys, '')
@@ -1523,14 +1535,20 @@ def log(stats, opts, frmt, keys):
"""Prints statistics as reiterating key block, multiple value blocks."""
line = 0
banner_repeat = 20
+ banner_printed = False
+
while True:
try:
time.sleep(opts.set_delay)
- if line % banner_repeat == 0:
+ if line % banner_repeat == 0 and not banner_printed:
print(frmt.get_banner())
- print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") +
- frmt.get_statline(keys, stats.get()))
+ banner_printed = True
+ statline = frmt.get_statline(keys, stats.get())
+ if len(statline) == 0:
+ continue
+ print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
line += 1
+ banner_printed = False
except KeyboardInterrupt:
break
@@ -1651,9 +1669,16 @@ Press any other key to refresh statistics immediately.
default=False,
help='retrieve statistics from tracepoints',
)
+ argparser.add_argument('-z', '--skip-zero-records',
+ action='store_true',
+ default=False,
+ help='omit records with all zeros in logging mode',
+ )
options = argparser.parse_args()
if options.csv and not options.log:
sys.exit('Error: Option -c/--csv requires -l/--log')
+ if options.skip_zero_records and not options.log:
+ sys.exit('Error: Option -z/--skip-zero-records requires -l/--log')
try:
# verify that we were passed a valid regex up front
re.compile(options.fields)
@@ -1736,9 +1761,9 @@ def main():
if options.log:
keys = sorted(stats.get().keys())
if options.csv:
- frmt = CSVFormat(keys)
+ frmt = CSVFormat(keys, options.skip_zero_records)
else:
- frmt = StdFormat(keys)
+ frmt = StdFormat(keys, options.skip_zero_records)
log(stats, options, frmt, keys)
elif not options.once:
with Tui(stats, options) as tui:
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index a97ded2aedad..24296dccc00a 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -104,6 +104,10 @@ OPTIONS
--tracepoints::
retrieve statistics from tracepoints
+*z*::
+--skip-zero-records::
+ omit records with all zeros in logging mode
+
SEE ALSO
--------
'perf'(1), 'trace-cmd'(1)
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records
2020-03-31 20:00 ` [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records Stefan Raspl
@ 2020-03-31 21:45 ` Paolo Bonzini
2020-04-01 8:19 ` Stefan Raspl
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-31 21:45 UTC (permalink / raw)
To: Stefan Raspl, kvm
On 31/03/20 22:00, Stefan Raspl wrote:
> @@ -1523,14 +1535,20 @@ def log(stats, opts, frmt, keys):
> """Prints statistics as reiterating key block, multiple value blocks."""
> line = 0
> banner_repeat = 20
> + banner_printed = False
> +
> while True:
> try:
> time.sleep(opts.set_delay)
> - if line % banner_repeat == 0:
> + if line % banner_repeat == 0 and not banner_printed:
> print(frmt.get_banner())
> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") +
> - frmt.get_statline(keys, stats.get()))
> + banner_printed = True
Can't skip_zero_records be handled here instead?
values = stats.get()
if not opts.skip_zero_records or \
any((values[k].delta != 0 for k in keys):
statline = frmt.get_statline(keys, values)
print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
Paolo
> + print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
> line += 1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records
2020-03-31 21:45 ` Paolo Bonzini
@ 2020-04-01 8:19 ` Stefan Raspl
2020-04-01 13:23 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Raspl @ 2020-04-01 8:19 UTC (permalink / raw)
To: Paolo Bonzini, kvm
On 2020-03-31 23:45, Paolo Bonzini wrote:
> On 31/03/20 22:00, Stefan Raspl wrote:
>> @@ -1523,14 +1535,20 @@ def log(stats, opts, frmt, keys):
>> """Prints statistics as reiterating key block, multiple value blocks."""
>> line = 0
>> banner_repeat = 20
>> + banner_printed = False
>> +
>> while True:
>> try:
>> time.sleep(opts.set_delay)
>> - if line % banner_repeat == 0:
>> + if line % banner_repeat == 0 and not banner_printed:
>> print(frmt.get_banner())
>> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") +
>> - frmt.get_statline(keys, stats.get()))
>> + banner_printed = True
>
> Can't skip_zero_records be handled here instead?
>
> values = stats.get()
> if not opts.skip_zero_records or \
> any((values[k].delta != 0 for k in keys):
> statline = frmt.get_statline(keys, values)
> print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
I wanted to avoid such an extra check for performance reasons. Granted, I have
to do something likewise (i.e. checking for non-zero values) in my patch for csv
records, but at least for the standard format things are a bit less costly
(avoiding an extra pass over the data).
Ciao,
Stefan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records
2020-04-01 8:19 ` Stefan Raspl
@ 2020-04-01 13:23 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-04-01 13:23 UTC (permalink / raw)
To: Stefan Raspl, kvm
On 01/04/20 10:19, Stefan Raspl wrote:
> On 2020-03-31 23:45, Paolo Bonzini wrote:
>> On 31/03/20 22:00, Stefan Raspl wrote:
>>> @@ -1523,14 +1535,20 @@ def log(stats, opts, frmt, keys):
>>> """Prints statistics as reiterating key block, multiple value blocks."""
>>> line = 0
>>> banner_repeat = 20
>>> + banner_printed = False
>>> +
>>> while True:
>>> try:
>>> time.sleep(opts.set_delay)
>>> - if line % banner_repeat == 0:
>>> + if line % banner_repeat == 0 and not banner_printed:
>>> print(frmt.get_banner())
>>> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") +
>>> - frmt.get_statline(keys, stats.get()))
>>> + banner_printed = True
>>
>> Can't skip_zero_records be handled here instead?
>>
>> values = stats.get()
>> if not opts.skip_zero_records or \
>> any((values[k].delta != 0 for k in keys):
>> statline = frmt.get_statline(keys, values)
>> print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
>
> I wanted to avoid such an extra check for performance reasons. Granted, I have
> to do something likewise (i.e. checking for non-zero values) in my patch for csv
> records, but at least for the standard format things are a bit less costly
> (avoiding an extra pass over the data).
I don't think it's a perceivable difference, and it simplifies the code
noticeably.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file
2020-03-31 20:00 [PATCH 0/3] tools/kvm_stat: add logfile support Stefan Raspl
2020-03-31 20:00 ` [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records Stefan Raspl
@ 2020-03-31 20:00 ` Stefan Raspl
2020-03-31 21:02 ` Paolo Bonzini
2020-03-31 20:00 ` [PATCH 3/3] tools/kvm_stat: add sample systemd unit file Stefan Raspl
2 siblings, 1 reply; 10+ messages in thread
From: Stefan Raspl @ 2020-03-31 20:00 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: Stefan Raspl <raspl@de.ibm.com>
To integrate with logrotate, we have a signal handler that will re-open
the logfile.
Assuming we have a systemd unit file with
ExecStart=kvm_stat -dtc -s 10 -L /var/log/kvm_stat.csv
ExecReload=/bin/kill -HUP $MAINPID
and a logrotate config featuring
postrotate
/bin/systemctl restart kvm_stat.service
endscript
Then the overall flow will look like this:
(1) systemd starts kvm_stat, logging to A.
(2) At some point, logrotate runs, moving A to B.
kvm_stat continues to write to B at this point.
(3) After rotating, logrotate restarts the kvm_stat unit via systemctl.
(4) The kvm_stat unit sends a SIGHUP to kvm_stat, finally making it
switch over to writing to A again.
Note that in order to keep the structure of the cvs output in tact, we
make sure to, in contrast to the standard log format, only write the
header once at the beginning of a file. This implies that the header is
suppressed when appending to an existing file. Unlike the standard
format, where we append to an existing file by starting out with a
header.
Signed-off-by: Stefan Raspl <raspl@linux.ibm.com>
---
tools/kvm/kvm_stat/kvm_stat | 75 +++++++++++++++++++++++++++++----
tools/kvm/kvm_stat/kvm_stat.txt | 5 +++
2 files changed, 71 insertions(+), 9 deletions(-)
diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 54000ac508f9..e4f67f0629ee 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -32,6 +32,7 @@ import resource
import struct
import re
import subprocess
+import signal
from collections import defaultdict, namedtuple
from functools import reduce
from datetime import datetime
@@ -228,6 +229,8 @@ IOCTL_NUMBERS = {
'RESET': 0x00002403,
}
+signal_received = False
+
ENCODING = locale.getpreferredencoding(False)
TRACE_FILTER = re.compile(r'^[^\(]*$')
@@ -1533,25 +1536,73 @@ class CSVFormat(object):
def log(stats, opts, frmt, keys):
"""Prints statistics as reiterating key block, multiple value blocks."""
+ global signal_received
line = 0
banner_repeat = 20
- banner_printed = False
+ f = None
+
+ def do_banner(opts):
+ nonlocal f
+ if opts.log_to_file:
+ if not f:
+ try:
+ f = open(opts.log_to_file, 'a')
+ except (IOError, OSError):
+ sys.exit("Error: Could not open file: %s" %
+ opts.log_to_file)
+ if isinstance(frmt, CSVFormat) and f.tell() != 0:
+ return
+ f.write(frmt.get_banner())
+ f.write('\n')
+ else:
+ print(frmt.get_banner())
+
+ def do_statline(opts):
+ statline = frmt.get_statline(keys, stats.get())
+ if len(statline) == 0:
+ return False
+ statline = datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline
+ if opts.log_to_file:
+ f.write(statline)
+ f.write('\n')
+ else:
+ print(statline)
+ return True
+
+ do_banner(opts)
+ banner_printed = True
while True:
try:
time.sleep(opts.set_delay)
- if line % banner_repeat == 0 and not banner_printed:
- print(frmt.get_banner())
+ if signal_received:
+ banner_printed = True
+ line = 0
+ f.close()
+ do_banner(opts)
+ signal_received = False
+ if (line % banner_repeat == 0 and not banner_printed and
+ not (opts.log_to_file and isinstance(frmt, CSVFormat))):
+ do_banner(opts)
banner_printed = True
- statline = frmt.get_statline(keys, stats.get())
- if len(statline) == 0:
+ if not do_statline(opts):
continue
- print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
line += 1
banner_printed = False
except KeyboardInterrupt:
break
+ if opts.log_to_file:
+ f.close()
+
+
+def handle_signal(sig, frame):
+ global signal_received
+
+ signal_received = True
+
+ return
+
def is_delay_valid(delay):
"""Verify delay is in valid value range."""
@@ -1652,6 +1703,10 @@ Press any other key to refresh statistics immediately.
default=False,
help='run in logging mode (like vmstat)',
)
+ argparser.add_argument('-L', '--log-to-file',
+ type=str,
+ help="like '--log', but logging to a file"
+ )
argparser.add_argument('-p', '--pid',
type=int,
default=0,
@@ -1675,9 +1730,9 @@ Press any other key to refresh statistics immediately.
help='omit records with all zeros in logging mode',
)
options = argparser.parse_args()
- if options.csv and not options.log:
+ if options.csv and not (options.log or options.log_to_file):
sys.exit('Error: Option -c/--csv requires -l/--log')
- if options.skip_zero_records and not options.log:
+ if options.skip_zero_records and not (options.log or options.log_to_file):
sys.exit('Error: Option -z/--skip-zero-records requires -l/--log')
try:
# verify that we were passed a valid regex up front
@@ -1758,7 +1813,9 @@ def main():
sys.stdout.write(' ' + '\n '.join(sorted(set(event_list))) + '\n')
sys.exit(0)
- if options.log:
+ if options.log or options.log_to_file:
+ if options.log_to_file:
+ signal.signal(signal.SIGHUP, handle_signal)
keys = sorted(stats.get().keys())
if options.csv:
frmt = CSVFormat(keys, options.skip_zero_records)
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index 24296dccc00a..de7c4a2497f9 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -92,6 +92,11 @@ OPTIONS
--log::
run in logging mode (like vmstat)
+
+-L::
+--log-to-file::
+ like '--log', but logging to a file
+
-p<pid>::
--pid=<pid>::
limit statistics to one virtual machine (pid)
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file
2020-03-31 20:00 ` [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file Stefan Raspl
@ 2020-03-31 21:02 ` Paolo Bonzini
2020-04-01 12:59 ` Stefan Raspl
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-31 21:02 UTC (permalink / raw)
To: Stefan Raspl, kvm
On 31/03/20 22:00, Stefan Raspl wrote:
> From: Stefan Raspl <raspl@de.ibm.com>
>
> To integrate with logrotate, we have a signal handler that will re-open
> the logfile.
> Assuming we have a systemd unit file with
> ExecStart=kvm_stat -dtc -s 10 -L /var/log/kvm_stat.csv
> ExecReload=/bin/kill -HUP $MAINPID
> and a logrotate config featuring
> postrotate
> /bin/systemctl restart kvm_stat.service
Does reload work too?
Regarding the code, I only have a few nits.
> + f.write(frmt.get_banner())
> + f.write('\n')
> + else:
> + print(frmt.get_banner())
What about
print(frmt.get_banner(), file=f or sys.stdout)
> +
> + def do_statline(opts):
> + statline = frmt.get_statline(keys, stats.get())
> + if len(statline) == 0:
> + return False
> + statline = datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline
> + if opts.log_to_file:
> + f.write(statline)
> + f.write('\n')
> + else:
> + print(statline)
... and likewise here? (
>
> + return True
> +
> + do_banner(opts)
> + banner_printed = True
> while True:
> try:
> time.sleep(opts.set_delay)
> - if line % banner_repeat == 0 and not banner_printed:
> - print(frmt.get_banner())
> + if signal_received:
> + banner_printed = True
> + line = 0
> + f.close()
> + do_banner(opts)
> + signal_received = False
> + if (line % banner_repeat == 0 and not banner_printed and
> + not (opts.log_to_file and isinstance(frmt, CSVFormat))):
> + do_banner(opts)
> banner_printed = True
> - statline = frmt.get_statline(keys, stats.get())
> - if len(statline) == 0:
> + if not do_statline(opts):
> continue
> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
> line += 1
> banner_printed = False
> except KeyboardInterrupt:
> break
>
> + if opts.log_to_file:
> + f.close()
"if f:"?
> +
> +
> +def handle_signal(sig, frame):
> + global signal_received
> +
> + signal_received = True
> +
> + return
> +
>
> def is_delay_valid(delay):
> """Verify delay is in valid value range."""
> @@ -1652,6 +1703,10 @@ Press any other key to refresh statistics immediately.
> default=False,
> help='run in logging mode (like vmstat)',
> )
> + argparser.add_argument('-L', '--log-to-file',
> + type=str,
> + help="like '--log', but logging to a file"
> + )
> argparser.add_argument('-p', '--pid',
> type=int,
> default=0,
> @@ -1675,9 +1730,9 @@ Press any other key to refresh statistics immediately.
> help='omit records with all zeros in logging mode',
> )
> options = argparser.parse_args()
> - if options.csv and not options.log:
> + if options.csv and not (options.log or options.log_to_file):
> sys.exit('Error: Option -c/--csv requires -l/--log')
"requires -l/-L"?
> - if options.skip_zero_records and not options.log:
> + if options.skip_zero_records and not (options.log or options.log_to_file):
> sys.exit('Error: Option -z/--skip-zero-records requires -l/--log')
Likewise.
> try:
> # verify that we were passed a valid regex up front
> @@ -1758,7 +1813,9 @@ def main():
> sys.stdout.write(' ' + '\n '.join(sorted(set(event_list))) + '\n')
> sys.exit(0)
>
> - if options.log:
> + if options.log or options.log_to_file:
> + if options.log_to_file:
> + signal.signal(signal.SIGHUP, handle_signal)
> keys = sorted(stats.get().keys())
> if options.csv:
> frmt = CSVFormat(keys, options.skip_zero_records)
> diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
> index 24296dccc00a..de7c4a2497f9 100644
> --- a/tools/kvm/kvm_stat/kvm_stat.txt
> +++ b/tools/kvm/kvm_stat/kvm_stat.txt
> @@ -92,6 +92,11 @@ OPTIONS
> --log::
> run in logging mode (like vmstat)
>
> +
> +-L::
> +--log-to-file::
> + like '--log', but logging to a file
-L<file>:: / --log-to-file=<file>
> +
> -p<pid>::
> --pid=<pid>::
> limit statistics to one virtual machine (pid)
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file
2020-03-31 21:02 ` Paolo Bonzini
@ 2020-04-01 12:59 ` Stefan Raspl
2020-04-01 13:24 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Raspl @ 2020-04-01 12:59 UTC (permalink / raw)
To: Paolo Bonzini, kvm
On 2020-03-31 23:02, Paolo Bonzini wrote:
> On 31/03/20 22:00, Stefan Raspl wrote:
>> From: Stefan Raspl <raspl@de.ibm.com>
>>
>> To integrate with logrotate, we have a signal handler that will re-open
>> the logfile.
>> Assuming we have a systemd unit file with
>> ExecStart=kvm_stat -dtc -s 10 -L /var/log/kvm_stat.csv
>> ExecReload=/bin/kill -HUP $MAINPID
>> and a logrotate config featuring
>> postrotate
>> /bin/systemctl restart kvm_stat.service
>
> Does reload work too?
Reload and restart work fine - any specific concerns or areas to look for?
> Regarding the code, I only have a few nits.
>
>
>> + f.write(frmt.get_banner())
>> + f.write('\n')
>> + else:
>> + print(frmt.get_banner())
>
> What about
>
> print(frmt.get_banner(), file=f or sys.stdout)
Yup, thx!
>> +
>> + def do_statline(opts):
>> + statline = frmt.get_statline(keys, stats.get())
>> + if len(statline) == 0:
>> + return False
>> + statline = datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline
>> + if opts.log_to_file:
>> + f.write(statline)
>> + f.write('\n')
>> + else:
>> + print(statline)
>
> ... and likewise here? (
Sure
>>
>> + return True
>> +
>> + do_banner(opts)
>> + banner_printed = True
>> while True:
>> try:
>> time.sleep(opts.set_delay)
>> - if line % banner_repeat == 0 and not banner_printed:
>> - print(frmt.get_banner())
>> + if signal_received:
>> + banner_printed = True
>> + line = 0
>> + f.close()
>> + do_banner(opts)
>> + signal_received = False
>> + if (line % banner_repeat == 0 and not banner_printed and
>> + not (opts.log_to_file and isinstance(frmt, CSVFormat))):
>> + do_banner(opts)
>> banner_printed = True
>> - statline = frmt.get_statline(keys, stats.get())
>> - if len(statline) == 0:
>> + if not do_statline(opts):
>> continue
>> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
>> line += 1
>> banner_printed = False
>> except KeyboardInterrupt:
>> break
>>
>> + if opts.log_to_file:
>> + f.close()
>
> "if f:"?
I'd argue the former is a lot more telling/easier to read - one of the downsides
of using extremely short variable names like 'f'.
>> +
>> +
>> +def handle_signal(sig, frame):
>> + global signal_received
>> +
>> + signal_received = True
>> +
>> + return
>> +
>>
>> def is_delay_valid(delay):
>> """Verify delay is in valid value range."""
>> @@ -1652,6 +1703,10 @@ Press any other key to refresh statistics immediately.
>> default=False,
>> help='run in logging mode (like vmstat)',
>> )
>> + argparser.add_argument('-L', '--log-to-file',
>> + type=str,
>> + help="like '--log', but logging to a file"
>> + )
>> argparser.add_argument('-p', '--pid',
>> type=int,
>> default=0,
>> @@ -1675,9 +1730,9 @@ Press any other key to refresh statistics immediately.
>> help='omit records with all zeros in logging mode',
>> )
>> options = argparser.parse_args()
>> - if options.csv and not options.log:
>> + if options.csv and not (options.log or options.log_to_file):
>> sys.exit('Error: Option -c/--csv requires -l/--log')
>
> "requires -l/-L"?
Yes!
>> - if options.skip_zero_records and not options.log:
>> + if options.skip_zero_records and not (options.log or options.log_to_file):
>> sys.exit('Error: Option -z/--skip-zero-records requires -l/--log')
>
> Likewise.
Of course.
>> try:
>> # verify that we were passed a valid regex up front
>> @@ -1758,7 +1813,9 @@ def main():
>> sys.stdout.write(' ' + '\n '.join(sorted(set(event_list))) + '\n')
>> sys.exit(0)
>>
>> - if options.log:
>> + if options.log or options.log_to_file:
>> + if options.log_to_file:
>> + signal.signal(signal.SIGHUP, handle_signal)
>> keys = sorted(stats.get().keys())
>> if options.csv:
>> frmt = CSVFormat(keys, options.skip_zero_records)
>> diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
>> index 24296dccc00a..de7c4a2497f9 100644
>> --- a/tools/kvm/kvm_stat/kvm_stat.txt
>> +++ b/tools/kvm/kvm_stat/kvm_stat.txt
>> @@ -92,6 +92,11 @@ OPTIONS
>> --log::
>> run in logging mode (like vmstat)
>>
>> +
>> +-L::
>> +--log-to-file::
>> + like '--log', but logging to a file
>
> -L<file>:: / --log-to-file=<file>
Argh...
Ciao,
Stefan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file
2020-04-01 12:59 ` Stefan Raspl
@ 2020-04-01 13:24 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-04-01 13:24 UTC (permalink / raw)
To: Stefan Raspl, kvm
On 01/04/20 14:59, Stefan Raspl wrote:
> On 2020-03-31 23:02, Paolo Bonzini wrote:
>> On 31/03/20 22:00, Stefan Raspl wrote:
>>> From: Stefan Raspl <raspl@de.ibm.com>
>>>
>>> To integrate with logrotate, we have a signal handler that will re-open
>>> the logfile.
>>> Assuming we have a systemd unit file with
>>> ExecStart=kvm_stat -dtc -s 10 -L /var/log/kvm_stat.csv
>>> ExecReload=/bin/kill -HUP $MAINPID
>>> and a logrotate config featuring
>>> postrotate
>>> /bin/systemctl restart kvm_stat.service
>>
>> Does reload work too?
>
> Reload and restart work fine - any specific concerns or areas to look for?
No, I would just put reload in the postrotate script.
>>> + if opts.log_to_file:
>>> + f.close()
>>
>> "if f:"?
>
> I'd argue the former is a lot more telling/easier to read - one of the downsides
> of using extremely short variable names like 'f'.
Fair enough.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] tools/kvm_stat: add sample systemd unit file
2020-03-31 20:00 [PATCH 0/3] tools/kvm_stat: add logfile support Stefan Raspl
2020-03-31 20:00 ` [PATCH 1/3] tools/kvm_stat: add command line switch '-z' to skip zero records Stefan Raspl
2020-03-31 20:00 ` [PATCH 2/3] tools/kvm_stat: Add command line switch '-L' to log to file Stefan Raspl
@ 2020-03-31 20:00 ` Stefan Raspl
2 siblings, 0 replies; 10+ messages in thread
From: Stefan Raspl @ 2020-03-31 20:00 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: Stefan Raspl <raspl@de.ibm.com>
Add a sample unit file as a basis for systemd integration of kvm_stat
logs.
Signed-off-by: Stefan Raspl <raspl@linux.ibm.com>
---
tools/kvm/kvm_stat/kvm_stat.service | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 tools/kvm/kvm_stat/kvm_stat.service
diff --git a/tools/kvm/kvm_stat/kvm_stat.service b/tools/kvm/kvm_stat/kvm_stat.service
new file mode 100644
index 000000000000..71aabaffe779
--- /dev/null
+++ b/tools/kvm/kvm_stat/kvm_stat.service
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+[Unit]
+Description=Service that logs KVM kernel module trace events
+Before=qemu-kvm.service
+
+[Service]
+Type=simple
+ExecStart=/usr/bin/kvm_stat -dtcz -s 10 -L /var/log/kvm_stat.csv
+ExecReload=/bin/kill -HUP $MAINPID
+Restart=always
+SyslogIdentifier=kvm_stat
+SyslogLevel=debug
+
+[Install]
+WantedBy=multi-user.target
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread