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 Review | Assignee: | manuel wolfshant <manuel.wolfshant> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | fedora-package-review, gilboad, mtasaka, notting | ||||
Target Milestone: | --- | Flags: | manuel.wolfshant:
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 22:58:02 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Attachments: |
|
Description
Kairo Araujo
2008-01-27 03:55:48 UTC
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) ? 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 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. 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 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 ================ *** Bug 438575 has been marked as a duplicate of this bug. *** Are you still waiting for your sponsor? If so, I will recheck your package as one of the sponsor members. I am waiting to see some reviews performed by Kairo. I will sponsor him after that. I will make some reviews today. Thanks. Kairo, please go ahead, I will sponsor you. - Update to 1.23 - bug on atop.init corrected http://kairo.freeshell.org/devel/fedora/atop/atop.spec http://kairo.freeshell.org/devel/fedora/atop/atop-1.23-1.fc8.src.rpm 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" 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 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 {} \; )& 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 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] 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 Created attachment 299654 [details]
modified crondaily script which has some sane default in case the global config misses some required data
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 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". 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) 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... 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. 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. 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 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 cvs done. closing the bug, the package is in rawhide |