[vorlon@debian.org: [Pkg-openldap-devel] Problems with current slapd postinst]

[Steve Langasek]

--8NvZYKFJsRX2Djef
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 18, 2005 at 12:41:36AM +0200, Torsten Landschoff wrote:
> > do that is that there's unnecessary user interaction here, or at least =
we're
> > asking the wrong question.

> My idea was to ask if automatic upgrades should be attempted once (or
> for each upgrade?). Otherwise I'd like to ask the following question. If
> you'd like to eliminate that it is fine with me too. The problem is that
> we need the LDIF for automatic upgrades. I'd expect that not everybody
> will want the automatic upgrade but would still like the LDIF so that's
> the idea.=20

Well, I definitely don't think the package should be asking *more*
questions than it already does.  Given that the existing question is asked
at medium priority, I suppose it doesn't matter much if it stays.

Personally, I still don't understand why anyone would not want to use the
automatic upgrade; except for Stephen Frost, but that's just because he
apparently doesn't trust his own packages. ;)  Seriously, the worst case
scenario on an automatic upgrade is ENOSPC and the admin has to deal with
the problem by hand:  which is what he has to do anyway without an automatic
upgrade, and slapadd -q is cheap and has a high rate of success.  I can't
imagine a sane server config where there's not enough local storage for an
LDIF copy, anyway.

> >   Template: slapd/dump_database
> >   Type: select
> >   Choices: always, when needed, never
> >   Default: when needed
> >   _Description: Dump databases to file on upgrade:

> > *why* would anyone want the package to dump the database for them when =
it's
> > not needed?  This makes the upgrade slow and error prone, and eats up d=
isk
> > space with both the redundant binary copies of the database and the

> The question was not about keeping redundant binary copies. It's just to
> have the state of the old directory available in LDIF format before the
> server was upgraded. Just in case the new version breaks the database
> miserably.

Right, the fact that it was trying to create redundant binary copies was
just a bug :)

> > which should be fairly idempotent actually, since
> > move_incompatible_databases_away checks for an empty source directory a=
nd
> > moves on, while bombing out if there are files at both the source and t=
arget
> > locations.
>=20
> In fact the idea is to even move the databases away when there is no
> ldif and the format has changed (which it will do now) as the old
> database will probably get severly broken (tried that... :() with the
> new slapd. I still wonder if the postinst should complete successfully
> in that case.

I personally don't think it's worth trying to have the postinst complete
successfully in this case; which is another reason why allowing the admin to
say "never dump" doesn't seem useful to me.

> The use-case I was imaging is that the upgrading process runs like this:

> 	- apt-get install slapd (or dist-upgrade, whatever)
> 	- debconf: "Try automatic upgrade?" -> no
> 	- debconf: "Create an LDIF file?"=20
> 		-> Sane admin backed everything up to remote host, so:
> 		never
> 	- ...
> 	- after upgrade: Admin runs slapadd from his own LDIF file.

Hmm.  In that case, perhaps something like this in the postinst:

  if database_format_changed; then
  	move_incompatible_databases_away
  	db_get slapd/dump_database || true
  	if [ "$RET" !=3D "never" ]; then
  		load_databases
  	fi
  fi

?  But, the postinst will *still* fail, because slapd will not start
correctly without a database, and slapadd && dpkg --configure --pending will
fail because it will again try to move the database aside...  Again, I don't
think it's worthwhile to try to solve this.

> > Dropping the question would also partially solve another problem with t=
he
> > preinst script, which is that it does
> >=20
> >   if [ -e "/usr/share/debconf/confmodule" ]; then
> >          . /usr/share/debconf/confmodule || true
> >   fi
> >=20
> > at the top, but then goes on to call functions that depend on debconf a=
nd
> > *will* fail if it's not present.  (Both the check for whether to dump, =
and
> > the check for where to dump, fail to handle the case when the debconf
> > package is not in an installed state.)  So effectively we have an undec=
lared

> Valid point. I wanted to remove that dependency by handling that in the
> respective functions but otoh when upgrading slapd (which is the only=20
> time triggering that question) debconf should be installed because of
> the dependency of the old package. I am not sure if the old package has
> to be in installed state before the preinst of the new is run but I
> don't think this is a problem in practice. Anyway, it is an important
> point.=20

A Pre-Depend is the only way to ensure that debconf is in an *installed*
state at the time the preinst is called; without making this requirement
explicit, the package manager is free to leave debconf in a broken state
while calling the preinst for slapd, which has a decent chance of happening
on a major upgrade.

Personally, I think that if the questions will be kept, a Pre-Depends should
be added.  Stephen seemed to disagree, though; I'm not sure if he had any
more substantial reason than a visceral dislike for Pre-Depends. :)

> The other debconf data that is queried is the location of the LDIFs
> which is not fixed that easily. I really wonder why we can't make
> debconf required anyway.

Yes, the easiest (and IMHO best) way to fix that if not Pre-Depending on
debconf would be to hard-code a default location into the scripts.

> > - /usr/share/doc/slapd/examples/DB_CONFIG -- I realize this is wrapped =
with
> >   checks for whether the directory exists, but users who choose not to =
have
> >   /usr/share/doc installed on their system shouldn't be penalized by ge=
tting
> >   a broken bdb setup.  /usr/share/slapd would be much better.  (we alre=
ady
> >   have /usr/share/slapd/slapd.conf, after all.)  linking
> >   /usr/share/doc/slapd/examples/ to /usr/share/slapd/ is explicitly all=
owed,
> >   btw.

> The DB_CONFIG contains only comment lines currently. So it's not such a
> big deal currently anyway.

Well, 303057 seems like a good reason to ship it with more than just comment
lines.  Do you agree with the submitter on that point?  It's unfortunate
that BDB tuning can't be done more automatically, but if the defaults are
not sensible I think it's worth making an effort to providing defaults that
are *more* sensible even if they aren't perfect.

> > - alert_user -- if it's really that important that the user see it, use
> >   db_input critical, not db_input high; in any case, I don't think it's
> >   appropriate to spit a replacement message out on stderr just because =
the
> >   admin has configured debconf not to display the message.

> Erm, I was thinking about the debconf not installed possibility at that
> point - this /was/ called from preinst. I am not sure if it still is,
> the only point would be the slapcat of the old directories.

It seems that currently, it's not used from anywhere...

> > - In general, the maintainer scripts are a lot noisier than I think they
> >   should be, in keeping with Policy section 3.10.  I hope some of these
> >   echo's can be phased out as we gain more confidence that the maintain=
er
> >   scripts are doing the right thing.

> Absolutely not. I think it is rather important to see what's going on as
> this can take really long. And the information where the LDIFs are
> stored etc. are really very useful in case something goes wrong. I did
> not remember reading that maintainer scripts should be quiet and I think
> this should be discussed (on -policy, not here) as I don't think
> more information can be bad. And I yet have to see an admin watching the
> dpkg --configure -a output - that's what screen with logging is for. If
> something goes wrong I have the last few lines and go hunting for the
> cause from there.

Ok.  I admit, it does look rather elegant when upgrading slapd; not nearly
as bad as I thought it would based on the number of echo's present.

> > - # Better back up the config file in any case
> >   echo -n "  Backing up $SLAPD_CONF in `database_dumping_destdir`... " =
>&2
> >   backup_config_once
> >   echo done. >&2

> >   Hmm, well, once update_access_config_directives is fixed, I disagree.=
 :)

> The point is that in case anything goes wrong during the upgrade, I'd
> like to be able to just reinstall the old package and have everything
> still available. The new slapd.conf will not work with old slapd so I
> think it should be backed up. And, truth to be told, I guess without the
> message nobody will ever seek in /var/backups for that.

Yes, things can go wrong in the upgrade, but my point was that there's no
reason to back up the config unless we know we'll be changing it.  Since
every single function that will edit slapd.conf also calls
backup_config_once, there's no need to call it here as well, is there?  For
that matter, the "echo" command could be moved into the
if [ -z "$FLAG_CONFIG_BACKED_UP" ] part of backup_config_once.

Cheers,
--=20
Steve Langasek
postmodern programmer

--8NvZYKFJsRX2Djef
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCZi0WKN6ufymYLloRAoq4AJ95yTu50XlU4uprgqclCCLepwz5EQCeJcUM
lWodF4H22Dt7oCBRt6p9oI4=
=T+bY
-----END PGP SIGNATURE-----

--8NvZYKFJsRX2Djef--