[Openstack-devel] Splitting the nova package (and possibly others)?

[Roland Mas]

Thomas Goirand, 2012-11-20 10:06:06 +0800 :

> Hi Roland,
>
> I believe the main issue we are having here is that pgsql doesn't
> support remote configuration of users, and that dbconfig-common doesn't
> take this into account. But that's it. Please read further.

No.  That was the previous main issue.  The main issue now is that you
reverted something that was required and worked, without testing, based
on flawed assumptions, and for the wrong reasons.

> On 11/20/2012 12:24 AM, Roland Mas wrote:
>>   For the rest, I did some fact-checking, and it appears that:
>> - dbconfig-common is indeed able to create remote databases; however,
>>   that is hidden behind a low-priority Debconf question, so most users
>>   won't even see that;
>
> Yeah! Which is why we explicitly wrote in all of our templates: "You can
> change this setting later on by running "dpkg-reconfigure -plow
> <pkg-name>".

  Wrong.  The low-priority Debconf question is in dbconfig-common, so
unless you dpkg-reconfigure -plow *dbconfig-common*, you'll never see
the possibility to use remote databases.

>> - apparently it cannot be told to just *use* existing remote databases;
>> - it always tries to create the db.
>
> The important bit is that it wouldn't *fail* in the case of MySQL.
> Attempting to create a db which already exists isn't a problem, but it
> doesn't even do that. dbconfig-common sees the db already exists and
> doesn't creates it.

  Yes, and in the case of PostgreSQL, it requires the admin credentials,
to check that, so it's broken.

>>> But it does *not*! 
>> 
>>   It does.  Really, truly, giving the database server password to some
>> app/host/person does mean giving them full access to the database
>> server, even if only temporarily.  I know plenty of places where this
>> isn't going to happen (and that includes my own admittedly very small
>> network of servers and VMs).
>
> In this case, feel free to contribute patches against dbconfig-common
> itself. But IMO, this is not a sufficient reason to reimplement it in
> a different way, with new debconf templates asking the same thing
> dbconfig-common does.

  Which is why I agree the patch may be improved.  However, just
reverting it is *wrong*.

>>> dbconfig-common asks for the admin privileges for the time to create
>>> the db. Once it is done, it flushes it and never remembers it.
>> 
>>   I'm not sure it asks for it on every package upgrade, but even if it
>> only asks for it on the initial installation, that would be once too
>> many.
>
> Upon a package upgrade, it will ask to the user "do you want to upgrade
> the db with dbconfig-common?".
>
>> Note that there are many database servers (including the default
>> PostgreSQL installation in Debian) where the database admin can't even
>> connect remotely.
>
> I never tried PostgreSQL. Almost everyone uses MySQL with Openstack,
> anyway, so don't bother with PostgreSQL too much.

  You don't bother testing with two computers, you don't bother testing
anything else beyond MySQL, and yet you revert others' work.  Please
stop doing that.

>>>> So you need the remote db to be setup externally, and what I
>>>> did is just a way to help the admin of the compute node in configuring
>>>> the node by providing a UI to type the required parameters.
>>>
>>> Well, this UI exists already in dbconfig-common.
>> 
>>   Not really.  What exists in dbconfig-common is UI to create a
>> remote database, not a UI to use an existing remote one.  It'll
>> always try to create it with the database admin account, which means
>> it needs the admin credentials.
>
> But that's not a problem for us (apart from the fact that you need the
> admin credentials). Attempting to create the db, even if it exists
> already, will not destroy it.

  It is a problem for us, because you need the admin credentials.
Please *read*.

>>>> (That's exactly the same as what dbconfig-common policy says to do,
>>>> by the way: if we're not setting up the DB, then assume the local
>>>> admin will take care of that.) Now if you can coax dbconfig-common
>>>> into providing this UI, then fine, my change can be rewritten to use
>>>> this.  I believe this is going to need changes *in* dbconfig-common,
>>>> but I may be wrong.  In any case, as long as the results still work,
>>>> I don't really care.
>>>
>>> Well, the result does *not* work. 
>> 
>>   Actually, it does.  I checked before pushing last week, and I just
>> checked again, and it does work.
>
> What you told me what you've checked was your preseed scripts. That
> isn't the thing which I thought was broken.

  I also checked the non-preseed scripts.  Did you?  Did you actually
test anything before asserting that my code was wrong?

>>> Try to set debconf in non-interactive
>>> mode, and you'll have db_get ${PKG_NAME}/configure_db set to false, then
>>> it will go in your:
>>> db_input high ${PKG_NAME}/remote_db_* || true
>>> things, which will have no value (because running in non-interactive
>>> mode), and then fail.
>> 
>> Fail how?  The installation succeeds.  It doesn't configure a
>> database, apart from creating an empty SQLite one.  In other words, it
>> does exactly what it did before my changes ("let the admin handle the
>> database part").
>
> Well, your script is just calling db_input without even checking what's
> stored in the configuration file. If there was something there, with the
> info about how to connect to a remote PostgreSQL db, it would fail to do
> what the user expect.

  Then just tell me that, and I'll fix it.  But don't revert.

>> Except we now also have the ability to use a remote database, either
>> with interactive prompts or with preseeding, which we didn't have
>> before.
>
> I don't agree. We did have such option. 

  Until you provide working Debconf and dbconfig-common preseed files,
this is not true.

> The problem is this:
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=476946

  So we don't actually have what you claim we have.

> This bug has been opened since 2008. If there is something to change,
> it is in dbconfig-common, *not* in our packaging which depends on it.

  I'll be happy to use the infrastructure in dbconfig-common when it
appears (however, given that it's not appeared in more than four years,
I'm not holding my breath).  In the meantime, I provide a working
workaround.

[...]

>>   To summarize: contrary to what you claim, my changes:
>> - work (yes, I do test);
>> - made it possible to *use* a remote database (as opposed to creating
>>   it), which wasn't possible before;
>> - didn't remove anything;
>> - didn't break anything.
>
> Your code:
> - needs new debconf templates which are otherwise also available in
> dbconfig-common, and for which the l12n team will complain very loudly
> when they will translate (believe me, they will! if you don't trust this
> fact, ask them now, before attempting any reimplementation...).
> - removes the ability to be able to just reads everything from the
> config file (your code doesn't read it, but of course, this could be
> implemented...).

  So it could be improved.  Fair enough.

> - is attempting to re-invent something we have already.

  No, it implements something that was not previously available.

> I don't agree it wasn't possible to use a remote db before. It only
> breaks (according to you, as I didn't try myself) with PGSQL, since you
> can't remotely create users in this case. If it really wasn't working,
> please show me how dbconfig-common *destroys* the existing db while
> *re*-creating it. That's simply not the case, dbconfig-common checks if
> the db exists, and if it does, then it doesn't creates it. It even shows
> what it does on the console with nice echoes...

  You're getting it wrong again: it fails before even that, because it
doesn't have the credentials to try and create the DB.

> Now and again, if you think the current dbconfig-common is a problem
> (which it is, apparently, with PGSQL), then fix it in the
> dbconfig-common package.

  I agree this would be the best place for it to be fixed.  In the
meantime, a workaround is required.

>>   I'm not saying the changeset is perfect, and maybe it can be made
>> better.  But the current code is actually working and actually useful,
>> so I don't understand your objections to it.
>
> My objection is that it did work before. 

  For you.  On your one-node, MySQL, setup.

>> I just noticed you actually went and reverted anyway, without
>> waiting for the explanations you requested.  I'm not too happy
>> about that, as you can guess.  But whatever, I'll find something
>> else to work on.
>
> Of course I did! You left last Thursday evening with a commit on
> openstack-pkg-tools which requires to add new templates in at least 5
> packages (glance, keystone, nova, quantum, cinder...), 

  Wrong.  I carefully wrote my patch so it uses these templates if
they're present, but still works if not (and I'm certain it still works,
because I actually tested it).  Here's the relevant code, which you
didn't bother to read apparently:

,----
| db_get ${PKG_NAME}/remote_db_type || v=$?
|     if [ "$v" = 10 ] ; then
| 	# No such question yet, returning
| 	return
`----

> but you added these templates only in nova. As a consequence, 4
> packages were broken, 

  They were not broken.  Stop asserting things are broken when they were
tested and found working.

> and I couldn't afford to leave them in this state, since Loic
> explicitly asked me to go through the howto and stop doing anything
> else. I really had no choice.

  You could have tried before reverting on flawed assumptions.

> Now, please realize that I wasn't "too happy about that" either, when I
> saw that you implemented something I was strongly opposed to. The first
> time was with the apache configuration files for Horizon which you
> insisted to store in a patch file, and which I had to later fix with
> this commit:
> http://anonscm.debian.org/gitweb/?p=openstack/horizon.git;a=commitdiff;h=f8ee35931c773ec79fb824403c13af24d07c47f9
>
> I believe that this last patch will satisfy both of us (eg: it uses
> debian/openstack-dashboard-apache.install, and not BSD install, but it's
> not a quilt patch anymore). However, that's not the point. You
> completely ignore my requests to revert the changes making the
> configuration files a patch. I explicitly wrote on IRC "please revert",
> yet you didn't do anything for that one.
>
> Now, you are now accusing me of not taking your opinion into
> account?!?

  I'm just saying that you reverted working code and introduced a
regression with that.

> I have the opposite feeling... :/
> We shall try that this doesn't happen again and that we can both learn
> how to communicate better.

  The first thing about that would be to actually make sure of your
claims.  You say that there's UI in dbconfig-common to use a remote
database (which is wrong), you claim my code breaks packages (which it
doesn't), and so on.

> Going back to this dbconfig-common, we discussed these changes, I told
> you I strongly did not agree with them, and even strongly opposed to it,
> and you still went into that direction, broke 5 packages doing so (see
> just above), then went away for 4 days, saying that you had something
> else to deal with (which I respect, but you shouldn't break things and
> run...). In this kind of situation, you can expect that I would revert a
> patch (at least temporarily).
>
> Don't take me wrongly, I do believe there's a problem, and that it shall
> be fixed. But not this way.

  Okay.  Fix it your way then.  But fix it.

>> In the meantime, since you seem to appropriate this piece of code,
>> please provide a way to tell nova-common to use a remote database
>> server with Debconf.
>
> Yup. As written in our templates. For example (from
> debian/nova-common.templates):
>
>  You can change this setting later on by running "dpkg-reconfigure
>  -plow nova-common".

  It doesn't allow using a remote db without the database admin's
credentials, as explained before.

> It's not perfect, and that even can be considered a bug in
> dbconfig-common that it doesn't ask the right question on the first run,
> and doesn't support a remote PGSQL db correctly. If dbconfig-common has
> issues, then we shouldn't try to go around its problems, but we shall
> fix it.
>
> I would very welcome some changes in dbconfig-common by the way. Being
> able to use *standard* preseeding (and not be forced into write files in
> /etc/dbconfig-common/<package>) for example, would be very good.
>
> Probably, you should also try with MySQL, as the dbconfig-common might
> only be broken in the PGSQL case (I never tried it with pgsql, so I
> can't tell, and you said that with PGSQL, it wouldn't give root access
> remotely...).
>
>> Until you do that, those of us who actually try to run the
>> packages on several hosts are stuck.
>
> I don't agree with this statement. A dpkg-reconfigure -plow does work, I
> did use it before, it does ask for the host, and configures the db
> correctly. The problem we are having is that it doesn't work with
> pgsql,

  So it does not work.  Stop claiming it does.  If you do, back it with
facts, and provide a set of preseeds that I can use in my
non-interactive scripts.

> because pgsql doesn't have the feature to be administered remotely. If
> you wish to explicitly write a warning about this in our debconf
> templates, and/or patch dbconfig-common so that it will not do what's
> needed in this case, I support this idea!

  Honestly, I'd rather provide a working solution than documenting
failures.

Roland.
-- 
Roland Mas

...your network won't even know it's talking to a proxy, unless of
course, the proxy doesn't work.  -- in Linux 2.4 NAT HOWTO