Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Morales, Victor]

I just want to add my two cents on this.  Several months ago I asked to nova 
community about a bug that they decided to don’t implement, this bug was 
suggesting to add a validation to String OVO field[1] to check if the length of 
the columns has been changed, the answer was that they considered hard to track 
those DB changes.  I mentioned this because maybe we can explore this other 
alternative, just in case that we don’t have the same type of restrictions that 
nova has. By other hand, if our goal is ensuring that integrity of the class 
maybe we can validate them as we do in OVO, using a hash code.

[1] 
https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/fields.py#L254-L265





On 10/15/16, 2:36 PM, "Henry Gessau"  wrote:

>Hi neutrinos,
>
>In Neutron many attributes are stored in database fields. The size of these
>fields therefore determines the maximum length of the attribute values.
>
>I would like to get some consistency in place around how we define the
>constants and where they are used. Here are my thoughts...
>
>
>1. Raw sizes in alembic migrations
>
>In the alembic migrations which build the DB schema, we should use the raw
>number value of the field size.
>
>2. FOO_FIELD_SIZE in the sqlalchemy models
>
>In the sqlalchemy models, we should use the _FIELD_SIZE constants
>defined in neutron_lib/db/constants.py
>
>3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like FOO_MAX_LEN)
>based on FOO_FIELD_SIZE.
>
>
>"Why raw numbers in alembic migrations?", you may ask. Well, we have tests
>that verify that the models match the schema generated by migrations. If both
>the models and the migrations use the constants then the tests would not
>detect if a patch changes the constant value.
>
>By using raw numbers in migrations, together with our rule of not allowing
>changes to existing migrations, we allow the tests to detect and fail on any
>attempt to alter a FIELD_SIZE constant.
>
>Let me know if this makes sense or if you think it's a terrible idea.
>
>If there are no objections, I intend to submit a patch or patches to:
> - replace constants with numbers in existing migrations
> - ensure all models use the appropriate constants
>
>Existing code uses FOO_MAX_LEN in a lot of places. In most of these places it
>would make sense to simply switch to using FOO_FIELD_SIZE. However, some code
>may be quite far removed from the DB and would look better by sticking to
>FOO_MAX_LEN. I added item 3. above to allow for that.
>
>
>__
>OpenStack Development Mailing List (not for usage questions)
>Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Anna Taraday]

Ihar, we all adults here, but this does not save us from making silly
mistakes sometimes :(
I think that if we can protect from possible mistakes - lets do this, as
messing up with size of fields is a common issue.

On Mon, Oct 17, 2016 at 2:16 PM Ihar Hrachyshka  wrote:

> Henry Gessau  wrote:
>
> > Anna Taraday  wrote:
> >> Henry, thanks for taking care of this!
> >>
> >> In my opinion, it is just safe to use raw values in migration, because
> >> migration is a strict point in time.
> >>
> >> I remember how many patches I send in havana in Neutron for fixing
> >> synchronization issues. Usage constants everywhere can be good in this
> >> case,
> >> but ModelMigrationSyc did such check of this for us already.
> >>
> >> If we want to have constants everywhere, we should guarantee that they
> are
> >> unchanged - have test in neutron-lib which verifies their values.
> >
> > Yes, we could have some tests, although a patch changing a constant would
> > probably also have a change to the test. :) We might need to also have a
> > prominent "Warning! Do not change this test!" comment for each test.
>
> I actually think (or hope) everyone is adult here, and will be able to
> block a patch changing a constant, even if there is no unit test written,
> especially if we put such a warning in a comment above the constants we
> know are especially unsafe to touch.
>
> But if we are still think that it’s better safe than sorry, we could have
> some tests to make that even more explicit. It just seems like a waste of
> cpu cycles when any careful reviewer can spot such an issue.
>
> Anyhow, whatever works for the goal, that I think is: making constants safe
> to use in all relevant contexts, including alembic.
>
> Ihar
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-- 
Regards,
Ann Taraday
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Ihar Hrachyshka]

Henry Gessau  wrote:


Anna Taraday  wrote:

Henry, thanks for taking care of this!

In my opinion, it is just safe to use raw values in migration, because
migration is a strict point in time.

I remember how many patches I send in havana in Neutron for fixing
synchronization issues. Usage constants everywhere can be good in this  
case,

but ModelMigrationSyc did such check of this for us already.

If we want to have constants everywhere, we should guarantee that they are
unchanged - have test in neutron-lib which verifies their values.


Yes, we could have some tests, although a patch changing a constant would
probably also have a change to the test. :) We might need to also have a
prominent "Warning! Do not change this test!" comment for each test.


I actually think (or hope) everyone is adult here, and will be able to  
block a patch changing a constant, even if there is no unit test written,  
especially if we put such a warning in a comment above the constants we  
know are especially unsafe to touch.


But if we are still think that it’s better safe than sorry, we could have  
some tests to make that even more explicit. It just seems like a waste of  
cpu cycles when any careful reviewer can spot such an issue.


Anyhow, whatever works for the goal, that I think is: making constants safe  
to use in all relevant contexts, including alembic.


Ihar

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Henry Gessau]

Anna Taraday  wrote:
> Henry, thanks for taking care of this!
> 
> In my opinion, it is just safe to use raw values in migration, because
> migration is a strict point in time.
> 
> I remember how many patches I send in havana in Neutron for fixing
> synchronization issues. Usage constants everywhere can be good in this case,
> but ModelMigrationSyc did such check of this for us already.
> 
> If we want to have constants everywhere, we should guarantee that they are
> unchanged - have test in neutron-lib which verifies their values. 

Yes, we could have some tests, although a patch changing a constant would
probably also have a change to the test. :) We might need to also have a
prominent "Warning! Do not change this test!" comment for each test.

> 
> 
> On Sat, Oct 15, 2016 at 10:41 PM Henry Gessau  > wrote:
> 
> Hi neutrinos,
> 
> In Neutron many attributes are stored in database fields. The size of 
> these
> fields therefore determines the maximum length of the attribute values.
> 
> I would like to get some consistency in place around how we define the
> constants and where they are used. Here are my thoughts...
> 
> 
> 1. Raw sizes in alembic migrations
> 
> In the alembic migrations which build the DB schema, we should use the raw
> number value of the field size.
> 
> 2. FOO_FIELD_SIZE in the sqlalchemy models
> 
> In the sqlalchemy models, we should use the _FIELD_SIZE constants
> defined in neutron_lib/db/constants.py
> 
> 3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like 
> FOO_MAX_LEN)
> based on FOO_FIELD_SIZE.
> 
> 
> "Why raw numbers in alembic migrations?", you may ask. Well, we have tests
> that verify that the models match the schema generated by migrations. If 
> both
> the models and the migrations use the constants then the tests would not
> detect if a patch changes the constant value.
> 
> By using raw numbers in migrations, together with our rule of not allowing
> changes to existing migrations, we allow the tests to detect and fail on 
> any
> attempt to alter a FIELD_SIZE constant.
> 
> Let me know if this makes sense or if you think it's a terrible idea.
> 
> If there are no objections, I intend to submit a patch or patches to:
>  - replace constants with numbers in existing migrations
>  - ensure all models use the appropriate constants
> 
> Existing code uses FOO_MAX_LEN in a lot of places. In most of these 
> places it
> would make sense to simply switch to using FOO_FIELD_SIZE. However, some 
> code
> may be quite far removed from the DB and would look better by sticking to
> FOO_MAX_LEN. I added item 3. above to allow for that.


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Henry Gessau]

Ihar Hrachyshka  wrote:
> Henry Gessau  wrote:
> 
>> Hi neutrinos,
>>
>> In Neutron many attributes are stored in database fields. The size of these
>> fields therefore determines the maximum length of the attribute values.
>>
>> I would like to get some consistency in place around how we define the
>> constants and where they are used. Here are my thoughts...
>>
>>
>> 1. Raw sizes in alembic migrations
>>
>> In the alembic migrations which build the DB schema, we should use the raw
>> number value of the field size.
>>
>> 2. FOO_FIELD_SIZE in the sqlalchemy models
>>
>> In the sqlalchemy models, we should use the _FIELD_SIZE constants
>> defined in neutron_lib/db/constants.py
>>
>> 3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like  
>> FOO_MAX_LEN)
>> based on FOO_FIELD_SIZE.
>>
>>
>> "Why raw numbers in alembic migrations?", you may ask. Well, we have tests
>> that verify that the models match the schema generated by migrations. If  
>> both
>> the models and the migrations use the constants then the tests would not
>> detect if a patch changes the constant value.
>>
>> By using raw numbers in migrations, together with our rule of not allowing
>> changes to existing migrations, we allow the tests to detect and fail on  
>> any
>> attempt to alter a FIELD_SIZE constant.
>>
>> Let me know if this makes sense or if you think it's a terrible idea.
> 
> Not sure. I mean, if you envision that a ‘constant’ value maintained in  
> neutron-lib may be changed in the future, then it’s not safe to use it  
> anywhere, both in models and alembic scripts.
> 
> But I believe those lib constants are supposed to be unchanged, and  
> reviewers of the library should understand that; in which case we should be  
> safe to use them in alembic scripts too.
> 
> Is it that we don’t trust neutron-lib core reviewers to follow the rule?

OK, I suppose using *_FIELD_SIZE constants from neutron-lib in the alembic
scripts is safe enough.

I began thinking about this problem before I added field sizes to neutron-lib,
and it was not a matter of trust. It was more about having a mechanism to
detect a problem if someone tries to change, say, FILE_NAME_MAX_LEN from 16 to
80 without realizing it is related to a DB field. That's a hypothetical
example, but we have many constants and I did not want to count on humans to
detect every impacting case.


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Anna Taraday]

Henry, thanks for taking care of this!

In my opinion, it is just safe to use raw values in migration, because
migration is a strict point in time.

I remember how many patches I send in havana in Neutron for fixing
synchronization issues. Usage constants everywhere can be good in this
case, but ModelMigrationSyc did such check of this for us already.

If we want to have constants everywhere, we should guarantee that they are
unchanged - have test in neutron-lib which verifies their values.


On Sat, Oct 15, 2016 at 10:41 PM Henry Gessau  wrote:

Hi neutrinos,

In Neutron many attributes are stored in database fields. The size of these
fields therefore determines the maximum length of the attribute values.

I would like to get some consistency in place around how we define the
constants and where they are used. Here are my thoughts...


1. Raw sizes in alembic migrations

In the alembic migrations which build the DB schema, we should use the raw
number value of the field size.

2. FOO_FIELD_SIZE in the sqlalchemy models

In the sqlalchemy models, we should use the _FIELD_SIZE constants
defined in neutron_lib/db/constants.py

3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like
FOO_MAX_LEN)
based on FOO_FIELD_SIZE.


"Why raw numbers in alembic migrations?", you may ask. Well, we have tests
that verify that the models match the schema generated by migrations. If
both
the models and the migrations use the constants then the tests would not
detect if a patch changes the constant value.

By using raw numbers in migrations, together with our rule of not allowing
changes to existing migrations, we allow the tests to detect and fail on any
attempt to alter a FIELD_SIZE constant.

Let me know if this makes sense or if you think it's a terrible idea.

If there are no objections, I intend to submit a patch or patches to:
 - replace constants with numbers in existing migrations
 - ensure all models use the appropriate constants

Existing code uses FOO_MAX_LEN in a lot of places. In most of these places
it
would make sense to simply switch to using FOO_FIELD_SIZE. However, some
code
may be quite far removed from the DB and would look better by sticking to
FOO_MAX_LEN. I added item 3. above to allow for that.


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 
Regards,
Ann Taraday
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Neutron] Database field sizes and attribute "MAX_LEN" constants

[Ihar Hrachyshka]

Henry Gessau  wrote:


Hi neutrinos,

In Neutron many attributes are stored in database fields. The size of these
fields therefore determines the maximum length of the attribute values.

I would like to get some consistency in place around how we define the
constants and where they are used. Here are my thoughts...


1. Raw sizes in alembic migrations

In the alembic migrations which build the DB schema, we should use the raw
number value of the field size.

2. FOO_FIELD_SIZE in the sqlalchemy models

In the sqlalchemy models, we should use the _FIELD_SIZE constants
defined in neutron_lib/db/constants.py

3. Everywhere else, use FOO_FIELD_SIZE, or another constant (like  
FOO_MAX_LEN)

based on FOO_FIELD_SIZE.


"Why raw numbers in alembic migrations?", you may ask. Well, we have tests
that verify that the models match the schema generated by migrations. If  
both

the models and the migrations use the constants then the tests would not
detect if a patch changes the constant value.

By using raw numbers in migrations, together with our rule of not allowing
changes to existing migrations, we allow the tests to detect and fail on  
any

attempt to alter a FIELD_SIZE constant.

Let me know if this makes sense or if you think it's a terrible idea.


Not sure. I mean, if you envision that a ‘constant’ value maintained in  
neutron-lib may be changed in the future, then it’s not safe to use it  
anywhere, both in models and alembic scripts.


But I believe those lib constants are supposed to be unchanged, and  
reviewers of the library should understand that; in which case we should be  
safe to use them in alembic scripts too.


Is it that we don’t trust neutron-lib core reviewers to follow the rule?

Ihar

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev