Open Stack

On 24 November 2015 at 15:00, Balázs Gibizer
<balazs.gibizer at ericsson.com> wrote:
>> From: Andrew Laski [mailto:andrew at lascii.com]
>> Sent: November 24, 2015 15:35
>> On 11/24/15 at 10:26am, Balázs Gibizer wrote:
>> >> From: Ryan Rossiter [mailto:rlrossit at linux.vnet.ibm.com]
>> >> Sent: November 23, 2015 22:33
>> >> On 11/23/2015 2:23 PM, Andrew Laski wrote:
>> >> > On 11/23/15 at 04:43pm, Balázs Gibizer wrote:
>> >> >>> From: Andrew Laski [mailto:andrew at lascii.com]
>> >> >>> Sent: November 23, 2015 17:03
>> >> >>>
>> >> >>> On 11/23/15 at 08:54am, Ryan Rossiter wrote:
>> >> >>> >
>> >> >>> >
>> >> >>> >On 11/23/2015 5:33 AM, John Garbutt wrote:
>> >> >>> >>On 20 November 2015 at 09:37, Balázs Gibizer
>> >> >>> >><balazs.gibizer at ericsson.com> wrote:
>> >> >>> >>><snip>
>> >> >>> >>><snip>
>> >> >> >>
>> >> >>> >>There is a bit I am conflicted/worried about, and thats when we
>> >> >>> >>start including verbatim, DB objects into the notifications. At
>> >> >>> >>least you can now quickly detect if that blob is something
>> >> >>> >>compatible with your current parsing code. My preference is
>> >> >>> >>really to keep the Notifications as a totally separate object
>> >> >>> >>tree, but I am sure there are many cases where that ends up
>> >> >>> >>being seemingly stupid duplicate work. I am not expressing this
>> >> >>> >>well in text form :(
>> >> >>> >Are you saying we don't want to be willy-nilly tossing DB
>> >> >>> >objects across the wire? Yeah that was part of the rug-pulling
>> >> >>> >of just having the payload contain an object. We're
>> >> >>> >automatically tossing everything with the object then, whether
>> >> >>> >or not some of that was supposed to be a secret. We could add
>> >> >>> >some sort of property to the field like
>> >> >>> >dont_put_me_on_the_wire=True (or I guess a
>> >> >>> >notification_ready() function that helps an object sanitize
>> >> >>> >itself?) that the notifications will look at to know if it puts
>> >> >>> >that on the wire-serialized dict, but that's adding a lot more
>> >> >>> >complexity and work to a pile that's already growing rapidly.
>> >> >>>
>> >> >>> I don't want to be tossing db objects across the wire.  But I
>> >> >>> also am not convinced that we should be tossing the current
>> >> >>> objects over the wire either.
>> >> >>> You make the point that there may be things in the object that
>> >> >>> shouldn't be exposed, and I think object version bumps is another
>> >> >>> thing to watch out for.
>> >> >>> So far the only object that has been bumped is Instance but in
>> >> >>> doing so no notifications needed to change.  I think if we just
>> >> >>> put objects into notifications we're coupling the notification
>> >> >>> versions to db or RPC changes unnecessarily.  Some times they'll
>> >> >>> move together but other times, like moving flavor into
>> >> >>> instance_extra, there's no reason to bump notifications.
>> >> >>
>> >> >>
>> >> >> Sanitizing existing versioned objects before putting them to the
>> >> >> wire is not hard to do.
>> >> >> You can see an example of doing it in
>> >> >> https://review.openstack.org/#/c/245678/8/nova/objects/service.py,
>> >> >> cm
>> >> >> L382.
>> >> >> We don't need extra effort to take care of minor version bumps
>> >> >> because that does not break a well written consumer. We do have to
>> >> >> take care of the major version bumps but that is a rare event and
>> >> >> therefore can be handled one by one in a way John suggested, by
>> >> >> keep sending the previous major version for a while too.
>> >> >
>> >> > That review is doing much of what I was suggesting.  There is a
>> >> > separate notification and payload object.  The issue I have is that
>> >> > within the ServiceStatusPayload the raw Service object and version
>> >> > is being dumped, with the filter you point out.  But I don't think
>> >> > that consumers really care about tracking Service object versions
>> >> > and dealing with compatibility there, it would be easier for them
>> >> > to track the ServiceStatusPayload version which can remain
>> >> > relatively stable even if Service is changing to adapt to db/RPC changes.
>> >> Not only do they not really care about tracking the Service object
>> >> versions, they probably also don't care about what's in that filter list.
>> >>
>> >> But I think you're getting on the right track as to where this needs
>> >> to go. We can integrate the filtering into the versioning of the payload.
>> >> But instead of a blacklist, we turn the filter into a white list. If
>> >> the underlying object adds a new field that we don't want/care if
>> >> people know about, the payload version doesn't have to change. But if
>> >> we add something (or if we're changing the existing fields) that we
>> >> want to expose, we then assert that we need to update the version of
>> >> the payload, so the consumer can look at the payload and say "oh, in
>> >> 1.x, now I get _______" and can add the appropriate checks/compat.
>> >> Granted with this you can get into rebase nightmares ([1] still
>> >> haunts me in my sleep), but I don't see us frantically changing the
>> >> exposed fields all too often. This way gives us some form of
>> >> pseudo-pinning of the subobject. Heck, in this method, we could even
>> >> pass the whitelist on the wire right? That way we tell the consumer
>> explicitly what's available to them (kinda like a fake schema).
>> >
>> >I think see your point, and it seems like a good way forward. Let's
>> >turn the black list to a white list. Now I'm thinking about creating a
>> >new Field type something like WhiteListedObjectField which get a type
>> >name (as the ObjectField) but also get a white_list that describes which
>> fields needs to be used from the original type.
>> >Then this new field serializes only the white listed fields from the
>> >original type and only forces a version bump on the parent object if
>> >one of the white_listed field changed or a new field added to the
>> white_list.
>> >What it does not solve out of the box is the transitive dependency. If
>> >today we Have an o.vo object having a filed to another o.vo object and
>> >we want to put the first object into a notification payload but want to
>> >white_list fields from the second o.vo then our white list needs to be
>> >able to handle not just first level fields but subfields too. I guess
>> >this is doable but I'm wondering if we can avoid inventing a syntax
>> expressing something like 'field.subfield.subsubfield'
>> >in the white list.
>>
>> Rather than a whitelist/blacklist why not just define the schema of the
>> notification within the notification object and then have the object code
>> handle pulling the appropriate fields, converting formats if necessary, from
>> contained objects.  Something like:
>>
>> class ServicePayloadObject(NovaObject):
>>      SCHEMA = {'host': ('service', 'host'),
>>                'binary': ('service', 'binary'),
>>                'compute_node_foo': ('compute_node', 'foo'),
>>               }
>>
>>      fields = {
>>          'service': fields.ObjectField('Service'),
>>          'compute_node': fields.ObjectField('ComputeNode'),
>>      }
>>
>>      def populate_schema(self):
>>          self.compute_node = self.service.compute_node
>>          notification = {}
>>          for key, (obj, field) in schema.iteritems():
>>              notification[key] = getattr(getattr(self, obj), field)
>>
>> Then object changes have no effect on the notifications unless there's a
>> major version bump in which case a SCHEMA_VNEXT could be defined if
>> necessary.
>
> Nice idea I will try it. Thanks! It is seems to avoid the sub object field white lists
> problem as the needed notification field can always be pulled directly from an object field.

+1
This is my preference, specific notification objects that are
independently versioned.
It feels like time saving to re-use existing objects, but it breaks
the interface really.

Thanks,
johnthetubaguy