Excerpts from Jamie Lennox's message of 2016-07-20 10:28:29 +1000: > On 20 July 2016 at 00:06, Joshua Harlow <harlo...@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 > >>> <mrie...@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-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 > >>> > >>> > >> > >> __________________________________________________________________________ > >> 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 > > __________________________________________________________________________ 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
