Open Stack

Excerpts from Hayes, Graham's message of 2016-01-28 20:42:39 +0000:
> On 28/01/2016 20:16, Doug Hellmann wrote:
> > Excerpts from Hayes, Graham's message of 2016-01-28 19:17:54 +0000:
> >> On 28/01/2016 18:54, Doug Hellmann wrote:
> >>> Excerpts from Hayes, Graham's message of 2016-01-28 17:01:09 +0000:
> >>>> Recently I tried started to use oslo.versionedobjects for a project.
> >>>>
> >>>> After playing around with it for a while, I noticed I could set "this is
> >>>> not a uuid" as the value of a UUIDField.
> >>>>
> >>>> After making sure I made no mistakes - I looked at the underlying code,
> >>>> and found:[0]
> >>>>
> >>>> class UUID(FieldType):
> >>>>        @staticmethod
> >>>>        def coerce(obj, attr, value):
> >>>>            # FIXME(danms): We should actually verify the UUIDness here
> >>>>            return str(value)
> >>>>
> >>>> So, I went to implement this. [1]
> >>>>
> >>>> it quickly got -2'd as it would break Nova - so I went and implemented 2
> >>>> steps of a 4 step process to get this field working as it should.
> >>>>
> >>>> In the review I was told: [2]
> >>>>
> >>>> "... I think that if a project wants that level of enforcement it
> >>>> needs to land the project, not in the library. Libraries ideally should
> >>>> support all supported branches of OpenStack."
> >>>>
> >>>> Basically - if a project wants the UUIDField to act like a UUIDField,
> >>>> and not a field that str()'s all input, they should copy and paste code
> >>>> around.
> >>>
> >>> That's not actually the only option, as you point out below.
> >>>
> >>>>
> >>>> This is being blocked by the -2 until Nova's unit tests are fixed (just
> >>>> Nova's - we have no way of knowing how many projects assumed it was
> >>>> testing UUIDness and will break)
> >>>>
> >>>> The steps I had looked at doing was this:
> >>>>
> >>>> 1. Allow a "validate" flag on the Field __init__() defaulting to False.
> >>>> 1.1. This would allow current projects to continue as is, and projects
> >>>>         starting for the first time to do the right thing.
> >>>> 2. Deprecate the default value - issue a FutureWarning that it is
> >>>>       changing to True
> >>>> 3. Deprecate the option entirely.
> >>>> 4. Remove the option, and always validate.
> >>>>
> >>>> 3 & 4 are even optional if some projects want to keep using UUIDFields
> >>>> like StringFields.
> >>>>
> >>>> Currently the -2 still stands as the reviewer does not like the idea of
> >>>> a flag.
> >>>>
> >>>> What are the options for this now? If we are supposed to support all
> >>>> stable branches of all projects, this is the only option if it is going
> >>>> to merge in the next 2 years.
> >>>>
> >>>> Or we can create a ActuallyValidatingUUIDField?
> >>>
> >>> I like the idea of adding a new class, though maybe not the name
> >>> you've proposed here. Projects that want enforcement could use that
> >>> instead of the UUIDField. Then, as we're able to "fix" UUIDs in
> >>> other projects, the existing UUIDField class can be deprecated in
> >>> favor of the new one.
> >>
> >> This is my least favorite option - as it allows people to do the same
> >> as me, and wonder why their UUIDField is not working.
> >>
> >> The steps above allow for having a single Field type, and still allow
> >> current users to keep current usage.
> >
> > I'm not sure what's involved in rolling out the changes in all projects
> > that would eventually let us deprecate the argument. How many projects
> > are we talking about, and what sort of time frame?
> 
> I was assuming we would follow the standard deprecation model, so 1
> cycle for the default, and then another cycle for the option removal
> (which as I said in the initial email could be optional if enough
> people shout about it.)
> 
> It would be the same amount of projects that would have to switch to
> whatever new "ValidatingUUIDField" (or whatever it is called) after
> UUIDField is deprecated.
> 
> This all started when I saw nova or cinder using a UUIDField, and
> thought "that would be handy", implementing it locally, and then
> deciding that it might be useful in the lib so other projects could
> ensure they had valid UUIDs, and not assume that they did, based on
> the name of the class. That is the main reason for my objection to the
> new class.

OK, that all sounds reasonable.  Given that we have a way to deprecate
the old behavior with either approach, it seems like either would
work.

I know we have some projects (heat, I think?) that don't have UUIDs at
all. Are they using oslo.versionedobjects? I suppose we could move them
to a string field instead of a UUID field, instead of flipping the
enforcement flag on. Alternately, if we add a new class we wouldn't have
to ever actually deprecate the UUIDField class, though we might add a
warning to the docs that it isn't doing any validation and the new class
is preferred.

I'll be curious to see what Dan and Sean have to say when they catch up
on this thread.

> 
> >
> >>
> >>>>
> >>>> Also, olso seem to be very -2 heavy. This means that alternative views
> >>>> on the review are very unlikely. My question is what is the difference
> >>>> between a -1 and a -2 for oslo?
> >>
> >> As I said in another message - I think they are quite "sticky"
> >>
> >>>
> >>> I'm not sure the Oslo review team's patterns are the same as in some
> >>> other projects. We do tend to discuss things that have negative reviews.
> >>>
> >>> [1] https://review.openstack.org/270178
> >>>
> >>>>
> >>>> In designate we reserve -2 for things that will completely break our
> >>>> code, or is completely out of line for the project. (I would hope
> >>>> implementing a FIXME is not out of line for the project)
> >>>
> >>> No, but fixing it in a way that is known to break other projects
> >>> is. In this case, the change is known to break at least one project.
> >>> We have to be extremely careful with things that look like breaking
> >>> changes, since we can break *everyone* with a release. So I think
> >>> in this case the -2 is warranted.
> >>
> >> When I broke everything - yes it was. My point is when it stopped being
> >> breaking, it should no longer have the -2.
> >>
> >>> The other case you've pointed out on IRC, of the logging timezone
> >>> thing [1], is my -2. It was originally implemented as a breaking
> >>> change.  That has been fixed, but it still needs some discussion
> >>> on the mailing list, at least in part because I don't see the point
> >>> of the change.
> >>
> >> And my point is that while you have a -2, even if 2 other cores can see
> >> the point, they cannot merge it until your -2 is removed.
> >
> > Yes, I deliberately chose to use -2 for that very reason.
> >
> > Although I am certainly not the only contributor, I have been the
> > most recent principal developer on oslo.log. I feel quite strongly
> > that the change you have proposed is a bad idea, and not in the
> > best interests of the future direction for that library (as you say
> > above "out of line for the project"), if for no other reason than
> > it seems trivial to modify the timezone used for logging of a given
> > process without changing oslo.log code at all. I have indicated my
> > opinion using the review system in the way it is meant to be used.
> >
> > If at some point a significant portion of the core team is convinced
> > I'm wrong, or I become convinced that the patch is a better idea
> > than it seems right now, I'll remove the -2. In the mean time, maybe
> > we should talk about why you think the change is important and
> > useful.
> 
> I prefer config files to prepending env vars to startup commands for
> services.

Presumably there's a startup script of some sort. The variable can
be set there, and doesn't have to be set by prepending it to the
command (that was merely presented as an example to show how trivial
it is to change the value, without screwing up my shell).

> Files that I can render out using ansible / salt / chef / puppet / bash
> and have the whole config for a service in make my life easier than
> remembering the logging config is all in $file apart from the UTC
> logging change, which is in the init script.

Why would you be configuring the service to log in UTC if the default
timezone for the box is not UTC? Wouldn't that make it difficult to
combine log output from different services on the same host?

Doug

> 
> That's my 2 cents.
> 
> > Doug
> >
> > __________________________________________________________________________
> > OpenStack Development Mailing List (not for usage questions)
> > Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
>