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

[Torsten Landschoff]

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

Hi Steve,=20

On Sun, Apr 17, 2005 at 02:41:35PM -0700, Steve Langasek wrote:
> I had sent this message to the pkg-openldap-devel mailing list, and only
> afterwards learned that none of the openldap maintainers were subscribed =
to
> that list!  Did something happen to break people's subscriptions to it?

Good question, I created the list ages ago and subscribed (with the
intention of moving the old list which only catches spam nowadays) and
then forgot about it since I was essentially the only person still
working on OpenLDAP.

> Anyway, comments on the below are welcome.  Stephen Frost has already
> offered some feedback after I forwarded it to him separately; he's also n=
ow
> on the alioth list, so if discussion could continue there, that'd be grea=
t.

Subscribing now.

> I'm looking over the current slapd maintainer scripts, and I'm seeing a
> number of issues that I think are bugs that I'd like to discuss here befo=
re
> I start making changes and committing them to svn; apologies for the long
> mail, but this is something of a brain dump of all the issues I'm seeing.

No problem with that. Most of the problem popped up in my brain as well
but I never get the time to fix everything that I don't like :(

> First, there's bug #304840, which is that 2.2.23-2 tries to load an ldif
> unconditionally, even if the preinst had no reason to write one out.
> Obviously we would want to keep the dumping/loading in sync, so that one
> isn't happening without the other; but I think the larger issue in trying=
 to

The idea was to dump the database before upgrading in case something
fails after upgrading. Of course everybody should keep backups but you
know the reality. It seems that even Debian has not real backup
infrastructure in place (thinking of the gluck breakage).

> 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

>   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 disk
> 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.

> redundant ldifs.  I always keep my own LDIFs around for backup purposes; =
why
> would the package need to create another one for me?

It doesn't. But there are other admins not thinking like that and not
keeping backups. But anyway, I made my point I think.

> If there is a use case for always creating an LDIF, I think that we at le=
ast
> shouldn't reload the database just for this reason.  Thus, I think the ch=
eck
> in the postinst should be:
>=20
>   if database_format_changed; then
>   	move_incompatible_databases_away
>   	load_databases
>   fi

Fully agreed. I don't know why I didn't write this in the first place.

> which should be fairly idempotent actually, since
> move_incompatible_databases_away checks for an empty source directory and
> moves on, while bombing out if there are files at both the source and tar=
get
> locations.

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.

> However, this code doesn't deal with the case when the user answered to
> 'never' dump databases, because then the preinst does:
>=20
> dump_databases() {                                                      #=
 {{{
> # If the user wants us to dump the databases they are dumped to the
> # configured directory.
>=20
>         local db suffix file dir failed
>=20
>         database_dumping_enabled || return 0
>=20
> 	[...]
> }
>=20
> and thus returns success from a function that hasn't dumped anything simp=
ly
> because the user said not to.  Why would we want to allow the user to do
> this?  We *know* that the user will end up with an unusable install if
> upgrading from an incompatible old version, and that if they do have an L=
DIF
> for backup purposes, we don't know where it is.  They *may* (if they don't
> have a good backup policy) be left with no LDIF, and no tools on the syst=
em
> that they can use to create one.  Does it make sense to let the user break
> the package this way?  (It will break -- the best case scenario is that
> the upgrade will die in the postinst, and they'll have to dig through the
> maintainer scripts to see where they have to put their LDIF to have it
> auto-loaded.)

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.

> I think the best thing here is to drop the slapd/dump_database question; =
the
> second-best thing is to no longer give "never" as an option.  Since this
> question is only asked at medium priority anyway, it's pretty clear to me
> that having control over this isn't actually all that important.  If users
> need to have the option to not do a dump, then I think this really needs =
to
> be turned into an "abort upgrade" option instead.

What if I already have an LDIF? Or don't have the disk space available
on the same machine? Okay, you could still NFS-mount the storage but if
I am perfectly able to keep my own backups why should the maintainer
script force it down my throat? (To turn your argument back on you ;-)

> Dropping the question would also partially solve another problem with the
> 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 and
> *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 undecla=
red

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

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.

> pre-depends on debconf here, which it would be nice to get away from, IMH=
O.
> Alternatively, it is valid to Pre-Depend on debconf -- there are 71 packa=
ges
> in unstable which already do so.
>=20
> Other minor comments:
>=20
> - database_dumping_destdir pulls a value from debconf; this value could
>   change between the preinst and the postinst.  Should that be handled
>   somehow?

I think the current behaviour is quite okay - the LDIF is not there
which triggers an error during postinst.=20

> - /usr/share/doc/slapd/examples/DB_CONFIG -- I realize this is wrapped wi=
th
>   checks for whether the directory exists, but users who choose not to ha=
ve
>   /usr/share/doc installed on their system shouldn't be penalized by gett=
ing
>   a broken bdb setup.  /usr/share/slapd would be much better.  (we already
>   have /usr/share/slapd/slapd.conf, after all.)  linking
>   /usr/share/doc/slapd/examples/ to /usr/share/slapd/ is explicitly allow=
ed,
>   btw.

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

> - 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.

> - 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 maintainer
>   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.

> - update_access_config_directives -- eew, no.  Torsten, I've already talk=
ed
>   to you about this, I'll probably cook up a replacement this weekend. :)

already commited, looks fine. Quite simple solution in fact. Not sure if
it is correct in all cases but I wasn't sure before either - not to
mention that users reported problems with that function.

> - # Better back up the config file in any case
>   echo -n "  Backing up $SLAPD_CONF in =04atabase_dumping_destdir... " >&2
>   backup_config_once
>   echo done. >&2
>=20
>   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.

Greetings

	Torsten

--Nq2Wo0NMKNjxTN9z
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)

iD4DBQFCYuYgdQgHtVUb5EcRApxhAJiC0WUSciELNzM52yeoLI/Co5RcAJ9CEZZG
AS7vsngr67DOAz7uDRJNGQ==
=bBN4
-----END PGP SIGNATURE-----

--Nq2Wo0NMKNjxTN9z--