Bug 446390 - Review Request: cluster - RedHat Cluster Suite
Review Request: cluster - RedHat Cluster Suite
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
: 225646 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-14 09:12 EDT by Fabio Massimo Di Nitto
Modified: 2009-05-22 12:53 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-20 00:15:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabio Massimo Di Nitto 2008-05-14 09:12:55 EDT
Spec URL: http://bassotto.seabone.net/~fabbione/cluster.spec
SRPM URL: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-1.fc10.src.rpm
Description: RedHat Cluster Suite High Availability core infrastructure.

This package is built from the pre-release snapshots that will become 3.0.

It addresses several issues in the actual packages of the same stack. 
Most important ones:
1) the whole set of binary rpm's is built from the same source rather than N copies of the same
2) it makes security updates simpler (no need to update N packages for the same problem)
3) it allows to keep the whole stack in sync easily 
4) major rework of the several spec files have gone into this spec file as an attempt to adhere 100% to the packaging policies and all suitable changes have gone upstream rather as local patches/hacks.

With this package upstream is doing a very strong commitment to fedora to incorporate all required changes in the upstream releases rather than carry around tons of extra patches.

NOTE: This is my very first package and I am looking for a sponsor.

Thanks
Fabio

PS due to the nature of the software (that requires at least 2 machines for testing), in case the sponsor does not have access to enough equipment, I can provide access to hardware for testing.
Comment 1 Tom "spot" Callaway 2008-05-14 12:36:09 EDT
You shouldn't need BuildRequires: openais, the BR on openais-devel will pull
that in everytime. :)
Comment 2 Tom "spot" Callaway 2008-05-14 12:43:04 EDT
A few additional minor items:

%postun -n cman
/sbin/ldconfig > /dev/null

can be changed into

%postun -n cman -p /sbin/ldconfig

Please avoid using file requires, except where absolutely necessary. Instead of
Requires: net-tools mount /sbin/findfs /bin/bash
 use
Requires: net-tools mount e2fsprogs
(bash is already a Requires, two lines above)

Please show an updated SPEC/SRPM and I will finish the review. :)
Comment 3 Fabio Massimo Di Nitto 2008-05-14 13:32:09 EDT
(In reply to comment #1)
> You shouldn't need BuildRequires: openais, the BR on openais-devel will pull
> that in everytime. :)

true that.. done.
Comment 4 Fabio Massimo Di Nitto 2008-05-14 13:49:11 EDT
(In reply to comment #2)
> A few additional minor items:
> 
> %postun -n cman
> /sbin/ldconfig > /dev/null
> 
> can be changed into
> 
> %postun -n cman -p /sbin/ldconfig

I have done the postun section on your suggestion but i have a question:
does this apply also to %post? google is not helping me here to find references
to this format and I don't like to do blind changes (even when i know they are
coming from trusted sources ;))
What does -p mean?

> Please avoid using file requires, except where absolutely necessary. Instead of
> Requires: net-tools mount /sbin/findfs /bin/bash
>  use
> Requires: net-tools mount e2fsprogs
> (bash is already a Requires, two lines above)

done.

> 
> Please show an updated SPEC/SRPM and I will finish the review. :)

Spec URL: http://bassotto.seabone.net/~fabbione/cluster.spec
65f4fe55f70bb14a85d83c5232d8a0f7  cluster.spec (just in case there is proxy on
the way since the URL is the same)
SRPM URL: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-2.fc10.src.rpm

Comment 5 Tom "spot" Callaway 2008-05-14 14:55:14 EDT
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-d0dbcb7eec27622a21df280009c5b089b02f5bef
has some description of -p, but essentially, if you only have a single command
to execute in a %post/%postun scriptlet, you can call it with -p and it will
automatically resolve the dependencies for it.

Of course, this doesn't appear to be cleanly documented anywhere, but I assure
you, it works. :)
Comment 6 Fabio Massimo Di Nitto 2008-05-14 15:06:30 EDT
(In reply to comment #5)
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-d0dbcb7eec27622a21df280009c5b089b02f5bef
> has some description of -p, but essentially, if you only have a single command
> to execute in a %post/%postun scriptlet, you can call it with -p and it will
> automatically resolve the dependencies for it.
> 
> Of course, this doesn't appear to be cleanly documented anywhere, but I assure
> you, it works. :)

I absolutely believe you it does.. i am just being extra paranoid to build solid
references for the future.

Thanks
Fabio
Comment 7 Tom "spot" Callaway 2008-05-14 15:26:22 EDT
Since you're replacing existing packages, please be sure to coordinate with
their maintainers and properly End-Of-Life them, according to:
http://fedoraproject.org/wiki/PackageMaintainers/PackageEndOfLife

Now, for the review:

Good:

- rpmlint checks return:
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
gfs2-utils.x86_64: W: incoherent-init-script-name gfs2

The incoherent-init-script-name one is safe to ignore, but you should consider
whether all of those services should be enabled by default.

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2 and LGPLv2) OK, text in %doc, matches source

You should correct the license tag for the rgmanager subpackage, as it is
GPLv2+. Just add:

License: GPLv2+ into the rgmanager subpackage definition.

- spec file legible, in am. english
- source matches upstream (50ca482ccd4d2ee525a982c7cf08ac1100279251)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

Aside from the License tag and the initscripts, everything is fine. Show me a
fixed SRPM and I will approve and sponsor. Good work!
Comment 8 Fabio Massimo Di Nitto 2008-05-14 17:18:59 EDT
(In reply to comment #7)
> Since you're replacing existing packages, please be sure to coordinate with
> their maintainers and properly End-Of-Life them, according to:
> http://fedoraproject.org/wiki/PackageMaintainers/PackageEndOfLife

All the maintainers of the binary packages I am replacing are in CC in this bug
and we all agreed in advance on this step. I will make sure both Chris and
Steven will obsolete cman/gfs2-utils/rgmanager source packages for rawhide/F10.

Older branches will stay. Once i will be able to access them, they will receive
proper updates as well.

> - rpmlint checks return:
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
> rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
> rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
> gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
> gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2

So this is something i have been pondering myself for a few days now. All the
old .spec files and init.d files had services enabled by default, without
declaring dependencies etc.

These new ones are (in one way or another) all dependent on cman to start
properly and that happens only if cman is configured correctly (otherwise it's a
bug in the init scripts that should be addressed upstream _immediatly_).

cman is not autoconfigured from the packages since there is no such thing as
"cluster default config that will work everywhere" so basically having those
scripts enabled or disabled by default makes no difference.

enabling the services by default will (IMHO) avoid extra steps for the users
given the failsafe embedded in the scripts.

> gfs2-utils.x86_64: W: incoherent-init-script-name gfs2
> 
> The incoherent-init-script-name one is safe to ignore, but you should consider
> whether all of those services should be enabled by default.

Ok thanks. I did confirm this one also checking the recent gfs2-utils review.

> You should correct the license tag for the rgmanager subpackage, as it is
> GPLv2+. Just add:
> 
> License: GPLv2+ into the rgmanager subpackage definition.

Done. On the licensing side, we are working upstream to clean it up and also
provide a proper set of files to land in the %doc areas (usual
COPYING/COPYRIGHT/LICENCES shebang for the whole stack)

> Aside from the License tag and the initscripts, everything is fine. Show me a
> fixed SRPM and I will approve and sponsor.

Spec URL: http://bassotto.seabone.net/~fabbione/cluster.spec
dd1026852f948f97061f5573b27c5085  cluster.spec
SRPM URL: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-3.fc10.src.rpm

> Good work!

Thanks :)

Fabio
Comment 9 Fabio Massimo Di Nitto 2008-05-14 17:22:03 EDT
*** Bug 225646 has been marked as a duplicate of this bug. ***
Comment 10 Tom "spot" Callaway 2008-05-15 11:29:58 EDT
> These new ones are (in one way or another) all dependent on cman to start
> properly and that happens only if cman is configured correctly (otherwise 
> it's a bug in the init scripts that should be addressed upstream 
> _immediatly_).

But, since cman is not autoconfigured, does it really make sense for those
initscripts to be enabled by default? It seems like users configuring these
applications can also turn on these services. I'll leave that decision up to you.

The only other thing remaining is that there is a typo in the Summary for
rgmanager: Open Source HA Resource Group Failove for Red Hat Cluster

Should be Open Source HA Resource Group Failover for Red Hat Cluster

Package approved. :)
Comment 11 Fabio Massimo Di Nitto 2008-05-15 13:17:02 EDT
(In reply to comment #10)
> > These new ones are (in one way or another) all dependent on cman to start
> > properly and that happens only if cman is configured correctly (otherwise 
> > it's a bug in the init scripts that should be addressed upstream 
> > _immediatly_).
> 
> But, since cman is not autoconfigured, does it really make sense for those
> initscripts to be enabled by default? It seems like users configuring these
> applications can also turn on these services. I'll leave that decision up to you.

I guess it makes no real difference to me. I don't have strong opinions either
way. I will disable them in the next upstream release that I am planning to do
next monday.

> 
> The only other thing remaining is that there is a typo in the Summary for
> rgmanager: Open Source HA Resource Group Failove for Red Hat Cluster
> 
> Should be Open Source HA Resource Group Failover for Red Hat Cluster

Done.

SRPMS: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-4.fc10.src.rpm
SPEC: http://bassotto.seabone.net/~fabbione/cluster.spec
3da6d1bb600a1bff02442e652c2ac476  SPECS/cluster.spec

> Package approved. :)

great.

Thanks
Fabio
Comment 12 Fabio Massimo Di Nitto 2008-05-18 23:40:50 EDT
New Package CVS Request
=======================
Package Name: cluster
Short Description: Red Hat Cluster
Owners: fabbione swhiteho cfeist lon
Branches: F-10
InitialCC: fabbione
Cvsextras Commits: no

This package will be only in F-10 and higher. Not sure if that requires a branch
to be specified at all at this point in time.

Regarding cvsextra, since this is my first package, i would like to have full
control over it and relax my paranoia once i am more used to the Fedora way of
cross handling packages between maintainers. As upstream, we are also putting a
lot of effort to include distro patches in the main tree. I don't want CVS to
become a gateway between community patches and upstream nor a place where to
fight if a patch should be in or out. I also assume it is a flag its possible to
change in time..

Thanks
Fabio
Comment 13 Kevin Fenzi 2008-05-19 12:13:20 EDT
The 'devel' branch (which is default) will much later in this development cycle
be branched off to become the F-10 branch. 

You are of course free to change cvsextras commits later. It's quite rare for
anyone non closely associated with a particular package to change it however.
Allowing cvsextras commits does allow other maintainers to make changes in cases
where there is a simple bug, or where there is something like a broken
dependency where the package simply needs to be rebuilt. Do consider opening it
up as soon as you are comfortable. 

cvs done.
Comment 14 Fabio Massimo Di Nitto 2008-05-19 13:36:31 EDT
Hi Kevin,

thanks a lot for your work. Very much appreciated.

Fabio
Comment 15 Milos Jakubicek 2008-05-20 12:59:48 EDT
(In reply to comment #12)
> New Package CVS Request
> =======================
> Package Name: cluster
> Short Description: Red Hat Cluster
> Owners: fabbione swhiteho cfeist lon
> Branches: F-10

Hi Fabio,

please consider pushing this also into F-9 -- there shouldn't be any problems
with it now, are there any? (I mean F-9 and current rawhide is still almost the
same thing now).

Thanks in advance,
Milos
Comment 16 Fabio Massimo Di Nitto 2008-05-20 15:28:28 EDT
This package will not go in F-9. F-9 will get updates soon but from the stable2
branch (version 2.03.xx) once i have done with testing.

Fabio

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