main inclusion request: update-motd

[Dustin Kirkland ]

Please include update-motd in the main archive.

MIR can be found here:
 * https://wiki.ubuntu.com/MainInclusionReportUpdateMotd

:-Dustin

[Dustin Kirkland  (kirkland)]

Required by landscape-client.

:-Dustin

[Martin Pitt (pitti)]

The configuration file handling in here needs to be fixed. At the moment, the init script does

  sed "s/FREQ_IN_MIN/$FREQ_IN_MIN/" "/usr/share/update-motd/update-motd.cron" > /etc/cron.d/update-motd

unconditionally. This overwrites the admin's changes without checking or confirmation.

If you ask me, I would ship /etc/cron.d/update-motd as a conffile, so that the admin can change it directly, without any dynamic magic. If you don't want to do that for some reason, you should do an ucf-like approach, create the config file in the postinst if it doesn't exist yet, or use ucf to manage it on package install/upgrades if it exists and was modified. I don't really see the need to do it in the init script. No other package does it that way, and it is higly unusal and also pretty brittle.

[Dustin Kirkland  (kirkland)]

Martin-

Notice that the init script sources /etc/default/update-motd, where it should obtain $FREQ_IN_MIN.

To change /etc/cron.d/update-motd, the administrator could either "dpkg-reconfigure update-motd", or edit /etc/default/update-motd directly (which is what the postinst script will affect).

The file, /etc/cron.d/update-motd has a note to that effect in it:

    # /etc/cron.d/update-motd: crontab fragment for update-motd
    #
    # THIS FILE IS AUTOMATICALLY GENERATED, DO NOT EDIT DIRECTLY.
    # To change, use:
    # dpkg-reconfigure update-motd
    #
    # This updates the /etc/motd with a concatenation of output from each
    # script in /etc/update-motd.d
    #
    # This file is automatically generated by /etc/init.d/update-motd, which
    # uses /usr/share/update-motd/update-motd.cron as a template.
    */10 * * * * root /usr/sbin/update-motd 2>/dev/null

Does that assuage your concerns at all?

:-Dustin

[Dustin Kirkland  (kirkland)]

Martin-

I put it in the init script such that it would be easy for an administrator to "stop" update-motd from running, by removing the script from /etc/cron.d.

And the "start" restores the /etc/cron.d script by regenerating the script from the template in /usr/share, and reading the $FREQ_IN_MIN established by debconf question in /etc/default/update-motd.

:-Dustin

[Dustin Kirkland  (kirkland)]

Martin-

I'm open to other solutions, however, I really, really want to have working "start" and "stop" operations, that would effectively enable and disable the update-motd cronjob.

Any suggestions?

:-Dustin

[Martin Pitt (pitti)]

Hi,

Dustin Kirkland [2008-09-13 14:59 -0000]:
> I put it in the init script such that it would be easy for an
> administrator to "stop" update-motd from running, by removing the script
> from /etc/cron.d.
>
> And the "start" restores the /etc/cron.d script by regenerating the
> script from the template in /usr/share, and reading the $FREQ_IN_MIN
> established by debconf question in /etc/default/update-motd.

But this is so much unlike the behaviour of all other daemons. init
scripts are used for starting/stopping processes, not
creating/removing configuration files. OTOH "rm /etc/cron.d/update-motd"
should cause the cronjob to not be run any more, instead the next boot
would restore it again by running the init script.

So the current package violates common practice of init scripts and
configuration files. Also, TBH it seems way too overengineered to me.
Why not drop the default file, init script, and all that black magic,
and ship /etc/cron.d/update-motd as a standard conffile with a ten
minute interval? It's still just an one-line file, thus it is not
significantly easier or harder to change/maintain than the default
file, and it would make the behaviour so much more obvious. You'd also
retain the possibility of changing the defaults in a future version,
which is impossible in the current setup.

Thanks for considering,

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

[Dustin Kirkland  (kirkland)]

Hi Martin-

I just uploaded update-motd_1.3, hopefully addressing all of your concerns, and preserving most of the functionality that I really wanted to keep.

You can see the changelog in the last 2 uploads for per-file change documentation, but I'll explain here for continuity...

The dpkg-reconfigure continues to prompt for:
 * RUN=true|false

If RUN=true, then it will also prompt for:
 * FREQ_IN_MIN, default to the current user-specified value as extracted from /etc/cron.d/update-motd, or "10" if unspecified.

If RUN!=true, the cronjob will be removed entirely, saving CPU-cycles.

The value of RUN is written to the /etc/default/update-motd configuration file, which is sourced by /etc/init.d/update-motd, and determines whether or not the update-motd init script will start the "daemon" (by touching /var/run/update-motd.enabled).

The value of FREQ_IN_MINUTES is written to /etc/cron.d/update-motd by injecting the value into a template stored at /usr/share/update-motd/update-motd.cron. On package configuration/reconfiguration/upgrade, this value is seeded and read from /etc/cron.d/update-motd. The note at the top of that file remains, warning users that it would be best to use dpkg-reconfigure update-motd to regenerate that file.

If the init script /etc/init.d/update-motd was able to start update-motd (ie, RUN=yes, and the cronjob exists), then a file is touched, /var/run/update-motd.enabled. This is basically the equivalent of a "pidfile", except that update-motd runs as a cronjob rather than a daemon, and as such, has no pidfile.

The executable /usr/sbin/update-motd now checks that /var/run/update-motd.enabled exists, although it does allow for a --force override. It's basically a no-op in the case where /var/run/update-motd.enabled does not exist, and prints an error message informing the user how they might go about re-enabling update-motd.

With these changes, I think we both get what we want...

From your perspective, /etc/init.d/update-motd no longer inserts FREQ_IN_MIN; that is *only* handled by the dpkg-reconfigure.

From my perspective, FREQ_IN_MIN is still "dpkg-reconfigure enabled", and the user can stop update-motd either permanently (across reboots) using dpkg-reconfigure, or temporarily (within the current boot) using "/etc/init.d/update-motd stop".

Regarding your comment about being "over engineered"... The update-motd process runs as a simple cronjob, as opposed to spawning a real daemon. The non-standard things I might have written into the init.d script are as a result of this. Perhaps in an update-motd_2.0 version, I can write a simple daemon, but that's for a future feature request...

:-Dustin

[Colin Watson (cjwatson)]

= Cron job =

I really think we need to lose the debconf configuration of cron job frequency. I know that you've put effort into making it at least somewhat policy-compliant, but:

 * no other package that I know of offers debconf configuration of cron job frequency, so system administrators will not expect it;
 * CAPITALISED HEADERS telling people not to edit files in /etc/ are ugly, and have never been popular;
 * extra debconf-based configuration file management is always complicated and is typically a source of bugs;
 * I don't see where you've justified the need for this.

Honestly, a file in /etc/cron.d/ is trivial to edit, and editing files is the perfectly normal interface to changing cron job frequencies. I don't think it's at all necessary to add debconf configuration on top of this, and I think it is probably going to do more harm than good (on You Ain't Gonna Need It principles: bugs are correlated with number of lines of code, so unnecessary features should be removed).

I know the temptation to add debconf configurability for everything: I used to do it a lot. I later realised that it was causing me more problems than it was solving, so had to spend time ripping it out again.

In this case, most system administrators will find it entirely unsurprising that they should edit a file in /etc/cron.d/ to change the frequency of a cron job; very few of them will think to run dpkg-reconfigure to do so, even those familiar with Debian-based systems, because Debian has never had a standard practice of using debconf to configure cron job frequencies.

= Start/stop =

I think I'm OK with the start/stop semantics now. They are confusing at first glance, but essentially it's starting up a "daemon" whose existence is time-multiplexed. :-) Specifically, they are no longer modifying system state that will persist across reboots, which would definitely be intolerably confusing as no other init script works that way.

= Miscellaneous =

Don't use db_stop unless you know exactly what it does and you're sure you need it; it has surprising semantics (in particular, it doesn't put your file descriptors back the way they were to start with ...) and can cause problems. In the case of a daemon, it's usually better to make sure that the daemon closes file descriptors on startup. In this case, you aren't starting a daemon so there's no need for it.

[Dustin Kirkland  (kirkland)]

I just uploaded update-motd_1.6. I hope that this version satisfies the requests of both pitti and cjwatson.

Notably this release:
 * eliminates the init script entirely (purging it from the system in the postinst if upgrading from a version <1.6)
 * purges all debconf handling (config, postinst, po, templates)
 * installs /etc/cron.d/update-motd as a conffile
 * adds --enable/--disable mechanisms directly into /usr/sbin/update-motd itself, which will rm/touch /var/lib/update-motd/disable, which will persist across boots.
 * adjusts the build and documentation accordingly
 * writes a timestamp to /var/run/update-motd.lastrun

Can you please reconsider for main inclusion? This is still blocking landscape-client.

Thanks,
:-Dustin

[Martin Pitt (pitti)]

Hi Dustin,

thanks for the discussion yesterday.

Dustin Kirkland [2008-09-18 2:31 -0000]:
> * eliminates the init script entirely (purging it from the system in the postinst if upgrading from a version <1.6)
> * purges all debconf handling (config, postinst, po, templates)
> * installs /etc/cron.d/update-motd as a conffile
> * adds --enable/--disable mechanisms directly into /usr/sbin/update-motd itself, which will rm/touch /var/lib/update-motd/disable, which will persist across boots.
> * adjusts the build and documentation accordingly
> * writes a timestamp to /var/run/update-motd.lastrun

That sounds great, and should make maintenance and administration much
easier and conformant. I'll have another look ASAP, conference starts
soon (so if Colin beats me to it, please go ahead).

[Martin Pitt (pitti)]

Some review comments:
 - postinst: use lt-nl instead of lt for --compare-versions
 - prerm: You should be able to drop this entirely; conffiles are automatically removed on purge
 - there should be a preinst which makes sure you don't get a dpkg conffile conflict question on intra-intrepid upgrades, but don't worry too much about it; developers should be able to figure that out.
 - any reason why you use both dh_install and "install" in debian/rules to install files? ideally they would all use dh_install

Promoted to main.

[Dustin Kirkland  (kirkland)]

On Thu, Sep 18, 2008 at 11:37 AM, Martin Pitt <email address hidden> wrote:
> Some review comments:
> - postinst: use lt-nl instead of lt for --compare-versions

Cool, thanks. Will change.

> - prerm: You should be able to drop this entirely; conffiles are automatically removed on purge

Well, I thought it would be a good idea to remove the cronjob even on
just a normal removal (in addition to a purge). Once the binary
update-motd is gone, the cronjob is broken and is wasting cpu cycles
and spewing error messages.

Are you sure you want me to remove the prerm?

> - there should be a preinst which makes sure you don't get a dpkg conffile conflict question on intra-intrepid upgrades, but don't worry too much about it; developers should be able to figure that out.

I talked to Colin about this, and he said not to worry about it...only
affecting users running an Alpha version of Intrepid. I noticed it in
my testing, and suggested fixing this.

> - any reason why you use both dh_install and "install" in debian/rules to install files? ideally they would all use dh_install

No good reason. Sorry.

> Promoted to main.

Thanks!!!

:-Dustin

[Martin Pitt (pitti)]

Hi Dustin,

Dustin Kirkland [2008-09-18 17:03 -0000]:
> > - prerm: You should be able to drop this entirely; conffiles are
> automatically removed on purge
>
> Well, I thought it would be a good idea to remove the cronjob even on
> just a normal removal (in addition to a purge). Once the binary
> update-motd is gone, the cronjob is broken and is wasting cpu cycles
> and spewing error messages.

The error messages can be fixed by checking existence of the binary
first, or "|| true"ing it.

conffiles should not automatically be deleted on package removal at
least if they are modified. Since you are not checking this, I'd
suggest to rather leave conffile removal handling to dpkg, which will
ensure that it is conformant to policy, best practices, and what
admins are used to.

To be completely honest, the cronjob is already a waste of CPU cycles
in the default case (it will create the identical motds over and over
again every ten minutes). So I wouldn't worry too much about the
removed/not purged case.

> > - there should be a preinst which makes sure you don't get a dpkg
> > conffile conflict question on intra-intrepid upgrades, but don't worry
> > too much about it; developers should be able to figure that out.
>
> I talked to Colin about this, and he said not to worry about it

Full ack, I just wanted to mention it.