Bug 459281 - Review Request: corosync - The Corosync Cluster Engine
Review Request: corosync - The Corosync Cluster Engine
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity urgent
: ---
: ---
Assigned To: Chris Feist
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-15 12:52 EDT by Steven Dake
Modified: 2016-04-26 13:41 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-18 17:42:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
cfeist: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Steven Dake 2008-08-15 12:52:40 EDT
Spec URL: http://developer.osdl.org/dev/openais/downloads/review/corosync.spec
SRPM URL: http://developer.osdl.org/dev/openais/downlodas/review/corosync-0.91.src.rpm

Description: 
The Corosync Cluster Engine is a "user space kernel" for clustering services.  It provides the basis of communication, membership, and other services used in clustering environments.  This project was created as a division of the openais project (currently in Fedora) to better align communities and developers around each of the separate projects.  The openais package also now depends on corosync.  Also, the remaining Fedora 10 cluster infrastructure depends upon corosync being merged into Fedora 10.  With this merging, the latest cluster3 trees including gfs/gfs2 can be merged into Fedora 10.

If you would like more details of corosync, an Ottawa Linux Symposium 2008 Paper is available:

http://ols.fedoraproject.org/OLS/Reprints-2008/dake-reprint.pdf

Thank you for spending the time to review this package for inclusion.
Comment 1 Steven Dake 2008-08-15 12:55:02 EDT
The actual SRPM URL is:
http://developer.osdl.org/dev/openais/downloads/review/corosync-0.91-1.src.rpm
Comment 2 Jason Tibbitts 2008-08-15 20:38:03 EDT
Any particular reason this requires a fixed uid?

Why call %{_initrddir}/corosync directly instead of using /sbin/service corosync?

Also note that if you're targeting F10 and later, you can use _initddir.  I'm not sure anyone knows where that extra 'r' came from.

What is a ".lcrso" file, and why do they need to be under %{_libexecdir}?  Have you investigated how this works with multilib?

You shouldn't need to explicitly mark anything under %{_docdir} as %doc.  In fact, you don't usually install doc files in %doc, but just list them as %doc in the %files list and they'll be copied there.  I.e. you can just do
  %doc LICENSE SECURITY

Are you sure the libraries should be in the -devel package?  The unversioned .so files should go there, definitely, but the actual libraries should generally be in the main package.  Otherwise, in order to run something built against them you have to install the -devel package, which is rarely correct. 

Why is corosync-objctl installed in the main package while its manpage is in the -devel package?  I didn't read the other manpages, but if you're sure that they're of use only to developers then they're fine in the -devel package.

You could probably make your %files lists very much smaller if you didn't list each file separately.  The manpages, though, I guess you can't do much about.
You could have %files lists like these, but maybe you just prefer to list everything out:

%files
%defattr(-,root,root,-)
%doc LICENSE SECURITY
%{_sbindir}/corosync*
%config(noreplace) /etc/corosync.conf
%config(noreplace) /etc/ld.so.conf.d/corosync-*.conf
%{_initrddir}/corosync
%{_libexecdir}/lcrso
%dir %{_libdir}/corosync/
%{_libdir}/corosync/*.so.*
%{_mandir}/man8/corosync-objctl.8*
%{_mandir}/man8/corosync_overview.8*
%{_mandir}/man8/logsys_overview.8*
%{_mandir}/man5/corosync.conf.5*

%files devel
%defattr(-,root,root,-)
%doc README.devmap
%{_includedir}/corosync
%{_libdir}/corosync/*.so
%{_mandir}/man3/*
%{_mandir}/man8/cpg_overview.8*
%{_mandir}/man8/evs_overview.8*
%{_mandir}/man8/confdb_overview.8*
Comment 3 Steven Dake 2008-08-18 14:41:58 EDT
Jason,

Thanks for your review.

To ansewr your questions:
1) the user id is needed for IPC security.  That user id was previously part of openais and has been moved into this package instead.  IPC authenticates all users using a UID equal to ais (or root of course).

2) i will change the initrddir issue you mentioned.  This spec file was taken from the original openais spec file from fedora 6 ages ago.

3) A lcrso file is a plugin with a specific binary format.  It will only be loaded by the corosync binary for which the architecture is built.  Therefore multilib is not an issue, since we wont have a 64 bit corosync and 32 bit plugins.  The plugins are required to match the binary bit size of the corosync binary.

4) Thanks for the tips on doc I will fix that.

5) The corosync main package doesn't use any library in the -devel package.  The devel package shared objects are libraries for use by third party developers writing software.  Just so we are clear, here is an example, and you can correct me if I misunderstand the packaging guidelines.

User wants to write an application using the cpg service.  They install corosync and corosync-devel.  corosync-devel provides the API header files and library objects for usage by the application's cpg utilization.

At no time does anything in the main corosync package reference any shared object in the devel package.  The -devel libraries are only for third party developers.

6) man page in wrong location - thanks for the catch.

7) I really like to list everything out so that rpmbuild catches maintainer (my) errors in packaging by telling me about a missing file or unpackaged files.  If this will be an issue for review I can trim the list however.

---

I will make a new RPM with the following changes:
a) fix the initrdir
b) fix the service start
c) move the corosync-ctl man page to the proper package

If you feel more actions are required please let me know.

And thank you for your time reviewing the package.

Regards
-steve
Comment 4 Steven Dake 2008-08-20 15:15:34 EDT
New package available for review (corosync-0.91-2):

http://developer.osdl.org/dev/openais/downloads/review/corosync-0.91-1.src.rpm
http://developer.osdl.org/dev/openais/downloads/review/corosync.spec

The changelog is:
* Wed Aug 20 2008 Steven Dake <sdake@redhat.com> - 0.91-2
- use /sbin/service instead of calling init script directly.
- put corosync-objctl man page in the main package.
- change all initrddir to initddir for fedora 10 guidelines.

Thank you for taking the time to review this package.

Regards
-steve
Comment 5 Steven Dake 2008-08-20 15:16:08 EDT
New package available for review (corosync-0.91-2):

http://developer.osdl.org/dev/openais/downloads/review/corosync-0.91-2.src.rpm
http://developer.osdl.org/dev/openais/downloads/review/corosync.spec

The changelog is:
* Wed Aug 20 2008 Steven Dake <sdake@redhat.com> - 0.91-2
- use /sbin/service instead of calling init script directly.
- put corosync-objctl man page in the main package.
- change all initrddir to initddir for fedora 10 guidelines.

Thank you for taking the time to review this package.

Regards
-steve
Comment 6 Fabio Massimo Di Nitto 2008-08-25 04:30:43 EDT
Hi Tom,

i am not an official sponsor, but the spec file looks good to me.

corosync is now a major (Build)Require for cluster. Is there any way we can speed this up?

Thanks
Fabio
Comment 7 Steven Dake 2008-08-25 05:04:53 EDT
Based on Fabio's feedback:

New package available for review (corosync-0.91-3):

http://developer.osdl.org/dev/openais/downloads/review/corosync-0.91-3.src.rpm
http://developer.osdl.org/dev/openais/downloads/review/corosync.spec

* Sun Aug 24 2008 Steven Dake <sdake@redhat.com> - 0.91-3
- move logsys_overview.8.* to devel package.
- move shared libs to main package.
Comment 8 Chris Feist 2008-09-16 15:25:12 EDT
I've verified that Jason's requests have made it into the 0.91-3 src rpm.  And the .spec file looks good to me with no rpmlint output.  I'm marking this packages as approved.
Comment 9 Steven Dake 2008-09-24 02:37:34 EDT
New Package CVS Request
=======================
Package Name: corosync
Short Description: The Corosync Cluster Engine
Owners: sdake
Branches: F-10 Rawhide
InitialCC: sdake
Comment 10 Huzaifa S. Sidhpurwala 2008-09-24 03:56:04 EDT
(In reply to comment #9)
> New Package CVS Request
> =======================
> Package Name: corosync
> Short Description: The Corosync Cluster Engine
> Owners: sdake
> Branches: F-10 Rawhide
there is no F-10 Rawhide branch, the rawhide branch is called "devel" do you want me to go ahead with this?

> InitialCC: sdake
Comment 11 Steven Dake 2008-09-24 04:02:44 EDT
yes please.  I apologize for the confusion.  I didn't relize the trunk was called "devel".

Thanks
-steve
Comment 12 Huzaifa S. Sidhpurwala 2008-09-24 04:23:57 EDT
cvs is done
Please check if everything is ok.
Comment 13 Allisson Azevedo 2008-12-22 06:32:45 EST
Hi Steve,

if builds fine in koji, close this bug as NEXTRELEASE.
Comment 14 Steven Dake 2009-03-18 17:42:25 EDT
merged into rawhide.

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