Open Stack

Excerpts from Jamie Lennox's message of 2016-07-20 10:28:29 +1000:
> On 20 July 2016 at 00:06, Joshua Harlow <harlowja at fastmail.com> wrote:
> 
> > Hayes, Graham wrote:
> >
> >> On 18/07/16 22:27, Ronald Bradford wrote:
> >>
> >>> Hi All,
> >>>
> >>> For Oslo libraries we ensure that API's are backward compatible for 1+
> >>> releases.
> >>> When an Oslo API adds a new class attribute (as in this example of
> >>> is_admin_project and 4 other attributes) added to Oslo Context in
> >>> 2.6.0,  these are added to ensure this API is also forward compatible
> >>> with existing project code for any contract with the base class
> >>> instantiation or manipulation.
> >>>
> >>
> >> Which projects is this run against?
> >>
> >> The issue seen is presently Nova specific (as other projects can
> >>> utilize 2.6.0) and it is related to projects that sub-class
> >>> oslo.context, and how unit tests are written for using class
> >>> parameters.  Ideally, to implement using oslo.context correctly
> >>> OpenStack projects should:
> >>>
> >>
> >> Designate also had to make a quick change to support 2.6.0.
> >>
> >> We were lucky as it was noticed by the RDO builds, which had pulled in
> >> 2.6.0 before the requirements update was proposed, so it did not break
> >> our gate.
> >>
> >> I just did a quick search and there is a few projects that hardcoded
> >> this, like we did.
> >>
> >
> > Ya, that's bad, nothing in the docs of the to_dict API say what to even
> > compare against (or the keys produced), so I'm pretty sure anyone doing
> > this is setting themselves up for future failure and fragile software.
> >
> 
> Can you post that list?
> 
> >
> >
> >> * Not perform direct dictionary to dictionary comparisons with the
> >>> to_dict() method as this does not support when new attributes at
> >>> added. Two patches (one to nova) address this in offending projects
> >>> [5][6]
> >>> * Unit tests should focus on attributes specific to the sub-classed
> >>> signature, e.g. [7].  Oslo context provides an extensive set of unit
> >>> tests for base class functionality. This is a wish list item for
> >>> projects to implement.
> >>>
> >>> The to_dict() method exists as a convenience method only and is not an
> >>> API contract. The resulting set of keys should be used accordingly.
> >>> This is why there is no major release version.
> >>>
> >>
> >> How are developers supposed to know that?
> >>
> >
> > So we (in oslo) can (and ideally will) make this better but when the API
> > doesn't itself tell you what keys are produced or what the values of those
> > keys are then it should be pretty obvious to u (the library user) that u
> > can not reliably do dictionary comparisons (because how do u know what to
> > compare against when the docs don't state that?). I suppose people are
> > 'reverse engineering the dict' by just looking at the code but that's also
> > not so great...
> >
> 
> I think the obvious and only thing you should expect from the to_dict
> method is that it can be reversed by the from_dict method. Subclasses can
> then make small modifications to those methods to add additional
> information as appropriate. There is a bit of a problem in this with the
> way subclasses are done that is fixed in [1] but it does not affect any
> existing code.
> 
> We realize that the to_dict method is subclassed by a lot of services and
> affects RPC and so contexts must be serializable between different versions
> of the library so we will not modify existing to_dict values but as
> mentioned writing your tests to assume this will never be added to sets us
> up for these problems.

Exactly. It breaks the layering between the subclass and the base class.
Unit tests in nova should not be testing functionality defined in a
library, no matter if that library is from Oslo or anywhere else. In
this case, the contract is as Jamie describes above (the return value of
to_dict can be passed to from_dict to get a new context instance). The
contents of the dict are not meant to be used as-is outside of the
context class. The subclass should not be asserting anything about the
contents provided by the base class.

> In this case oslo.context was largely extracted from nova and so the
> fragile tests make sense and should therefore be fixed - but the oslo
> change does not constitute a breaking API change.

Right. Ronald's change should address this pretty cleanly by only
looking at the expected values defined by the subclass.

Doug

> 
> 
> [1] https://review.openstack.org/#/c/341250/
> 
> >
> >
> >> This kind of feels like semantics. This was an external API that changed
> >> and as a result should have been a major version.
> >>
> >
> > I think this is where it gets a little bit into as u said, semantics, but
> > the semantics IMHO are important here because it affects the ability of
> > oslo.context to move forward & change.
> >
> > I suppose we should/could just put a warning on this method like I did in
> > taskflow (for something similar) @
> > https://github.com/openstack/taskflow/blob/master/taskflow/engines/base.py#L71
> > to denote that nothing in the dict that is returned can be guaranteed to
> > always be the same.
> >
> >
> >
> >> There is a note from our discussion in Oslo to improve our
> >>> documentation to describe the API use of to_dict() better and state we
> >>> will not remove to_dict() keys within a release, but that may happen
> >>> between releases.
> >>>
> >>> There is a subsequent problem with how Nova performs a warning test
> >>> [8]. Additional reviews are looking at addressing this sub-class usage
> >>> of from_dict() and to_dict().
> >>>
> >>> Regards
> >>>
> >>> Ronald
> >>>
> >>>
> >>> [5] https://review.openstack.org/#/c/343694/,
> >>> [6] https://review.openstack.org/#/c/342367/
> >>> [7] https://review.openstack.org/#/c/342869/
> >>> [8]
> >>> http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/unit/test_context.py#n144
> >>>
> >>> On Mon, Jul 18, 2016 at 10:50 AM, Matt Riedemann
> >>> <mriedem at linux.vnet.ibm.com>  wrote:
> >>>
> >>>> On 7/18/2016 9:42 AM, Matt Riedemann wrote:
> >>>>
> >>>>> On 7/18/2016 9:09 AM, Markus Zoeller wrote:
> >>>>>
> >>>>>> Since yesterday, Nova uses "oslo.context" 2.6.0 [1] but the needed
> >>>>>> change [2] is not yet in place, which broke
> >>>>>> "gate-nova-python27-db"[3].
> >>>>>> Logstash counts 70 hits/h [4]. Most folks will be at the midcycle in
> >>>>>> Portland and won't be available for the next 2h or so.
> >>>>>> If you can have a look at it and merge it, that would be great.
> >>>>>>
> >>>>>> References:
> >>>>>> [1]
> >>>>>>
> >>>>>>
> >>>>>> https://github.com/openstack/requirements/commit/238389c4ee1bd3cc9be4931dd2639aea2dae70f1
> >>>>>>
> >>>>>> [2] https://review.openstack.org/#/c/342604/1
> >>>>>> [3] https://bugs.launchpad.net/nova/+bug/1603979
> >>>>>> [4] logstash: http://goo.gl/79yFb9
> >>>>>>
> >>>>>> This is an API change for oslo.context, why wasn't it released as
> >>>>> 3.0.0?
> >>>>>
> >>>>> Seems we should revert the upper-constraints bump and blacklist 2.6.0,
> >>>>> then get that released as 3.0.0.
> >>>>>
> >>>>> Here is the blacklist:
> >>>>
> >>>> https://review.openstack.org/#/c/343683/
> >>>>
> >>>>
> >>>> --
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Matt Riedemann
> >>>>
> >>>>
> >>>>
> >>>> __________________________________________________________________________
> >>>> 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
> >>>>
> >>>
> >>> __________________________________________________________________________
> >>> 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
> >>>
> >>>
> >>
> >> __________________________________________________________________________
> >> 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
> >>
> >
> > __________________________________________________________________________
> > 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
> >