Bug 552331 - Review Request: piranha - Tools for administration of Linux Virtual Server
Summary: Review Request: piranha - Tools for administration of Linux Virtual Server
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Milos Jakubicek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-04 18:02 UTC by Marek Grac
Modified: 2010-02-17 11:30 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2010-02-17 11:30:51 UTC
Type: ---
Embargoed:
xjakub: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Marek Grac 2010-01-04 18:02:32 UTC
Spec URL: http://marx.fedorapeople.org/piranha.spec
SRPM URL: http://marx.fedorapeople.org/piranha-0.8.4-15.el5.src.rpm

GIT URL: http://git.fedoraproject.org/git/piranha.git
Description: 

Various tools to administer and configure the Linux Virtual Server as well as
heartbeating and failover components.  The LVS is a dynamically adjusted
kernel routing mechanism that provides load balancing primarily for web
and ftp servers though other services are supported.

Comment 1 Marek Grac 2010-01-04 18:04:09 UTC
Scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1901427

Comment 2 Milos Jakubicek 2010-01-04 19:15:15 UTC
So:

* first, please make rpmlint happy:

>rpmlint piranha.spec                                                                             
piranha.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 80, tab: line 85)  

>rpmlint ../RPMS/x86_64/piranha-*.rpm
piranha-debuginfo.x86_64: W: no-url-tag
piranha.x86_64: W: no-url-tag

(add URL tag)

piranha.x86_64: W: dangling-symlink /etc/sysconfig/ha/modules /usr/lib64/httpd/modules

This seems to be ok, the target is provided by httpd which is required, but see my comment at the very end.

piranha.x86_64: E: non-standard-dir-perm /var/log/piranha 0775

Fix this please.

piranha.x86_64: W: dangling-symlink /usr/sbin/piranha_gui /usr/sbin/httpd

This is ok, as previously, though the "_gui" suffix isn't really motivating for being linked to apache.

piranha.x86_64: E: zero-length /var/log/piranha/piranha-gui
piranha.x86_64: E: zero-length /etc/sysconfig/ha/lvs.cf

Zero-length files shouldn't be packaged, either provide some default configuration (or at least explanation what users should use them for!) in them or remove them. 

piranha.x86_64: E: incoherent-logrotate-file /etc/logrotate.d/piranha-httpd

Your logrotate file should be named /etc/logrotate.d/<package name>.

piranha.x86_64: E: zero-length /var/log/piranha/piranha-gui-access

Likewise.

piranha.x86_64: E: non-readable /usr/sbin/piranha-passwd 0700
piranha.x86_64: E: non-standard-executable-perm /usr/sbin/piranha-passwd 0700

The file doesn't seem to contain passwords => should be 755.

piranha.x86_64: W: dangerous-command-in-%postun userdel

Users/groups must not be removed after package removal, refer to:
http://fedoraproject.org/wiki/Packaging:UsersAndGroups

btw, you can also use getent to check whether a given user/group exists (see the examples on the above referenced page).

Don't forget to remove shadow-utils from Requires(postun) as well.

piranha.x86_64: W: no-reload-entry /etc/rc.d/init.d/piranha-gui
In your init script (/etc/rc.d/init.d/your_file), you don't have a 'reload'
entry, which is necessary for good functionality.

See https://fedoraproject.org/wiki/Packaging/SysVInitScript

* please refer to Source and Patch policy here:

http://fedoraproject.org/wiki/Packaging:SourceURL
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

* use macros in the %files section (https://fedoraproject.org/w/index.php?title=Packaging:RPMMacros) -- %{_sysconfdir}, %{_sbindir}, %{_initddir}, %{_initddir}, %{_localstatedir}

* there is quite a mess what concerns licensing -- some source files (pulse.c, linkstate.c, nanny.c) refer to GPLv2+ (not just GPLv2), most refer to unspecified version of GPL (fos.c, lvsconfig.c, main.c, lvsd.c, util.c). Two files have quite problematic licensing: ipvs_exec.c contains just Copyright 1999 Red Hat Inc., and send_arp.c contains funny licensing which however could be troublesome (there were some cases that packages didn't pass review because you couldn't use them to prepare nuclear war or something like that).

To sum up => Everywhere should be a licensed fully specified, I guess you wanted GPLv2+. Can you satisfy this condition for send_arp.c? If not, I'd ask spot what to do with this.

* directory layout: the package misuses /etc/sysconfig heavily. The apache configuration files should placed under /etc/httpd/conf.d, "web" and "logs" should be moved to /usr/share. What's the purpose of the "modules" symlink?
=> There shouldn't be anything besides the "pulse" in /etc/sysconfig.

Comment 3 Marek Grac 2010-01-08 16:52:27 UTC
New build in koji:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1909778

all errors removed, warnings with symlinks still remains.

Author of send_arp.c was contacted and he wrote that he already give permission to use it under BSD + GPL licenses. I will create a new upstream version with correct licensing + incorporated patches. Please let me know if there are any other problems with this package.

Comment 4 Milos Jakubicek 2010-01-09 14:28:34 UTC
- Please don't forget to bump the release number, otherwise it's just confusing!

- Remove shadow-utils from Requires(postun) as well (as I wrote before).

- Use %{_initddir} instead of %{_initrddir}, read: https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem

- Fix %config(noreplace)                                /etc/logrotate.d/piranha
  => %config(noreplace) %{_sysconfdir}/logrotate.d/piranha

- patches documentation still missing

- /etc/sysconfig misuse still unhandled

- Remove the unnecessary Requires: popt (this will be added automatically by rpm)

- Looking forward to see the new sources solving the licensing issues...

Comment 5 Marek Grac 2010-01-14 19:00:58 UTC
SPEC URL: http://marx.fedorapeople.org/piranha.spec
SRPM URL: http://marx.fedorapeople.org/piranha-0.8.5-1.fc13.src.rpm

Most of the problems should be fixed now - only /etc/sysconfig should remain. Please approve that rest is okay :)

Comment 6 Marek Grac 2010-01-14 19:01:38 UTC
KOJI: koji.fedoraproject.org/koji/taskinfo?taskID=1921593

Comment 7 Milos Jakubicek 2010-01-16 12:52:19 UTC
I'm afraid there are still things to be fixed besides /etc/sysconfig:

1) you removed "shadow-utils" from Requires(pre) -- why? I explicitly wrote you should remove them from Requires(postun) -- because we don't delete user/group after package removal (as I wrote before), but you still need it to create them, so the Requires(pre): shadow-utils must be preserved.

It would be indeed also most robust to use getent to check whether the given user/group already exists -- you can just copy over the example from here:

http://fedoraproject.org/wiki/Packaging:UsersAndGroups

2) what I didn't mention in the second, only in the first post: please indeed read the http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control page and accompany the specfile with instructions which make creating your sources tarball reproducible (see the example on the page).

3)

>rpmlint ../RPMS/x86_64/piranha-0.8.5-1.fc12.x86_64.rpm
piranha.x86_64: W: incoherent-version-in-changelog 0.8.5-1.fc13 ['0.8.5-1.fc12', '0.8.5-1']

=> whatever format in comments you use, please be consistent, the specfile changelog alters the style every here and now. Note that there are only 3 possible changelog formats you can use (and this is a MUST since they are parsed automatically by tools like packagekit):

http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

4)

Licensing: the licese of send_arp.c is ok now, but none of the rest has been resolved, please read again my first post which described in detail what the problem is.

Comment 8 Marek Grac 2010-01-19 11:13:35 UTC
KOJI: http://koji.fedoraproject.org/koji/taskinfo?taskID=1931381
SRPM: http://marx.fedorapeople.org/piranha-0.8.6-2.fc13.src.rpm
SPEC: http://marx.fedorapeople.org/piranha.spec

rpmlint - only dangling links warning

license for ipvs_exec added (originally part of nanny.c)

Comment 9 Milos Jakubicek 2010-01-21 02:34:25 UTC
The %files section needs improvements after last changes:

- do *not* own the /etc/sysconfig directory (it's owned by the filesystem package), i.e. remove: %dir %{_sysconfdir}/sysconfig

- use %{_datadir} instead of %{_prefix}/share

- you must own %{_datadir}/piranha!

=> it should be "%{_datadir}/piranha/" instead of "%{_prefix}/share/piranha/web"


+ don't see the license for ipvs_exec -- ?

Comment 11 Marek Grac 2010-02-01 13:55:31 UTC
KOJI: http://koji.fedoraproject.org/koji/taskinfo?taskID=1956177

rpmlint on rpm - 2 warnings = dangling links

Comment 12 Milos Jakubicek 2010-02-02 16:01:54 UTC
New version of rpmlint reveals some typos:

>rpmlint ../SRPMS/piranha-0.8.6-3.fc12.src.rpm
piranha.src: W: spelling-error Summary(en_US) administation -> administration, administrative, administrator
rpiranha.src: W: spelling-error %description -l en_US heartbeating -> heart beating, heart-beating, heartbeat
ppiranha.src: W: spelling-error %description -l en_US failover -> fail over, fail-over, failure

(Please definitely fix at least the summary -- that's a clear error.)

More importantly:

1) updating the package results into:

varování: provedení %postun(piranha-0.8.6-2.fc12.x86_64) skripletu selhalo, návratový kód: 6

because of wrong path to lvs.cf in the init script. But fixing it is not enough, it still fails:

>service pulse start
Starting pulse: pulse: failed to open /etc/sysconfig/ha/lvs.cf

2) the /etc/piranha/httpd.conf should be, like I mentioned, in /etc/httpd/conf.d, that's really the proper place for it. It's ok and you don't need to own the directory since you depend on httpd.

Comment 13 Marek Grac 2010-02-06 17:03:35 UTC
SRPM: http://marx.fedorapeople.org/piranha-0.8.6-4.fc13.src.rpm
SPEC: http://marx.fedorapeople.org/piranha.spec

administation - fixed; rest are valid words in HA/LVM context

1) fixed - postun (condrestart) works as expected

2) we use our own instance of Apache so we can't use conf.d/

Comment 14 Milos Jakubicek 2010-02-06 19:35:51 UTC
OK, all the issues are resolved. As discussed offline, after importing the package please file a bug against selinux-policy to make it working fine with piranha.

This package is APPROVED.

Comment 15 Marek Grac 2010-02-06 19:55:22 UTC
New Package CVS Request
=======================
Package Name: piranha
Short Description: Cluster administration tools
Owners: marx
Branches: F-12
InitialCC:

Comment 16 Kevin Fenzi 2010-02-07 21:24:16 UTC
CVS done (by process-cvs-requests.py).


Note You need to log in before you can comment on or make changes to this bug.