Re: [openstack-dev] Adding DB migration items to the common review checklist

[Ben Nemec]

On 2013-12-18 22:08, Jay Pipes wrote:

On 12/18/2013 02:14 PM, Brant Knudson wrote:

Matt -

Could a test be added that goes through the models and checks these
things? Other projects could use this too.

Here's an example of a test that checks if the tables are all InnoDB:
http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/db/test_migrations.py?id=6e455cd97f04bf26bbe022be17c57e089cf502f4#n430


Actually, there's already work done for this.

https://review.openstack.org/#/c/42307/

I was initially put off by the unique constraint naming convention
(and it's still a little problematic due to constraint name length
constraints in certain RDBMS), but the patch above is an excellent
start.

Please show Svetlana's work a little review love :)


Big +1 to this.  I've been trying to review that patch series, but I 
don't have deep knowledge of the db stuff, so the more db folks that can 
weigh in the better. :-)


-Ben

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Jay Pipes]

On 12/18/2013 02:14 PM, Brant Knudson wrote:

Matt -

Could a test be added that goes through the models and checks these
things? Other projects could use this too.

Here's an example of a test that checks if the tables are all InnoDB:
http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/db/test_migrations.py?id=6e455cd97f04bf26bbe022be17c57e089cf502f4#n430


Actually, there's already work done for this.

https://review.openstack.org/#/c/42307/

I was initially put off by the unique constraint naming convention (and 
it's still a little problematic due to constraint name length 
constraints in certain RDBMS), but the patch above is an excellent start.


Please show Svetlana's work a little review love :)

Best,
-jay


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Matt Riedemann]

On 12/18/2013 2:11 PM, Dan Prince wrote:



- Original Message -

From: "Matt Riedemann" 
To: "OpenStack Development Mailing List (not for usage questions)" 

Sent: Wednesday, December 18, 2013 12:27:49 PM
Subject: [openstack-dev] Adding DB migration items to the common review 
checklist

I've seen this come up a few times in reviews and was thinking we should
put something in the general review checklist wiki for it [1].

Basically I have three things I'd like to have in the list for DB
migrations:

1. Unique constraints should be named. Different DB engines and
SQLAlchemy dialects automatically name the constraint their own way,
which can be troublesome for universal migrations. We should avoid this
by enforcing that UCs are named when they are created. This means not
using the unique=True arg in UniqueConstraint if the name arg isn't
provided.

2. Foreign keys should be named for the same reasons in #1.

3. Foreign keys shouldn't be created against nullable columns. Some DB
engines don't allow unique constraints over nullable columns and if you
can't create the unique constraint you can't create the foreign key, so
we should avoid this. If you need the FK, then the pre-req is to make
the target column non-nullable. Think of the instances.uuid column in
nova for example.

Unless anyone has a strong objection to this, I'll update the review
checklist wiki with these items.


No objection to these.

One possible addition would be to make sure that migrations stand on their own 
as much as possible. Code sharing, while good in many cases, can bite you in DB 
migrations because fixing a bug in the shared code may change the behavior of 
an old (released) migration. So by sharing migration code it then can  become 
easier to break upgrades paths down the road. If we make some exceptions to 
this rule with nova.db.sqlalchemy we need to be very careful that we don't 
change the behavior in those functions. Automated tests help here too.

Dan


Not sure if this is pure coincidence, but case in point:

https://review.openstack.org/#/c/62965/





[1] https://wiki.openstack.org/wiki/ReviewChecklist

--

Thanks,

Matt Riedemann


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



--

Thanks,

Matt Riedemann


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Matt Riedemann]

On 12/18/2013 1:14 PM, Brant Knudson wrote:

Matt -

Could a test be added that goes through the models and checks these
things? Other projects could use this too.

Here's an example of a test that checks if the tables are all InnoDB:
http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/db/test_migrations.py?id=6e455cd97f04bf26bbe022be17c57e089cf502f4#n430

- Brant



Brant, I could see automating #3 since you could trace the FK to the UC 
and then check if the columns in the UC are nullable or not, but I'm not 
sure how easy it would be to generically test 1 and 2 because we don't 
have strict naming conventions on UC/FK names as far as I know, but I 
guess we could start enforcing that with a test, and whitelist any 
existing UC/FK names that don't fit the new convention.


Thoughts on that or other ideas how to automate checking for this?





On Wed, Dec 18, 2013 at 11:27 AM, Matt Riedemann
mailto:mrie...@linux.vnet.ibm.com>> wrote:

I've seen this come up a few times in reviews and was thinking we
should put something in the general review checklist wiki for it [1].

Basically I have three things I'd like to have in the list for DB
migrations:

1. Unique constraints should be named. Different DB engines and
SQLAlchemy dialects automatically name the constraint their own way,
which can be troublesome for universal migrations. We should avoid
this by enforcing that UCs are named when they are created. This
means not using the unique=True arg in UniqueConstraint if the name
arg isn't provided.

2. Foreign keys should be named for the same reasons in #1.

3. Foreign keys shouldn't be created against nullable columns. Some
DB engines don't allow unique constraints over nullable columns and
if you can't create the unique constraint you can't create the
foreign key, so we should avoid this. If you need the FK, then the
pre-req is to make the target column non-nullable. Think of the
instances.uuid column in nova for example.

Unless anyone has a strong objection to this, I'll update the review
checklist wiki with these items.

[1] https://wiki.openstack.org/__wiki/ReviewChecklist


--

Thanks,

Matt Riedemann


_
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.__org

http://lists.openstack.org/__cgi-bin/mailman/listinfo/__openstack-dev 





___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



--

Thanks,

Matt Riedemann


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Dan Prince]

- Original Message -
> From: "Matt Riedemann" 
> To: "OpenStack Development Mailing List (not for usage questions)" 
> 
> Sent: Wednesday, December 18, 2013 12:27:49 PM
> Subject: [openstack-dev] Adding DB migration items to the common review   
> checklist
> 
> I've seen this come up a few times in reviews and was thinking we should
> put something in the general review checklist wiki for it [1].
> 
> Basically I have three things I'd like to have in the list for DB
> migrations:
> 
> 1. Unique constraints should be named. Different DB engines and
> SQLAlchemy dialects automatically name the constraint their own way,
> which can be troublesome for universal migrations. We should avoid this
> by enforcing that UCs are named when they are created. This means not
> using the unique=True arg in UniqueConstraint if the name arg isn't
> provided.
> 
> 2. Foreign keys should be named for the same reasons in #1.
> 
> 3. Foreign keys shouldn't be created against nullable columns. Some DB
> engines don't allow unique constraints over nullable columns and if you
> can't create the unique constraint you can't create the foreign key, so
> we should avoid this. If you need the FK, then the pre-req is to make
> the target column non-nullable. Think of the instances.uuid column in
> nova for example.
> 
> Unless anyone has a strong objection to this, I'll update the review
> checklist wiki with these items.

No objection to these.

One possible addition would be to make sure that migrations stand on their own 
as much as possible. Code sharing, while good in many cases, can bite you in DB 
migrations because fixing a bug in the shared code may change the behavior of 
an old (released) migration. So by sharing migration code it then can  become 
easier to break upgrades paths down the road. If we make some exceptions to 
this rule with nova.db.sqlalchemy we need to be very careful that we don't 
change the behavior in those functions. Automated tests help here too.

Dan

> 
> [1] https://wiki.openstack.org/wiki/ReviewChecklist
> 
> --
> 
> Thanks,
> 
> Matt Riedemann
> 
> 
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Michael Still]

I don't have a problem with any of these requirements, but I'd like to
explore automating the checks. Would it be possible to write a unit
test that verified this for all migrations? Then we don't need to add
it to the checklist...

Michael

On Thu, Dec 19, 2013 at 4:27 AM, Matt Riedemann
 wrote:
> I've seen this come up a few times in reviews and was thinking we should put
> something in the general review checklist wiki for it [1].
>
> Basically I have three things I'd like to have in the list for DB
> migrations:
>
> 1. Unique constraints should be named. Different DB engines and SQLAlchemy
> dialects automatically name the constraint their own way, which can be
> troublesome for universal migrations. We should avoid this by enforcing that
> UCs are named when they are created. This means not using the unique=True
> arg in UniqueConstraint if the name arg isn't provided.
>
> 2. Foreign keys should be named for the same reasons in #1.
>
> 3. Foreign keys shouldn't be created against nullable columns. Some DB
> engines don't allow unique constraints over nullable columns and if you
> can't create the unique constraint you can't create the foreign key, so we
> should avoid this. If you need the FK, then the pre-req is to make the
> target column non-nullable. Think of the instances.uuid column in nova for
> example.
>
> Unless anyone has a strong objection to this, I'll update the review
> checklist wiki with these items.
>
> [1] https://wiki.openstack.org/wiki/ReviewChecklist
>
> --
>
> Thanks,
>
> Matt Riedemann
>
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



-- 
Rackspace Australia

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Adding DB migration items to the common review checklist

[Brant Knudson]

Matt -

Could a test be added that goes through the models and checks these things?
Other projects could use this too.

Here's an example of a test that checks if the tables are all InnoDB:
http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/db/test_migrations.py?id=6e455cd97f04bf26bbe022be17c57e089cf502f4#n430

- Brant




On Wed, Dec 18, 2013 at 11:27 AM, Matt Riedemann  wrote:

> I've seen this come up a few times in reviews and was thinking we should
> put something in the general review checklist wiki for it [1].
>
> Basically I have three things I'd like to have in the list for DB
> migrations:
>
> 1. Unique constraints should be named. Different DB engines and SQLAlchemy
> dialects automatically name the constraint their own way, which can be
> troublesome for universal migrations. We should avoid this by enforcing
> that UCs are named when they are created. This means not using the
> unique=True arg in UniqueConstraint if the name arg isn't provided.
>
> 2. Foreign keys should be named for the same reasons in #1.
>
> 3. Foreign keys shouldn't be created against nullable columns. Some DB
> engines don't allow unique constraints over nullable columns and if you
> can't create the unique constraint you can't create the foreign key, so we
> should avoid this. If you need the FK, then the pre-req is to make the
> target column non-nullable. Think of the instances.uuid column in nova for
> example.
>
> Unless anyone has a strong objection to this, I'll update the review
> checklist wiki with these items.
>
> [1] https://wiki.openstack.org/wiki/ReviewChecklist
>
> --
>
> Thanks,
>
> Matt Riedemann
>
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] Adding DB migration items to the common review checklist

[Matt Riedemann]

I've seen this come up a few times in reviews and was thinking we should 
put something in the general review checklist wiki for it [1].


Basically I have three things I'd like to have in the list for DB 
migrations:


1. Unique constraints should be named. Different DB engines and 
SQLAlchemy dialects automatically name the constraint their own way, 
which can be troublesome for universal migrations. We should avoid this 
by enforcing that UCs are named when they are created. This means not 
using the unique=True arg in UniqueConstraint if the name arg isn't 
provided.


2. Foreign keys should be named for the same reasons in #1.

3. Foreign keys shouldn't be created against nullable columns. Some DB 
engines don't allow unique constraints over nullable columns and if you 
can't create the unique constraint you can't create the foreign key, so 
we should avoid this. If you need the FK, then the pre-req is to make 
the target column non-nullable. Think of the instances.uuid column in 
nova for example.


Unless anyone has a strong objection to this, I'll update the review 
checklist wiki with these items.


[1] https://wiki.openstack.org/wiki/ReviewChecklist

--

Thanks,

Matt Riedemann


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev