Bug 223633 - Review Request: csync2 - A cluster synchronization tool
Review Request: csync2 - A cluster synchronization tool
Status: CLOSED DUPLICATE of bug 676187
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-20 19:37 EST by Ruben Kerkhof
Modified: 2011-02-15 14:30 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-26 14:20:52 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ruben Kerkhof 2007-01-20 19:37:24 EST
Spec URL: http://rubenkerkhof.com/packages/csync2.spec
SRPM URL: http://rubenkerkhof.com/packages/csync2-1.33-1.src.rpm
Description:
Csync2 is a cluster synchronization tool. It can be used to keep files on 
multiple hosts in a cluster in sync. Csync2 can handle complex setups with 
much more than just 2 hosts, handle file deletions and can detect conflicts.
It is expedient for HA-clusters, HPC-clusters, COWs and server farms.
Comment 1 manuel wolfshant 2007-01-22 07:31:57 EST
      The fact that you install csync2.xinetd makes me think that you must
Require xinetd.
      Also, since the policy in Fedora is to NOT install services in a
configuration which leaves them activated by default, I think that you should
patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it
would not automatically start when the next service xinetd start/restart occurs.
The admin should specifically activate the new service.
Comment 2 Ruben Kerkhof 2007-01-22 17:41:47 EST
Hi Manuel,

>>The fact that you install csync2.xinetd makes me think that you must require xinetd.

Fixed

>> Also, since the policy in Fedora is to NOT install services in a
>> configuration which leaves them activated by default, I think that you should
>> patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it
>> would not automatically start when the next service xinetd start/restart occurs.
>> The admin should specifically activate the new service.

Fixed as well.

New versions:
http://rubenkerkhof.com/packages/csync2.spec
http://rubenkerkhof.com/packages/csync2-1.33-2.src.rpm
Comment 3 manuel wolfshant 2007-01-23 21:13:34 EST
Good:

- rpmlint checks do not return anything either on the source or on the binary rpm
- package meets naming guidelines
- package meets packaging guidelines
- buildroot has the recommended value
- license ( GPL ) is OK, text in %doc, matches source
- spec file legible, in am. english
- source is last version and matches upstream, sha1sum
8d94eb0c7ff997598be7b1cfc444bb74eae6712d  csync2-1.33.tar.gz
- package builds in mock for devel (i386) and FC6 (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates, does not own foreign files or directories
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- available documentation is included, but small so no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- no static, headers, libtool or .pc files

SHOULD
- builds fine in mock (FC6/x86_64, devel/i386)
- runs OK on FC6/x86_64 (once the proper instructions -- included in the rpm --
are followed, that is csync2 is added to /etc/services)

Suggestion: the changelog entry should describe the actual modifications
(especially since they are small - a one liner should suffice) rather then point
to a bugzilla entry (it makes life simpler for someone examining the rpms,
avoiding the need for an extra web query; not to mention that bugzilla might be
down or simply not accessible)
As a sidenote, you could have used a 1 line sed command in %install in order to
modify the offending line in xinetd.conf, rather then creating a separate patch,
but your method is perfectly fine.

  In this moment I see no blockers, but before approving this package I would
like for someone more experienced to comment on the following:
  In order to start the program (via xinetd), /etc/services must be modified,
adding a line specifying the port and protocol used by csync2 and SSL
certificates must be generated for each box where the program will run
  Editing /etc/services could be handled either in a %post entry (in the sources
there is a spec file which does the addition, but does not verify if the entry
already exists and neither removes it at uninstall time), or (as the packager
has chosen) manually by the admin.
   My question is: which one of this two methods is the proper procedure for
Fedora ?
Comment 4 Ruben Kerkhof 2007-01-24 07:03:45 EST
Thanks for the review Manuel.
My comments inline:

>> As a sidenote, you could have used a 1 line sed command in %install in
>> order to modify the offending line in xinetd.conf, rather then creating a
>> separate patch, but your method is perfectly fine.

I actually changed 2 lines in csync2.xinetd ;-) I prefer clean patches to 
regular expressions in spec files. That way, if something changes in the 
source, the patch fails to apply and I can fix it.

>> In order to start the program (via xinetd), /etc/services must be modified,
>> adding a line specifying the port and protocol used by csync2 and SSL
>> certificates must be generated for each box where the program will run

There's no need to edit /etc/services, since xinetd doesn't require it.
I tried creating the ssl certificates in %install (make cert does this), but 
in mock, openssl seems to have problems opening a random device. I'm not sure 
what's the best way to handle this. Maybe just ship a self-signed cert with 
the rpm?
Comment 5 manuel wolfshant 2007-01-24 07:20:48 EST
     I fully agree with clean patches, but for very small modifications I prefer
 one liners to separate patches. And since you could have done "s/default
on/default off; s/disable=no/disable=yes" one line would have been enough :)

     Serious stuff now:
In my case, the first attempt to run csync2 on FC6 (without editing
/etc/services) triggered this error:

Jan 24 03:27:18 wolfy Installed: xinetd.x86_64 2:2.3.14-8
Jan 24 03:27:19 wolfy Installed: csync2.x86_64 1.33-2.fc6
Jan 24 03:27:58 wolfy xinetd[19733]: Port not specified and can't find service:
csync2 with getservbyname
Jan 24 03:27:58 wolfy xinetd[19733]: xinetd Version 2.3.14 started with libwrap
loadavg labeled-networking options compiled in.
Jan 24 03:27:58 wolfy xinetd[19733]: Started working: 0 available services



     As of creating the certs, I'd include the relevant part of the Makefile 
rather then creating the certificates by default. Maybe because I generate ALL
certificates in only one place and transfer them wherever they are needed...
Comment 6 Ruben Kerkhof 2007-01-24 07:42:52 EST
man xinetd(8): xinetd is not limited to services listed in /etc/services. 
Therefore, anybody can use xinetd to start special-purpose servers.

But for that to work, you probably have to set "type" to UNLISTED.

I'm at work now, but I'll see what I can come up with tonight.
Comment 7 manuel wolfshant 2007-01-24 08:32:13 EST
Just tested. One can either edit /etc/services or add the following two lines to
/etc/xinetd.d/csync2:
        type            = UNLISTED
        port            = <desired port value>

Which makes me suggest the following
- modify a bit your patch as to include these two lines, using the default port
value assumed by csync2 ( 30865 )
- add a README.FEDORA pointing this difference towards the included docs
- bug upstream to modify the docs (the paper.pdf)

What do you think ?
Comment 8 Ruben Kerkhof 2007-01-24 09:11:56 EST
Sounds good to me. Will do that tonight.
Comment 9 Ruben Kerkhof 2007-01-24 16:33:16 EST
New version:
http://rubenkerkhof.com/packages/csync2-1.33-3.src.rpm
http://rubenkerkhof.com/packages/csync2.spec

One rpmlint error though because I changed permissions to 640 on the ssl key, but that's a lot safer than 
world readable.
Comment 10 manuel wolfshant 2007-01-24 18:09:51 EST
     I am sorry, but I definitely MUST veto the inclusion of certificates in the
binary rpm. The reason is that this would lead to each and every computer where
the rpm is installed to have the very same certificates. Which kind of defeats
the whole purpose of certificates.
     If you want to help the users, you could use for instance one of the
following approaches:
a)
- extract the certificate generation part from the Makefile included in sources
and save it as a separate mkcert.sh
- install this file as a %doc
- document it in the README.FEDORA file I have suggested in comment #7
b)
- generate the certificates in %post

     Personally I believe that the first approach is better, allowing for the
admins to have the certificates generated only if they feel like needing them.
     You could take a look at mod_ssl and dovecot. The first one includes the
directories where Apache SSL certificates are to be stored, as well as the
scripts needed to generate the certificates, but does not include the
certificates themselves. In addition a set of sample certificates are generated
at install time. A cleaner approach (in my opinion...) is used by dovecot which
includes a dovecot-openssl.cnf file (in /etc/pki) and a mkcert.sh script
(included as doc/sample).
Comment 11 Ruben Kerkhof 2007-01-25 15:25:29 EST
Hello Manuel,

I had a look at how dovecot did it, and that's definitely a better approach
Added a mkcert.sh script and a README.fedora file with a short Getting Started text.

New release:
http://rubenkerkhof.com/packages/csync2.spec
http://rubenkerkhof.com/packages/csync2-1.33-4.src.rpm
Comment 12 manuel wolfshant 2007-01-25 18:42:13 EST
Excelent job. All tests pass as per the original review (comment #3), the
package builds OK (as before) in mock for FC6/x86_64, the new sources are
installed cleanly, all permissions are sane, changelog correctly reflects all
changes.

This package is APPROVED by me.
Comment 13 Ruben Kerkhof 2007-01-26 14:20:52 EST
Thanks for the review Manuel.
Comment 14 Jason Tibbitts 2011-02-15 14:30:23 EST

*** This bug has been marked as a duplicate of bug 676187 ***

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