17:00:03 <gibi> #startmeeting nova notification
17:00:03 <openstack> Meeting started Tue Apr  4 17:00:03 2017 UTC and is due to finish in 60 minutes.  The chair is gibi. Information about MeetBot at http://wiki.debian.org/MeetBot.
17:00:04 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
17:00:06 <openstack> The meeting name has been set to 'nova_notification'
17:00:19 <mriedem> o/
17:00:24 <gibi> hi
17:00:28 <sneti> o/
17:01:00 <aunnam> o/
17:01:11 <gibi> ohh we are so many today :)
17:01:41 <gibi> Let's start with the searchlight integration patches
17:01:56 <gibi> there is couple of patches here https://review.openstack.org/#/q/status:open+topic:bp/additional-notification-fields-for-searchlight
17:02:01 <gibi> that needs only a second +2
17:02:35 <gibi> and also there is one question in https://review.openstack.org/#/c/448779/ to discuss
17:03:26 <gibi> so the BDM in the payload needs some extra db query
17:03:27 <gibi> https://review.openstack.org/#/c/448779/9/nova/compute/utils.py@217
17:04:00 <gibi> in the current proposal every intance notification would query the BDM table
17:04:25 <gibi> we touched it in the bp discussion
17:04:34 <gibi> and there the poroposal was to add a config option
17:05:00 <gibi> if the user need the BDM in the notification then set the option to True and nova will query the db and fill that part of the payload
17:05:30 <gibi> however if the user doesn't need the BDM just the other parts of the payload then she can turn that off
17:05:48 <gibi> before we jump into this implementation it would be good to see some oppinion about that
17:05:54 <mriedem> i think the config option might have been my idea in the bp originally
17:05:55 <gibi> mriedem: do you have a view on this?
17:06:05 <mriedem> having said that, i don't like config-driven apis :)
17:06:42 <mriedem> then i think we talked about doing this based on knowing if we'd even send notifications
17:07:02 <gibi> yeah that part has been solved
17:07:21 <gibi> if the user don't want to get any notification then no extra DB query will be executed
17:07:29 <mriedem> ok
17:07:44 <gibi> the only remaning question if the user want instance notification but dont want to pay the price of the BDM query
17:07:46 <mriedem> and if we know that a notification is going to be sent, then we need to decide if we are going to look these up?
17:07:52 <mriedem> hmm
17:07:53 <gibi> mriedem: yes
17:08:01 <mriedem> this is just for versioned notifications right? not legacy.
17:08:10 <gibi> this is just for the versioned yes
17:08:30 <mriedem> the spec about integrating searchlight was going to make that config driven,
17:09:07 <mriedem> so it might be ok to make this config driven for now, and eventually when we have the config option to tell nova to use searchlight, we deprecate this older 'bdms_in_notifications' option or whatever it's called and replace it with the searchlight one
17:09:19 <mriedem> because this is really just about caring about having bdms for the rest api for searchlight
17:09:47 <mriedem> notifications are already config driven anyway because of the notify_on_state_change option
17:10:03 <gibi> yes notify_on_state_change is a thing
17:10:32 <gibi> it sounds OK to me to add 'bdms_in_notification' config option for this for now
17:10:37 <mriedem> so i think i'm ok with config with the understanding that we eventually replace that config option with the searchlight one later
17:10:39 <gibi> I guess the default can be False for now
17:10:42 <mriedem> agree
17:10:55 <sneti> so when nova uses searchlight, are we okay with the db load then?
17:11:27 <sneti> when we deprecate the 'bdms_in_notifications' option in future?
17:11:53 <gibi> sneti: I guess we have no other choice. It is still a better to have a single BDM query for every notification than a big bulk query in every cells to list the instances
17:12:35 <sneti> ahh..ok
17:12:37 <gibi> sneti: I think we have to see how fast the searchlight integration will go
17:12:58 <gibi> sneti: I think in Pike we can have a proof of concept but not more
17:13:19 <gibi> sneti: so I guess we deprecate the config option earliest in Queen
17:13:23 <mriedem> yeah i think POC will be good progress in pike
17:13:30 <mriedem> POC + a CI job
17:13:45 <gibi> CI job for sure :)
17:14:07 <sneti> sneti, so config option should be good for now
17:14:32 <gibi> sounds OK to me. I will comment the patch to record this decision
17:15:09 <gibi> anyting else on the BDM or other searchlight related payload extensions?
17:16:00 <sneti> gibi, I will need to discuss with you about adding volume during boot..may be we can do that after meeting
17:16:43 <gibi> sneti: this is quite late here so I would leave after the meeting
17:16:52 <gibi> what if we take that at the end of the meeting
17:16:57 <sneti> gibi, sure
17:17:17 <gibi> Ok so moving forward
17:17:56 <gibi> I haven't seen any code patch up on searchlight side for the versioned notification switch so I will lurk on the weekly searchlight meeting to get more info
17:18:36 <gibi> that was all I know about the searchlight stuff
17:19:21 <gibi> There was some discussion about snapshot notification yestarday
17:19:28 <gibi> and as a result we have a patch now https://review.openstack.org/#/c/453077/
17:19:35 <mriedem> gibi: did you see my questions in there?
17:19:36 <gibi> mriedem: you have issues with the version there
17:19:38 <mriedem> yeah
17:19:39 <gibi> yeah
17:19:43 <mriedem> so we're in a bit of a mess
17:19:46 <gibi> yes
17:19:52 <mriedem> because we re-used InstanceActionPayload for everything
17:20:02 <mriedem> which limits our ability to add action-specific payload entries later
17:20:10 <mriedem> w/o resetting the version
17:20:32 <gibi> I can introduce InstanceActionSnapshotPayload withversion 1.2
17:20:39 <gibi> that would solve half of the problem
17:20:46 <gibi> version would increase
17:20:53 <gibi> but the object name still changes
17:21:51 <gibi> I have no better idea right now
17:21:57 <mriedem> that's a bit better, i don't have a better idea either
17:22:09 <mriedem> it's hard when we don't know how anything is consuming these
17:22:24 <mriedem> the thing that consumers probably really care about is the event_type
17:22:46 <mriedem> and yeah the version
17:22:52 <mriedem> i think we can't go from 1.1 to 1.0
17:22:56 <mriedem> so we have to do 1.1 to 1.2
17:22:59 <mriedem> even if the object name changes
17:23:26 <gibi> we should add a reno for that and hope that no consumer will be broken too much
17:24:13 <mriedem> heh
17:24:19 <mriedem> if only we said the same thing about rest api changes
17:24:24 <gibi> we can even try to drop the object name from the paylod to force consumers depending on the event type
17:24:31 <gibi> BUT we have nested objects
17:24:44 <gibi> and nested objects doesn't have event_type
17:25:12 <mriedem> i think i'm ok as long as the version increases
17:26:06 <gibi> we can say, we will never every create inheritance tree from payload objects ever :)
17:26:24 <gibi> we have the InstanceActionPayload as the only one
17:27:19 <gibi> I will think more about this rules and update the notification dev-ref
17:27:52 <mriedem> invent a time machine and we could just create action-specific payloads from the start
17:27:57 <gibi> and I will update the current patch with version 1.2
17:28:26 <gibi> we need a time machine spec in the nova backlog
17:28:49 <gibi> that would have been a nice joke for 1st of April
17:29:21 <gibi> anyhow let's move forward
17:29:38 <gibi> the short circuit notification patch has been merged. :)
17:30:08 <gibi> in the future we can use if_notifications_enabled decorator if we want to skip a function call
17:30:35 <gibi> I put that in the decorator in the generic places in the original patch
17:30:41 <mriedem> cool
17:31:25 <gibi> then the last thing from my side is that I picked up on an old comment of you about the redundant notification samples
17:31:50 <gibi> we we extend a payload a lot of samples needs to be updated
17:31:53 <gibi> and that is ugly
17:32:34 <mriedem> yeah the flavor one
17:32:40 <mriedem> triggered a bunch of changes
17:32:41 <gibi> so I proposed some patch with an idea that uses json refs (e.g. pointers) to reference common json fragments instead of duplicating those
17:32:47 <gibi> https://review.openstack.org/#/c/452818/
17:33:06 <gibi> actually it is a series of patches
17:33:18 <gibi> and I only pushed the first three to demo the idea
17:34:02 <mriedem> http://docs-draft.openstack.org/19/452819/1/check/gate-nova-docs-ubuntu-xenial/74191e5//doc/build/html/notifications.html#existing-versioned-notifications
17:34:10 <mriedem> "payload":{"$ref": "payloads/InstanceActionPayload.json#"},
17:34:14 <mriedem> looks like it needs work :)
17:34:17 <gibi> yeah that is broken
17:34:26 <gibi> I just noticed that too
17:34:30 <gibi> not hard to fix
17:34:46 <gibi> just needs to replace the refs in the doc generation like in the sample tests
17:35:20 <gibi> if nova community likes the general idea we can do the refactoring step by step
17:35:36 <mriedem> ok
17:35:50 <mriedem> i'd have to play with it,
17:35:57 <mriedem> by changing something that goes into the common sample ref
17:36:03 <mriedem> and making sure that's propagated
17:36:59 <gibi> one pain point can be that I use a python module called jsonrefs but that is not too old. it is like version 0.1 on pypi
17:37:26 <mriedem> yeah you'd have to get that into global-requirements...
17:37:28 <gibi> but i haven't found any alternatives that actually works :)
17:37:59 <gibi> I'm affraid openstack will not like to depend on version 0.1 stuff
17:38:16 <mriedem> it also doesn't list support for py 3.5
17:38:27 <mriedem> and hasn't been updated in nearly 4 years
17:38:30 <mriedem> so it's probably dead
17:38:46 <gibi> but it works! :) ...  I know I know...
17:38:55 <mriedem> yeah there are commits in 2016
17:38:56 <mriedem> https://github.com/gazpachoking/jsonref/commits/master
17:38:59 <mriedem> just hasn't had a release
17:39:20 <gibi> as an alternative I can hand roll a minimal implementation of the json ref replacement in the nova tree
17:39:50 <mriedem> i'd probably do that as last resort
17:40:01 <mriedem> maybe ask the maintainers to do a 1.0 release :)
17:40:28 <gibi> good point asking is easy, I will do that
17:41:22 <gibi> anyhow I will do some advertisment of this idea in the next notification weekly mail to get feedback from other too
17:41:32 <gibi> that was all from my side for today
17:41:36 <mriedem> i don't have anything
17:41:58 <gibi> sneti: then I think we can take the functional test issue about boot with bdm
17:42:40 <sneti> gibi, so you wanted me to change tests to have volume attach during boot
17:43:08 <sneti> gibi, but the problem is you should have atleast one bdm with boot_index 0
17:43:24 <gibi> even if we provide an image to boot from?
17:43:54 <sneti> yes..this is checked here: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1377
17:44:25 <sneti> http://lists.openstack.org/pipermail/openstack-dev/2015-March/059332.html
17:44:58 <gibi> but here it filters boot_index for None https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1372
17:46:18 * gibi have to read that mail thread
17:47:11 <gibi> ohh there is even a bug fix
17:47:39 <gibi> sneti: would that work for you If I read these references and get back to you?
17:48:18 <sneti> gibi, sure..but I'm facing this issue when I try to attach volume during boot with image
17:48:58 <gibi> so if you would add an image to local bdm to the boot and then a second bdm with a volume volume mapping that might simulate what we want
17:49:12 <gibi> but i have to read more
17:49:16 <sneti> and the bug fix is in nova client..
17:49:59 <sneti> gibi, right..we should have 2 bdms then
17:50:17 <gibi> could you try that?
17:50:45 <sneti> problem with that is I will have to change all the sample files again
17:51:02 <gibi> yeah
17:51:16 <sneti> not sure if that is needed...or we can just go with the current one attaching volume
17:51:56 <gibi> this is just testing so let's not overcomplicate things. let's go with the current attach_volume based test as it tests what we need
17:52:06 <gibi> and later we can improve this in a separate patch
17:52:30 <sneti> gibi, ya..lets do that..
17:52:36 <gibi> you are right it would be stupid blocking the searchlight work just because we want nicer tests
17:52:48 <gibi> we can make it nicer later
17:52:54 <gibi> good
17:53:10 <sneti> gibi, ok
17:53:45 <gibi> sneti: anything else to discuss?
17:54:01 <sneti> gibi, thats it for now
17:54:16 <gibi> thank you all for the good dicussion
17:54:20 <gibi> let's close this.
17:54:32 <gibi> #endmeeting