Open Stack

> From: John Garbutt [mailto:john at johngarbutt.com]
> Sent: November 24, 2015 16:09
> 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.

I implemented the suggestion [1], it seems to work with basic tests [2] but 
there are some technicalities in the _populate_schema() you might want to check
and comment.
There will be notification sub-team meeting today [3] where we can discuss the 
idea further.

Cheers,
Gibi

[1] https://review.openstack.org/#/c/247024/6/nova/objects/notification.py,cm 
[2] https://review.openstack.org/#/c/247024/6/nova/tests/unit/objects/test_notification.py,cm
[3] https://wiki.openstack.org/wiki/Meetings/NovaNotification 

> 
> Thanks,
> johnthetubaguy
> 
> __________________________________________________________
> ________________
> 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