[Bug 513896] Review Request: pcp - performance monitoring and collection service

Tue Aug 4 01:01:34 UTC 2009

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=513896

--- Comment #8 from Mark Goodwin <mgoodwin at redhat.com>  2009-08-03 21:01:33 EDT ---
(In reply to comment #5 and comment #6)
> Ok a few more comments, sorry for being iterative.

thanks for the review/update Eric, much appreciated,
sorry I didn't get back to this yesterday .. those pesky
customers again :)

> 
> Naming the spec "pcp_fedora.spec" threw me off, not sure if it's kosher or not.

I just checked it into (my) pcp git tree, mainly for safe-keeping,
but also for easier future reconcilliation with the upstream spec,
which is now quite different. I think it would also be reasonable
to check-in the SLES spec as well, since it's different too.

> Keeping a fedora spec upstream probably just makes it harder to keep things in
> sync; fedora cvs should be pretty capable of keeping track of the fedora pcp
> specfile.

yeah, once it's reconciled and checked into cvs, then maybe it can be
removed from the tree (or that commit can be revoked).

> %if %{have_ibdev}
> %define ib_prereqs libibmad libibumad  libibcommon
> %define ib_build_prereqs %{ib_prereqs} libibmad-devel libibumad-devel
> libibcommon-devel
> %endif
> 
> %ifarch ia64
> Requires: libunwind
> %endif
> 
> I think the ib_prereqs can be completely dropped; they're all libraries, and if
> configured to use it (?) rpm will work that out.  So I'd just drop
> %{_ib_prereqs} altogether.

yes they're all libraries, but no they can't be dropped. When Max wrote
the infiniband PMDA, he needed to change the IB libs. Those IB changes are
upstream now, but not (yet) available in any standard RH or FC build AFAIK.
So the build and run-time prereqs will need versioning added .. which
I'll add once I figure out what it is :) For now, %{have_ibdev} should
stay and be false. FWIW, having IB monitoring is important for most HPC
users, so we really do want to revisit this.

> Same goes for the libunwind stuff on ia64 most likely?  Or is it explicitly
> needed for some reason?  If so I'd add a comment.

I'd add a comment if I could remember the context for needing the prereq.
Another one lost in the depths of antiquity, probably an SGI-only thing?
Maybe we can drop it, run qa and see what breaks on ia64

> BuildRequires: gcc-c++ libstdc++-devel procps autoconf bison flex ncurses-devel
> %{?ib_build_prereqs}
> 
> as per: https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
> 
> There's no need for gcc-c++ or libstdc++-devel (as gcc-c++ requires this)

yep ok thanks

> However, doing a clean build fails due to missing BuildReqs:
> 
> checking if ExtUtils::MakeMaker is installed...  no
> FATAL ERROR: Perl ExtUtils::MakeMaker module missing.
> 
> so add:
> 
> BuildRequires: perl-ExtUtils-MakeMaker

ok thanks, added.

> Ok and some comments on some of the warnings & errors
> 
> > * pcp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpcp_pmda.so.3
> > Nothing can really be done about this - the exit() context is well
> > known and understood.
> 
> By the pcp folks I guess?  It's expected that the library exits, not the
> application?

yes that's correct. the error is basically fatal for a daemon PMDA
and is a known way for a daemon PMDA to terminate.
(and the exit() is not in the code-path for a DSO PMDA).

> > * pcp.x86_64: E: arch-dependent-file-in-usr-share
> > Several demo binaries are being installed in /usr/share/pcp/demo.
> > I can move these elsewhere, if requested. These binaries are not
> > used by the PCP core functionality.
> 
> Maybe don't build but just ship the c files?   As a hack, if you don't want to
> change the upstream build, you could rm them post-%install.

A better longer term plan would be separate packages for PMDAs, which
could also have -devel variants too. 

> > * pcp.x86_64: W: devel-file-in-non-devel-package
> > Several headers and some 'C' files in PCP are actually configuration
> > files. Discussed with the PCP community and they agree to leave these
> > as-is.
> 
> Maybe a comment in the %files section would help the next reviewer :)

yep ok, added the following :

# Note: there are some headers (e.g. domain.h) and in a few cases some
# C source files, that match the following pattern. rpmlint complains
# about this, but note: these are not devel files, but rather they are 
# (slightly obscure) PMDA config files.
%{_localstatedir}/lib/pcp/pmdas/*/*

> > * pcp.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/pcp
> > Well, it actually isn't a config file - we want it unconditionally
> > updated after an upgrade. Same for /etc/pcp.env
> 
> hah, well:
> 
> # rpmlint rpmlint
> rpmlint.noarch: W: non-conffile-in-etc /etc/bash_completion.d/rpmlint
> 
> so I suppose it'll be hard to argue too much :)

yep, so we'll ignore that one :)
Note that rpmlint also installs %dir /etc/bash_completion.d   :)

> 
> But I think if you mark it as %config, it will still be unconditionally updated
> on an upgrade, but if it's edited, the edited one will go to .rpmsave.

ok I've marked it %config, can't hurt

> > * pcp.x86_64: W: log-files-without-logrotate /var/log/pcp
> > PCP has it's own log management system, see pmlogger_daily(1)
> 
> Just to be a pain; is that necessary?  If logrotate is available, can it not be
> used so as not to surprise the admin?  Or is there a requirement for a special
> homegrown tool?

the special homegrown tool is integral to the whole PCP logging infrastructure.
I can't change that without major architectural changes ... which I'm not
willing to undertake at this point!

> > * pcp.x86_64: W: dangerous-command-in-%post chmod
> > * pcp.x86_64: W: dangerous-command-in-%preun rm
> > This could be avoided with %ghost directive, if needed.
> 
> just out of curiosity are the chmod's actually needed?

they're there for safety, I think because we weren't sure what umask is
prevailing 

> 
> Looking at the scriptlets, I have to say that the .rpmsave & .rpmnew
> manipulation kinda bugs me.  Those are supposed to be left for the admin to
> deal with.  If it really has to be there can you add a comment?

well that stuff was put there many moons ago because the default action
on an upgrade were NOT what (SGI) customers wanted - they complained.
I'll remove them for Fedora, for now, but they might come back oneday.

> Also just thinking aloud; the scripts source /etc/pcp.env but that's not
> expected  to change, right; would it make any more sense to drop that and just
> use the same rpm path macros as you installed to?

/etc/pcp.conf sources /etc/pcp.conf, which may or may not get edited
after install. That breaks the consistency of the whole rpm macro /
/etc/pcp.conf variable sync-up. I don't know what to do about that.
For now, yes let's got for the simple case and remove the crud.

> 
> IOW could you just do:
> 
> %preun
> if [ "$1" -eq 0 ]
> then
>     #
>     # Stop daemons before erasing the package
>     #
>     /sbin/service pcp stop >/dev/null 2>&1
>     /sbin/service pmie stop >/dev/null 2>&1
>     /sbin/service pmproxy stop >/dev/null 2>&1
> 
>     /sbin/chkconfig --del pcp >/dev/null 2>&1
>     /sbin/chkconfig --del pmie >/dev/null 2>&1
>     /sbin/chkconfig --del pmproxy >/dev/null 2>&1
> 
>     rm -f %{_datadir}/pcp/lib/.NeedRebuild
> fi
> exit 0
> 
> and similar for the creation of the .NeedRebuild stuff.  It just seems simpler
> to me, but just a suggestion.

ok, let's go simple and standard, for now. The crud might come back oneday
though ...

> 
> Also on the root.saved stuff - I can't find anything in scripts or source that
> creates this.  I see root.prev though.

I'll dig further for this one .. antiquity again. I think it has something to
do with the XFS NULL files bug, and the need to preserve a safe copy of a
critically important PCP config file.

New spec copied up to same place on OSS in a few mins (and checked into my
git tree too). I'll attach the new version to this BZ too.

Cheers and thanks
-- Mark

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.