On 11/24/2015 10:09 AM, John Garbutt wrote: > On 24 November 2015 at 15:00, Balázs Gibizer > <balazs.gibi...@ericsson.com> wrote: >>> From: Andrew Laski [mailto:and...@lascii.com] >>> Sent: November 24, 2015 15:35 >>> On 11/24/15 at 10:26am, Balázs Gibizer wrote: >>>>> From: Ryan Rossiter [mailto:rlros...@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:and...@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.gibi...@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.
Ok, so that means we'll now have: * REST representation (strongly versioned / documented) * Notification representation (strongly versioned / documented ) * Nova objects representation (strongly versioned / documented only in code) * Nova db objects (versioned by schema, documentated only in code) If the notifications are not going to be raw Nova objects, I think we need to really think about why they aren't the REST objects. Having a whole other additional interface surface seems really really weird. -Sean -- Sean Dague http://dague.net __________________________________________________________________________ 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