Bug 244894 - Review Request: yum-cron - get yum updates via a cron job
Review Request: yum-cron - get yum updates via a cron job
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-19 14:34 EDT by Habig, Alec
Modified: 2007-11-30 17:12 EST (History)
19 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-22 18:09:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Habig, Alec 2007-06-19 14:34:33 EDT
Spec URL: http://neutrino.d.umn.edu/~habig/yum-cron.spec
SRPM URL: http://neutrino.d.umn.edu/~habig/yum-cron-0.2-2.src.rpm
Description: 

These are the files needed to run yum updates as a cron job.  They are lifted straight from yum-2.6.1-0.fc5, but were left out of FC6's yum. Install this package if you want auto yum updates nightly via cron rather than the newer yum-updatesd daemon.

This is my first Extras contribution (although I maintained some stuff in contrib many years and distributions ago), so I do need a sponsor.
Comment 1 Habig, Alec 2007-06-19 14:45:43 EDT
This package was spawned by the discussion of the massive (and as of yet
unsolved) yum-updatesd breakage, and the ancillary question of if a daemon
checking for updates on a very rapid timescale is at all an appropriate thing in
the first place.  Especially on non-single-user desktop systems.  This can all
be found on bug:

  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212507
Comment 2 Bill McGonigle 2007-06-19 14:54:49 EDT
Just a note - I've been running Alec's package (the previous iteration) on a
dozen or so servers and they have worked correctly.

Thanks for running this up the flagpole for us, Alec.
Comment 3 Jason Tibbitts 2007-06-19 15:16:18 EDT
Tough to get a word in edgewise with all of the mid-air collisions....

I agree that this is needed, if only so that I can stop rolling my own package
to do the same thing.

rpmlint says this:
  W: yum-cron conffile-without-noreplace-flag /etc/yum/yum-daily.yum
I actually tweak this on some machines so it should probably be marked
%config(noreplace).

You shouldn't run "chockofng service on" in %post, or start the service either.
 You should also move the condrestart to %postun.  See
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets for more information
about installing services.
Comment 4 Habig, Alec 2007-06-19 16:38:19 EDT
Thanks for the review!  Updated:

Spec URL: http://neutrino.d.umn.edu/~habig/yum-cron.spec
SRPM URL: http://neutrino.d.umn.edu/~habig/yum-cron-0.2-3.src.rpm

Questions:

 1) /etc/yum/yum-daily.yum is not really a config file, it's the commands given
to yum.  So, a script, albeit not a bash or perl script, and not executable. 
So, I think we do want it replaced, as it's code not configurations.  Is there a
file class that's not an executable but not a config file which I should be
using instead?  Just leaving it unlabeled gets rpmlint grumpy since it's in /etc. 

 2) The only starting of the "service" now is done in %postun if "$1" -ge "1"
(there's been an upgrade), and that's a conditional restart.  So I think that's
now meets standards.

Comment 5 Gilles Detillieux 2007-06-20 13:02:32 EDT
I've been using the 0.1-1 version of this package for several months on FC6 and
it's been working like a charm.  More recently, I've also used it successfully
on FC7 Test 4 (which is now rawhide) and CenOS 5 (RHEL 5 clone).  I just
rebooted my rawhide system after it's been shut off for 5 days, installed the
yum-cron-0.2-2 package, and let anacron do its thing.  After a bit more than an
hour, the yum.log shows that 139 packages were updated, 8 installed, and 1
erased, all by the yum cron job.  What can I say, it still works like a charm!

I agree that /etc/yum/yum-daily.yum is really a script, not a config file, but
as I don't touch it myself it doesn't matter too much how the RPM designates it,
as long as I get any needed updates to it.

One little oddity is that when I did an "rpm -Uvh" to update from yum-cron 0.1-1
to 0.2-2, it left behind the /etc/rc.d/rc?.d/[KS]??yum symlinks and the lockfile
for the old "yum" service, which I needed to remove manually.  On other systems,
I did an "rpm -e yum-cron" before installing the new version, and that seemed to
do the trick.

I hope the package gets pushed out to rawhide, F7, FC6 and RHEL5.  Good work!
Comment 6 Habig, Alec 2007-06-20 13:14:42 EDT
Aha, I can confirm the non-deletion of the /etc/rc.d/rc?.d/[KS]??yum symlinks
when upgrading from the 0.1-1 version of the package.  Sorry.  This is a result
of the change of the initscript's name from "yum" to the rpmlint suggested
"yum-cron", combined with the old script's not properly checking for upgrade vs.
complete removal in the %postun script, something which is hopefully fixed in
the current version (0.2-3).

A question to the specfile experts out there: is cleaning this up something
which can be done gracefully in the new package?  As far as I can tell, anyone
with 0.1-1 version rpms installed is stuck with their malformed %postun script
running when the old version is cleaned up, which won't "chkconfig --delete" the
old "yum" service properly when upgraded rather than removed.  Is there an
appropriate place to put such housecleaning in the new rpm?
Comment 7 Bill McGonigle 2007-06-20 13:20:11 EDT
I don't know how complex you want to get, but you could have the script check
for a yum-daily-local.yum file and use it instead of yum-daily.yum if it exists.
 That way local admins who need to do something different won't get bothered by
updates but everybody else keeps on working.

There doesn't appear to be anything like an #include syntax to do more elegant
things.
Comment 8 Gilles Detillieux 2007-06-20 13:46:49 EDT
The old preun script was doing the right thing in only doing a "chkconfig --del"
on a full uninstall, not on an update.  The problem stemmed from the fact the
init script name changed, which is an exceptional condition, an not something
the writer of the old preun script could have foreseen.  I think in other
similar cases, when the init script name changes, the package name changes too,
and the new one is set up to obsolete the old one, so the old one gets fully
uninstalled.  In this case, as it's a simple update of the same package, I think
the only thing you can do is check in the pre script to see if it's an upgrade
($1 -gt 1) and then remove (or rename) the lockfile and chkconfig --del yum. 
Renaming, rather than removing, the lockfile will ensure the service keeps
running after the name change.  The "condrestart" in the post script didn't work
on the upgrade from 0.1-1 to 0.2-2, again because of the name change of the
lockfile.
Comment 9 Habig, Alec 2007-06-28 16:49:21 EDT
Updated in response to the problems with upgrading from the old yum-cron-0.1-1:

Spec URL: http://neutrino.d.umn.edu/~habig/yum-cron.spec
SRPM URL: http://neutrino.d.umn.edu/~habig/yum-cron-0.2-4.src.rpm

In the %post scriptlet, I now check for the existence of the oldly-named
/etc/init.d/yum script.  If it's there, I assume we're upgrading from the old
version.  If the old service was active, the new yum-cron is also started.  If
it was inactive, the new script defaults to installed but off.  In either case,
the oldly named yum service is removed from chkconfig control.

Tested on machines running 0.1-1 in various states, and in fresh installs.

The new %post scriptlet:

%post
# Make sure chkconfig knows about the service
/sbin/chkconfig --add yum-cron
# if an upgrade:
if [ "$1" -ge "1" ]; then
# if there's a /etc/rc.d/init.d/yum file left, assume that there was an
# older instance of yum-cron which used this naming convention.  Clean
# it up, do a conditional restart
 if [ -f /etc/init.d/yum ]; then
# was it on?
  /sbin/chkconfig yum
  RETVAL=$?
  if [ $RETVAL = 0 ]; then
# if it was, stop it, then turn on new yum-cron
   /sbin/service yum stop >> /dev/null
   /sbin/service yum-cron start >> /dev/null
   /sbin/chkconfig yum-cron on
  fi
# remove it from the service list
  /sbin/chkconfig --del yum
 fi
fi
exit 0
Comment 10 Habig, Alec 2007-07-09 12:03:08 EDT
So, how do we proceed from here?  I think all the technical and stylistic issues
with this package's spec file are now all sorted out (if not, please let me know!).

What do I need to do to wrap up this review process, get sponsored on the cvs
and build machines, and move the new .rpm into live the extras area?

Thanks,

  Alec
Comment 11 Răzvan Sandu 2007-07-12 16:15:08 EDT
Hello,

I also subscribe to the general choir that please to add this package in Fedora
7 (which is final now).

I've tested Alec's package and works well on my machines (about a hundred).


Regards,
Răzvan
Comment 12 Jason Tibbitts 2007-07-13 13:19:43 EDT
The link to the SRPM in comment #9 above seems to be invalid.

I'll be happy to shepherd this package through the system, but, well, I need a
package to review first.
Comment 13 Habig, Alec 2007-07-17 13:20:12 EDT
Doh, sorry.  Bogus symlink fixed, comment #9 URLs now work.  
Comment 14 Jason Tibbitts 2007-07-29 14:59:26 EDT
I finally have some more  free time....
Comment 15 Jason Tibbitts 2007-07-29 15:52:22 EDT
OK, this is a special package, since it's just a collection of crontab files. 
We actually have guidelines for how to deal with this; see
http://fedoraproject.org/wiki/Packaging/SourceURL (the "We Are Upstream" section).

I note that you're not using the dist tag.  This isn't a requirement (and for a
package like this which shouldn't need changes between distro versions it's
completely acceptable), but I want to make sure you understand how you'll need
to keep ordering consistent.

I would use "BuildArch" instead of "BuildArchitectures", but that's personal
preference.

You have no dependencies for your scriptlets.  For example, if you call
/sbin/chkconfig in %post, you need Requires(post): /sbin/chkconfig.  So you'll
need at least these:
   Requires(post): /sbin/chkconfig
   Requires(preun): /sbin/chkconfig
   Requires(preun): /sbin/service
   Requires(postun): /sbin/service
See http://fedoraproject.org/wiki/Packaging/ScriptletSnippets.

Conversely, you don't need /sbin/chkconfig and /sbin/service in your Requires:
list, because they aren't actually called by the files installed as part of this
package.

The scriptlets themselves have a few issues.  I guess it's OK to append to
/dev/null, although it does look a bit odd.  But you also need to redirect
STDERR (with 2>&1).  Otherwise they look OK.

There's a bunch of stuff going with init scripts which frankly I don't yet
understand.  I don't think the init script in this package conforms.  There's
some information at http://fedoraproject.org/wiki/FCNewInit/Initscripts but
there are still a bunch of unanswered questions and frankly I'm not going to
block this package based on what's in that document.

So there are a few minor things to fix up.  Do those and apply for cvsextras
access and I'll sponsor you.

Review:
X This package is the upstream, but this is not explained in the spec.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
X dist tag is not present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper (none)
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* rpmlint is silent.
X final provides and requires:
   config(yum-cron) = 0.2-4
   yum-cron = 0.2-4
  =
   /bin/bash
   /bin/sh
X  /sbin/chkconfig
X  /sbin/service
   config(yum-cron) = 0.2-4
   crontabs
   vixie-cron
   yum
  /sbin/chkconfig and /sbin/service should not be in the runtine dependency 
  list.

* %check is not present; no test suite upstream.
  There's plenty of evidence of successful testing in this review ticket.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
X scriptlets are completely missing dependencies.
X scriptlets should redirect STDERR but otherwise look OK.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
Comment 16 Habig, Alec 2007-07-30 01:50:06 EDT
Thanks for the review!  Updated package files, with your concerns addressed:

  Spec URL: http://neutrino.d.umn.edu/~habig/yum-cron.spec
  SRPM URL: http://neutrino.d.umn.edu/~habig/yum-cron-0.2-5.src.rpm

Questions/comments:

Regarding the "We Are Upstream" bit:  Added the comments.  Momentarily took out
the URL field (which points to the main yum website) since, well, We Are
Upstream.  But rpmlint didn't like not having a URL.  What's the appropriate URL
to use in this case?

Regarding the dist tag: went and read all about it, and think that it's not
appropriate here.  The same spec file will happily work for all dists which are
using the yum after which the cron files were yanked, yum-3.0.  So, I changed
the "requires" to yum >= 3.0, which should cover all the bases.

I see what you mean about the initscripts - it's moving to a whole new format,
while this script is of the old format.  I'll work on understanding and porting
to the new standards, but that won't happen for a while and I appreciate you not
blocking things while I do so.

One warning - I'm about to embark on a two-week combination of moving my family
to another town (on sabbatical this coming year) and camping/vacation trip, so
will have spotty net.access in the interim.  So, please don't be offended if I'm
not so responsive till mid-August.
Comment 17 Jason Tibbitts 2007-08-01 13:33:12 EDT
It's OK not to have a URL if there's a reason not to have one.  Yes, rpmlint
will complain but it's OK to ignore it when there's a valid reason to do so.

This looks good to me.  I've already done the sponsorship magic so you should be
able to make your CVS resquest and get this imported whenever you have time.

Note that because you're not using the dist tag and want to keep the same
package version across multiple releases, you need to understand how the build
inheritance works in koji.  I suggest you contact the releng team for a pointer;
it's possible they need to set something up.

APPROVED
Comment 18 Frank Wittig 2007-08-03 11:15:30 EDT
I've made an addidtion to the yum-cron script that makes yum skip all updates
except those for its own package. It then reports all pending updates by
cron-mail instead to inform the administrator instead of installing them.

The check-only feature can be configured by editing /etc/sysconfig/yum-cron.

I've merged my own additions into the above mentioned package version 0.2-5. The
resulting SRPM can be obtained under
ftp://ftp.weisshuhn.de/pub/yum-cron-0.2-6.src.rpm

Please consider including this changes into future releases.
Comment 19 Habig, Alec 2007-08-20 16:29:45 EDT
New Package CVS Request
=======================
Package Name: yum-cron
Short Description: Files needed to run yum updates as a cron job
Owners: habig
Branches: FC-6 F-7
InitialCC: 
Commits by cvsextras: yes
Comment 20 Kevin Fenzi 2007-08-20 19:01:28 EDT
cvs done.
Comment 21 Habig, Alec 2007-08-21 15:21:03 EDT
Package Change Request
======================
Package Name: yum-cron
New Branches: EL-5
Re-reading comments from reviewers, I see that this is needed for EL-5 as well
as the existing FC-6 and F-7.  Sorry for missing this on the first go-round.
Comment 22 Kevin Fenzi 2007-08-21 19:21:44 EDT
cvs done.

Note that EL-5 builds will go to the EPEL testing repository, so look for your
completed builds there. 
Comment 23 Habig, Alec 2007-08-22 18:09:57 EDT
yum-cron has appeared in the devel tree.  Thanks to everyone for your help
getting this off the ground.

Frank's tweaks from comment #18 are not included yet, I wanted to get the part
I'd thought about longer out first, and will use including his enhancements as
an excuse to go through the patch-build-test-release cycle once the initial
package successfully propagates (best way for me to learn the system is to do it).

It's also been tagged for pushing into testing for F-7, is awaiting signatures
for EPEL-5, and plague refuses to cooperate (plague throws an error even though
it builds OK) for FC-6.  But I can beat on that weirdness later.
Comment 24 Bill McGonigle 2007-08-30 15:53:43 EDT
For those impatient to wait for this to hit the updates repo, this worked for me
on F7 to pull it from testing:

  yum --enablerepo=updates-testing install yum-cron

and accept the testing key,.  Probably obvious to many, but I had to look it up.

I have no idea why, but the first run on the first F7 machine I tried it on took
5.5 hours on a 2.5GHz P4 server (to install 41 updates).  That was completely
surprising, as yum never takes that long to do anything but 'yum upgrade' a
distro.  Subsequent runs take a normal amount of time, but this is likely a yum
problem anyway, not a yum-cron problem.
Comment 25 Habig, Alec 2007-09-03 11:32:24 EDT
A warning - if you've installed 0.3-3, please watch for 0.4-1 to appear (just
built it and requested a push) and update it manually.  Sorry Bill, just as you
got the long-awaited package.

0.3-3 had a bug which prevented auto-updating.  Which is doubly nasty in this
package, since if you're relying on auto-updates to get your updates, then you
won't get the fix to the auto-update either.  Doh!

At least the bad one hadn't made it out of testing yet for F-7.

Apologies.

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