[ovs-dev] [PATCH] ovs-bugtool: Change log-days parameter based on file last_mod_time

[Shih-Hao Li]

Thanks for the review. Please see inlines. 

Shih-Hao 

----- Original Message -----

> From: "Gurucharan Shetty" <shettyg at nicira.com>
> To: "Shih-Hao Li" <shihli at nicira.com>
> Cc: "dev" <dev at openvswitch.org>, "Shih-Hao Li" <shihli at vmware.com>
> Sent: Friday, September 27, 2013 10:15:36 AM
> Subject: Re: [ovs-dev] [PATCH] ovs-bugtool: Change log-days parameter based
> on file last_mod_time

> On Thu, Sep 26, 2013 at 1:25 PM, Shih-Hao Li <shihli at nicira.com> wrote:
> > From: Shih-Hao Li <shihli at vmware.com>
> >
> > Previously the log-days parameter can only support rotated logs
> > based on their numbered filename extension. Thus it can not be
> > used for other types of log filenames. This patch changes it to
> > be based on file last modification time.
> >
> > Issue: #19671
> You need a Signed-off-by line here.
I will add this. 

> > ---
> > utilities/bugtool/ovs-bugtool.8.in | 6 ++--
> > utilities/bugtool/ovs-bugtool.in | 67 +++++++++++++++++++++++++-----------
> > 2 files changed, 49 insertions(+), 24 deletions(-)
> >
> > diff --git a/utilities/bugtool/ovs-bugtool.8.in
> > b/utilities/bugtool/ovs-bugtool.8.in
> > index e353604..6f4e0b5 100644
> > --- a/utilities/bugtool/ovs-bugtool.8.in
> > +++ b/utilities/bugtool/ovs-bugtool.8.in
> > @@ -33,9 +33,9 @@ Print verbose debugging output.
> > Use the capabilities specified in a comma-separated list.
> > .
> > .IP "\fB\-\-log\-days=\fIdays\fR"
> > -Include the logs rotated in the previous \fIdays\fR days in the debug
> > bundle.
> > -The number of log files included has a big impact on the eventual bundle
> > size.
> > -The default value is 20 days.
> > +Include the logs with last modification time in the previous \fIdays\fR
> > days
> > +in the debug bundle. The number of log files included has a big impact on
> > the
> > +eventual bundle size. The default value is 20 days.
> > .
> > .IP "\fB\-\-output=\fIfiletype\fR"
> > Generate a debug bundle with the specified file type. Options include
> > diff --git a/utilities/bugtool/ovs-bugtool.in
> > b/utilities/bugtool/ovs-bugtool.in
> > index baa0af7..b2172c8 100755
> > --- a/utilities/bugtool/ovs-bugtool.in
> > +++ b/utilities/bugtool/ovs-bugtool.in
> > @@ -222,6 +222,7 @@ unlimited_data = False
> > dbg = False
> > # Default value for the number of rotated logs.
> > log_days = 20
> > +log_min_time = None
> > free_disk_space = None
> >
> > def cap(key, pii=PII_MAYBE, min_size=-1, max_size=-1, min_time=-1,
> > @@ -286,7 +287,8 @@ def cmd_output(cap, args, label=None, filter=None,
> > binary=False):
> > 'binary': binary}
> >
> >
> > -def file_output(cap, path_list, newest_first=False):
> > +def file_output(cap, path_list, newest_first=False, min_time=None,
> > + max_time=None):
> I see max_time as an option. We don't seem to be using it for any
> particular feature. If so, may be we can get rid of it.

I added it to be paired with max_time, like the min_time and max_time in capability. 
They are not used now. Just for potential use in the future. 

> > """
> > If newest_first is True, the list of files in path_list is sorted
> > by file modification time in descending order, else its sorted
> > @@ -299,6 +301,10 @@ def file_output(cap, path_list, newest_first=False):
> > s = os.stat(path)
> > except OSError, e:
> > continue
> > + if min_time is not None and s.st_mtime < min_time:
> > + continue
> > + if max_time is not None and s.st_mtime > max_time:
> > + continue
> > path_entries.append((path, s))
> >
> > mtime = lambda(path, stat): stat.st_mtime
> > @@ -308,7 +314,8 @@ def file_output(cap, path_list, newest_first=False):
> > data[p] = {'cap': cap, 'filename': p[0]}
> >
> >
> > -def tree_output(cap, path, pattern=None, negate=False,
> > newest_first=False):
> > +def tree_output(cap, path, pattern=None, negate=False, newest_first=False,
> > + min_time=None, max_time=None):
> > """
> > Walks the directory tree rooted at path. Files in current dir are processed
> > before files in sub-dirs.
> > @@ -318,23 +325,27 @@ def tree_output(cap, path, pattern=None,
> > negate=False, newest_first=False):
> > for root, dirs, files in os.walk(path):
> > fns = [fn for fn in [os.path.join(root, f) for f in files]
> > if os.path.isfile(fn) and matches(fn, pattern, negate)]
> > - file_output(cap, fns, newest_first=newest_first)
> > + file_output(cap, fns, newest_first=newest_first,
> > + min_time=min_time, max_time=max_time)
> > +
> > +
> > +def prefix_output(cap, prefix, newest_first=False, min_time=None,
> > max_time=None):
> > + """
> > + Output files with the same prefix.
> > + """
> > + fns = []
> > + for root, dirs, files in os.walk(os.path.dirname(prefix)):
> > + fns += [fn for fn in [os.path.join(root, f) for f in files]
> > + if fn.startswith(prefix)]
> > + file_output(cap, fns, newest_first=newest_first,
> > + min_time=min_time, max_time=max_time)
> > +
> >
> > def func_output(cap, label, func):
> > if cap in entries:
> > t = str(func).split()
> > data[label] = {'cap': cap, 'func': func}
> >
> > -def log_output(cap, logs, newest_first=False):
> > - global log_days
> > - file_output(cap, logs)
> > - file_output(cap,
> > - ['%s.%d' % (f, n) for n in range(1, log_days+1) for f in logs], \
> > - newest_first=newest_first)
> > - file_output(cap,
> > - ['%s.%d.gz' % (f, n) for n in range(1, log_days+1) for f in logs], \
> > - newest_first=newest_first)
> > -
> > def collect_data():
> > process_lists = {}
> >
> > @@ -370,7 +381,8 @@ def collect_data():
> >
> > def main(argv=None):
> > global ANSWER_YES_TO_ALL, SILENT_MODE
> > - global entries, data, dbg, unlimited_data, log_days, free_disk_space
> > + global entries, data, dbg, unlimited_data, free_disk_space
> > + global log_days, log_min_time
> >
> > # Filter flags
> > only_ovs_info = False
> > @@ -473,6 +485,8 @@ def main(argv=None):
> > if output_file is not None and not unlimited_data:
> > free_disk_space = get_free_disk_space(output_file) * 90 / 100
> >
> > + log_min_time = int(time.time()) - log_days * 86400
> > +
> > if ANSWER_YES_TO_ALL:
> > output("Warning: '--yestoall' argument provided, will not prompt for
> > individual files.")
> >
> > @@ -585,11 +599,14 @@ exclude those logs from the archive.
> > system_logs = ([ VAR_LOG_DIR + x for x in
> > ['crit.log', 'kern.log', 'daemon.log', 'user.log',
> > 'syslog', 'messages', 'secure', 'debug', 'dmesg', 'boot']])
> > + for log in system_logs:
> > + prefix_output(CAP_SYSTEM_LOGS, log, min_time=log_min_time)
> > +
> > ovs_logs = ([ OPENVSWITCH_LOG_DIR + x for x in
> > ['ovs-vswitchd.log', 'ovsdb-server.log',
> > 'ovs-xapi-sync.log', 'ovs-monitor-ipsec.log', 'ovs-ctl.log']])
> > - log_output(CAP_SYSTEM_LOGS, system_logs)
> > - log_output(CAP_OPENVSWITCH_LOGS, ovs_logs)
> > + for log in ovs_logs:
> > + prefix_output(CAP_OPENVSWITCH_LOGS, log, min_time=log_min_time)
> >
> > if not os.path.exists('/var/log/dmesg') and not
> > os.path.exists('/var/log/boot'):
> > cmd_output(CAP_SYSTEM_LOGS, [DMESG])
> > @@ -808,6 +825,7 @@ def dump_rdac_groups(cap):
> > cmd_output(cap, [MPPUTIL, '-g', group])
> >
> > def load_plugins(just_capabilities=False, filter=None):
> > + global log_min_time
> > def getText(nodelist):
> > rc = ""
> > for node in nodelist:
> > @@ -868,8 +886,9 @@ def load_plugins(just_capabilities=False, filter=None):
> > if el.tagName == "files":
> > newest_first = getBoolAttr(el, 'newest_first')
> > if el.getAttribute("type") == "logs":
> > - log_output(dir, getText(el.childNodes).split(),
> > - newest_first=newest_first)
> > + file_output(dir, getText(el.childNodes).split(),
> > + newest_first=newest_first,
> > + min_time=log_min_time)
> I think this should be prefix_output() instead of file_output().
> Otherwise the rotated logs won't be picked up.

Good catch. But I am not sure if users expect the filenames listed under <files> 
to be exact-match or not. My preference is to use "type=logs" only for those 
files or directories that need to be controlled by "--log-days" parameter. 
If users want to use wildcard for rotated logs, they can define that in "pattern" 
under <directory>, such as 
<directory pattern = "messages.*" > /var/log</directory> 
What do you think? 

> > else:
> > file_output(dir, getText(el.childNodes).split(),
> > newest_first=newest_first)
> > @@ -878,9 +897,15 @@ def load_plugins(just_capabilities=False,
> > filter=None):
> > if pattern == '': pattern = None
> > negate = getBoolAttr(el, 'negate')
> > newest_first = getBoolAttr(el, 'newest_first')
> > - tree_output(dir, getText(el.childNodes),
> > - pattern and re.compile(pattern) or None,
> > - negate=negate, newest_first=newest_first)
> > + if el.getAttribute("type") == "logs":
> > + tree_output(dir, getText(el.childNodes),
> > + pattern and re.compile(pattern) or None,
> > + negate=negate, newest_first=newest_first,
> > + min_time=log_min_time)
> > + else:
> > + tree_output(dir, getText(el.childNodes),
> > + pattern and re.compile(pattern) or None,
> > + negate=negate, newest_first=newest_first)
> > elif el.tagName == "command":
> > label = el.getAttribute("label")
> > if label == '': label = None
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130927/06fc3730/attachment-0003.html>