Bug 430377

Summary: Review Request: atop - An advanced interactive monitor to view the load on system and process level
Product: [Fedora] Fedora Reporter: Kairo Araujo <kairoaraujo>
Component: Package ReviewAssignee: manuel wolfshant <wolfy>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, gilboad, mtasaka, notting
Target Milestone: ---Flags: wolfy: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-04-17 18:58:02 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Attachments:
Description Flags
modified crondaily script which has some sane default in case the global config misses some required data none

Description Kairo Araujo 2008-01-26 22:55:48 EST
Spec URL: http://kairo.freeshell.org/devel/fedora/atop/atop.spec
SRPM URL: http://kairo.freeshell.org/devel/fedora/atop/atop-1.22-1.fc8.src.rpm
Description: 
An advanced interactive monitor for Linux-systems to view the load on
system-level and process-level.
The command atop has some major advantages compared to other
performance-monitors:
   - Resource consumption by all processes
   - Utilization of all relevant resources
   - Permanent logging of resource utilization
   - Highlight critical resources
   - Watch activity only
   - Watch deviations only
   - Accumulated process activity per user
   - Accumulated process activity per program
For more informations: http://www.atcomputing.nl/Tools/atop
Comment 1 manuel wolfshant 2008-01-27 04:20:48 EST
The package has an excellent start, but there are a few things that must be fixed
- according to the soruces, the correct license tag is GPLv2+
- in the init.d script, please replace _all_ references to ntpd with atop
- in your %build section, make does not activate parallel compiling 
- the default Makefile enforces some specific compiler flags, ignoring the
mandatory Fedora ones.
Using
   make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
solves both the above issues
- your %pre/%post scriptlets make use of chkconfig and service. Therefore your
Requires section must contain (see
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e
for details):
Requires(post): /sbin/chkconfig
Requires(preun): /sbin/chkconfig
Requires(preun): /sbin/service
- the name of the logrotate file is quite odd ("dummy..."). May I suggest to 
settle on a more meaningful one ?
- %description contains the following line "** package don't has resources the
patch:" 
It is neither correct English ( English grammar recommends "package doesn't have
<whatever else is in the phrase>) nor does it have a sense (the last noun is in
no way correlated to the first part of the sentence). As a non-native English
speaker, frankly I have absolutely no idea of what you want to communicate here
(OK, I lie, I have looked at upstream's site and I do have an idea). Could you
please rephrase the whole sentence (for instance "The package does not make use
of the patches available at http://www.atcomputing.nl/Tools/atop/kernpatch.html"
, if this is what you meant to say) ?
Comment 2 Kairo Araujo 2008-01-27 07:45:21 EST
New revision created with corrections. Thanks.

Spec URL: http://kairo.freeshell.org/devel/fedora/atop/atop.spec
SRPM URL: http://kairo.freeshell.org/devel/fedora/atop/atop-1.22-2.fc8.src.rpm
Comment 3 manuel wolfshant 2008-01-27 16:22:21 EST
There are also two typos in atop.crondaily: LOGPATHH is used instead of $LOGPATH
(in the "# verify if log file exist" ) section. There is also a small English
error, you should have said "verify if the file exists" (note the ending "s").

Everything else seems fine. BTW, at least on Centos 5 (which has ncurses-5.5 )
it builds / runs fine so I think that the requires/build-requires ncurses>=5.6
is excessive

I am willing to sponsor you, but I would like to see some more input from you.
Please follow http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
in order to get sponsored.
Comment 4 Kairo Araujo 2008-01-27 17:32:11 EST
New revision created. 
- removed indication of minimum version of the requires/build-requires
- corrected the comment line

Spec URL: http://kairo.freeshell.org/devel/fedora/atop/atop.spec
SRPM URL: http://kairo.freeshell.org/devel/fedora/atop/atop-1.22-3.fc8.src.rpm

I've another review request #429715
I am reading these documents: http://fedoraproject.org/wiki/PackageMaintainers
Thanks for support

Comment 5 manuel wolfshant 2008-01-30 03:15:29 EST
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on:devel/x86_64 and F7/x86_64
 [x] Rpmlint output:
source RPM:empty
binary RPM:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:GPLv2+
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     SHA1SUM of package: 8a120508a21b70881a0d6b46e5fd2d2452e40e7b 
/tmp/atop-1.22.tar.gz
 [x] Package is not known to require ExclusiveArch
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
       
=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64& F7/x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:only x86_64
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [x] File based requires are sane.


=== Issues ===
1. non critical: there exists an unneeded Requires for mcurses


================
*** APPROVED *** pending sponsorhip
================

Comment 6 Mamoru TASAKA 2008-03-24 10:52:49 EDT
*** Bug 438575 has been marked as a duplicate of this bug. ***
Comment 7 Mamoru TASAKA 2008-03-24 10:56:14 EDT
Are you still waiting for your sponsor? If so, I will recheck your
package as one of the sponsor members.
Comment 8 manuel wolfshant 2008-03-24 11:04:56 EDT
I am waiting to see some reviews performed by Kairo. I will sponsor him after that.
Comment 9 Kairo Araujo 2008-03-24 11:36:56 EDT
I will make some reviews today.
Thanks.
Comment 10 manuel wolfshant 2008-03-26 03:19:57 EDT
Kairo, please go ahead, I will sponsor you.
Comment 12 manuel wolfshant 2008-03-27 15:23:14 EDT
There are a couple of small errors which need fixing 
a) in atop.spec: please do not start atop in %post. Fedora policy mandates to
leave services stopped. It's up to the user to decide if the service should be
started
b) cron.daily should exit after checking if atop is started and finding out that
it is not. Your script checks if atop is running, takes a last sample if it is
and procedes with starting atop even if it is was not running
I suggest:
PID=`cat $PIDFILE`
if [ -e $PIDFILE ] && ps -p $PID | grep 'atop$' > /dev/null
then
        kill -USR1 $PID       # take final sample
        sleep 3
        kill -TERM $PID
        rm $PIDFILE
        sleep 1
else
        exit 1
fi

 A nice touch would be to use /etc/sysconfig/atop for storing all variables
which now are defined at the start of atop.init and atop.crondaily (CURDAY,
INTERVAL, PATHes etc) and source /etc/sysconfig/atop where ever it is needed.

b) in atop.init: 
- you cannot use daemon to start a cron script, it's purpose is to daemonize a
program
- there is a typo error, CURDAY lacks the "C"
Comment 13 Kairo Araujo 2008-03-28 13:08:13 EDT
New Review
http://kairo.freeshell.org/devel/fedora/atop/atop-1.23-2.fc8.src.rpm
http://kairo.freeshell.org/devel/fedora/atop/atop.spec

Answers for comment #12:
a) OK
b) OK
c) OK - atop.d created for init. (If note correct, please suggest.)  

[OK] rpmlimp spec 
[OK] rpmlimp rpm
[OK] rpmlimp srpm
[OK] rpmlimp rpm debuinfo
Comment 14 manuel wolfshant 2008-03-28 17:56:30 EDT
First of all, I must take back the c) from comment #12. I was wrong, launching
scripts with "daemon" is perfectly valid. I am sorry for misleading you.

There are still a couple of small problems:
a) minimal importance: all the install lines include the option "-p" twice.
Please make sure you delete one of them before uploading the files to CVS
b) I suggest to replace the "start atop for all processes with interval of 10
minutes" with the more accurate "start atop for all processes with interval of
$INTERVAL (by default 10) minutes". I have no objections if you want to keep it
as it is, it's a just a cosmetic issue.
c) could you please explain the purpose of the section from cron.daily which
verifies if atop.log exists and creates it if it does not ? As far as I have
understood, the process goes like this (please correct me if I am wrong):
- the startup script launches atop as a daemon, writes raw information in a
/var/log/atop/atop_$currentdate file and redirects stdout and stderr to
/var/log/atop/atop.log (which gets either created or overwritten with this occasion)
- the daily cron job verifies if atop is running and 1) if it is running, forces
a flush of the current state, kills the daemon and restarts it with a new raw
log 2) exits without doing anything if atop is not running 3) deletes old logs.
  If I am correct
- I fail to see the need for checking at the start of the script if the logfile
exists. It gets created anyway when the daemon is restarted and forcing an
advanced re-creation of it is useless in the event that it was deleted while the
daemon was running.
- deleting old logs is usually a job for logrotate. Admins might wish to keep
logs for a longer time and deleting their logs is not a nice thing to do. On the
other hand, since each raw log file has a different filename, logrotate cannot
be used to rotate them. Therefore I will not object if you want to preserve the
"find ... -exec rm {}" line. However I suggest the following:
1) create two sections in the logrotate file you supply:
- existing one should be used for atop_* files only (which this way would be
archived daily)
- one with a longer lifetime ( rotate 4 + weekly) for atop.log
2) make a note somewhere (for instance a separate readme.fedora or a couple of
lines in sysconfig/atop ) that mentions that by default raw logs are deleted
after 28 days. You could even add another variable in sysconfig/atop and use it
in the find line in cron.daily, for instance:
        #the cron.daily job deletes raw data after 28 days
        DATALIFE=28
and
# delete logfiles older than four weeks
#
(sleep 3; find $LOGPATH -name 'atop_*' -mtime +$DATALIFE -exec rm {} \; )&

Comment 15 Kairo Araujo 2008-03-29 15:05:06 EDT
Problems solved.
Thank you for helping to correct these details.
I have learnt a lot.

http://kairo.freeshell.org/devel/fedora/atop/atop.spec
http://kairo.freeshell.org/devel/fedora/atop/atop-1.23-3.fc8.src.rpm

[OK] rpmlint spec 
[OK] rpmlint rpm
[OK] rpmlint srpm
[OK] rpmlint rpm debuinfo
Comment 16 manuel wolfshant 2008-03-30 20:50:48 EDT
Your script which starts atop has a slight error in the logic.
Please find below the result of an aborted start, I'll let you identify the
error yourself. If you cannot identify it, I'll be glad to pinpoint it:

[wolfy@wolfy atop]$ sudo rpm -i
/var/lib/mock//fedora-7-x86_64/result/atop-1.23-3.fc7.x86_64.rpm
[wolfy@wolfy atop]$ sudo touch /var/run/atop.pid
[wolfy@wolfy atop]$ /etc/init.d/atop start
Starting atop: ERROR: List of process IDs must follow -p.
********* simple selection *********  ********* selection by list *********
-A all processes                      -C by command name
-N negate selection                   -G by real group ID (supports names)
-a all w/ tty except session leaders  -U by real user ID (supports names)
-d all except session leaders         -g by session OR by effective group name
-e all processes                      -p by process ID
T  all processes on this terminal     -s processes in the sessions given
a  all w/ tty, including other users  -t by tty
g  OBSOLETE -- DO NOT USE             -u by effective user ID (supports names)
r  only running processes             U  processes for specified users
x  processes w/o controlling ttys     t  by tty
*********** output format **********  *********** long options ***********
-o,o user-defined  -f full            --Group --User --pid --cols --ppid
-j,j job control   s  signal          --group --user --sid --rows --info
-O,O preloaded -o  v  virtual memory  --cumulative --format --deselect
-l,l long          u  user-oriented   --sort --tty --forest --version
-F   extra full    X  registers       --heading --no-heading --context
                    ********* misc options *********
-V,V  show version      L  list format codes  f  ASCII art forest
-m,m,-L,-T,H  threads   S  children in sum    -y change -l format
-M,Z  security data     c  true command name  -c scheduling class
-w,w  wide output       n  numeric WCHAN,UID  -H process hierarchy
                                                           [FAILED]

Comment 17 manuel wolfshant 2008-03-30 21:03:43 EDT
  One more suggestion, since we have to go through one more iteration any way:
for the case when a "smart user" accidentally deletes /etc/sysconfig/atop, a
safety net would be to include some defaults for the global variables in the
scripts which source the config file. I've attached a slightly modified
crondaily script which you might use if you find my idea acceptable. A similar
idea could be used in the script which starts atop
Comment 18 manuel wolfshant 2008-03-30 21:05:22 EDT
Created attachment 299654 [details]
modified crondaily script which has some sane default in case the global config misses some required data
Comment 19 Kairo Araujo 2008-03-31 11:01:39 EDT
Manuel, thanks for suggestion.
About error in the logic. We can check if pid file exists and has a size greater
than zero.
Example:

if [ -f $PIDFILE ]; then
        PID=`cat $PIDFILE`
        if [ -s $PIDFILE ] && ps -p $PID | grep 'atop$' > /dev/null
        then
                kill -USR1 $PID       
                sleep 3
                kill -TERM $PID
                rm $PIDFILE
                sleep 1
        else
                exit 1
        fi
fi

It's correct? 
	
I thought also to make:
1.) check if file pid exists 
   1.a) if yes: check pid is correct
      1.a.a) if yes: exit 1
      1.a.b) if not: remove pid file and start atop
   1.b) if not: start atop

Comment 20 manuel wolfshant 2008-03-31 13:43:12 EDT
I'd say this approach is excellent. Please go ahead.
If you have the time, maybe you can work a common script to be used both by the
startup script and by the cron.daily one, they are almost identical. Maybe a
function sourced from both places? Up to you if you want to do it and how, it's
just a "nice to have". 
Comment 21 Kairo Araujo 2008-03-31 20:03:38 EDT
http://kairo.freeshell.org/devel/fedora/atop/atop.spec
http://kairo.freeshell.org/devel/fedora/atop/atop-1.23-4.fc8.src.rpm

[OK] rpmlint spec 
[OK] rpmlint rpm
[OK] rpmlint srpm
[OK] rpmlint rpm debuinfo
[OK] mock: Done(rpmbuild/SRPMS/atop-1.23-4.fc8.src.rpm) Config(fedora-devel-i386)
Comment 22 Kairo Araujo 2008-04-02 12:41:26 EDT
Spec URL: http://kairo.fedorapeople.org/atop/atop.spec
SRPM URL: http://kairo.fedorapeople.org/atop/atop-1.23-5.fc8.src.rpm

- improved atop.d and atop.crondaily (Manuel Wolfshant)

[OK] rpmlint spec 
[OK] rpmlint rpm
[OK] rpmlint srpm
[OK] rpmlint rpm debuinfo
[OK] mock: Done(rpmbuild/SRPMS/atop-1.23-5.fc8.src.rpm) Config(fedora-devel-i386)

Tests with cron.daily and daemon:

[kairo@f8dev01eav ~]$ sudo su -
[root@f8dev01eav ~]# service atop status
atop is stopped
[root@f8dev01eav ~]# /etc/cron.daily/atop
[root@f8dev01eav ~]# service atop status
atop is stopped
[root@f8dev01eav ~]# service atop start
Starting atop:                                             [  OK  ]
[root@f8dev01eav ~]# service atop status
atop (pid 22212) is running...
[root@f8dev01eav ~]# /etc/cron.daily/atop
[root@f8dev01eav ~]# service atop status
atop (pid 22242) is running...
Comment 23 manuel wolfshant 2008-04-02 17:00:43 EDT
Well done, Kairo. Please remove the references to DATALIFE (this variable is no
longer used anywhere in your scripts, as far as I can see) and go ahead with CVS.
Comment 24 manuel wolfshant 2008-04-02 17:37:36 EDT
Kairo, if you do not mind, please include branches for EL-4 and EL-5 in your CVS
request. I'll be glad to co-maintain with you (or maintain) them if you do not
have the time or interest to take care of them.
Comment 25 Kairo Araujo 2008-04-03 09:22:44 EDT
Manuel,
I've interest in maintain this package on EL. I work with EL and this packages
is very good for troubleshooting.
Thanks again.

Spec URL: http://kairo.fedorapeople.org/atop/atop.spec
SRPM URL: http://kairo.fedorapeople.org/atop/atop-1.23-6.fc8.src.rpm

- removed variable DATALIFE from atop.d and atop.crondaily

[OK] rpmlint spec 
[OK] rpmlint rpm
[OK] rpmlint srpm
[OK] rpmlint rpm debuinfo
[OK] mock: fedora-devel-i386 epel-4-i386 epel-5-i386

Requesting CVS
Comment 26 Kairo Araujo 2008-04-03 09:23:47 EDT
New Package CVS Request
=======================
Package Name: atop
Short Description: An advanced interactive monitor to view the load on system
and process level
Owners: kairo
Branches: F-8 EL-4 EL-5
InitialCC:
Cvsextras Commits: yes
Comment 27 Kevin Fenzi 2008-04-03 16:13:01 EDT
cvs done.
Comment 28 manuel wolfshant 2008-04-17 18:58:02 EDT
closing the bug, the package is in rawhide