Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-06-14
13:45:14 mriedem gibi: ^
13:45:28 mriedem efried: which member?
13:45:34 efried Hehe. gibi is the reason I'm asking.
13:45:42 efried Trying to get to the bottom of https://review.openstack.org/#/c/455778/3/nova/notifications/base.py@394
13:45:56 openstackgerrit Alex Xu proposed openstack/nova master: [placement] Update placement devref to modern features https://review.openstack.org/469047
13:46:30 dansmith mriedem: we know about that one right?
13:46:46 Kevin_Zheng mriedem: just saying, nova currently support it :)
13:46:49 dansmith mriedem: tripleo was doing that, and we said starting compute before your catalog was setup was not a thing
13:47:13 mriedem Kevin_Zheng: sure, but i don't see that as a need for SL to need the same information
13:47:16 mriedem when it has the vm_state
13:47:19 mriedem which is essentially the same
13:47:36 mriedem dansmith: forgot about the tripleo case
13:47:41 Kevin_Zheng got it
13:48:25 mriedem Kevin_Zheng: again i'm not blocking it, i would just like a better idea of the end use case and flow, and requirements - which seem pretty thin if it's just "nova rest api has this field so SL should too"
13:48:37 mriedem but, we're doing that for other things
13:48:48 Kevin_Zheng yeah, I understand
13:48:52 mriedem https://blueprints.launchpad.net/nova/+spec/additional-notification-fields-for-searchlight i mean
13:49:39 mriedem efried: so setting https://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/notifications/base.py#L478 to None makes legacy notifications blow up?
13:50:16 efried mriedem I think that's what we don't know.
13:50:29 mriedem because for a versioned payload that should be fine https://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/notifications/objects/instance.py#L86
13:50:34 mriedem since it's nullable=True
13:51:11 efried mriedem Right, although the only other place I can see that field being used, it's being defaulted to 0
13:51:24 efried https://github.com/openstack/nova/blob/9dde4fe0eacc01c135401da3313c493caa99a30c/nova/api/openstack/compute/views/servers.py#L153
13:53:42 efried What's a "legacy notification"? Or any notification, for that matter? Is it a way for an API consumer to register a kind of callback to return instance info periodically or when something happens?
13:54:28 efried (jaypipes FYI, trying to get to the bottom of https://review.openstack.org/#/c/455778/3/nova/notifications/base.py@394)
13:55:34 jaypipes efried: a legacy notification is any notification that doesn't use versioning
13:56:10 efried jaypipes Is there some reason a piece of the notification payload can't be None?
13:56:13 jaypipes efried: notification messages are sent out on the notifications topic queue and subscribers to that queue can take whatever action they want depending on the event type and payload
13:56:34 jaypipes efried: I don't believe so, which is the reason for my comment there :)
13:58:17 openstackgerrit Ed Leafe proposed openstack/nova master: Extract custom resource classes from flavors https://review.openstack.org/473627
14:00:15 edleafe Is it the responsibility of an operator to create a flavor's extra_specs with only valid data?
14:00:30 jaypipes edleafe: good question :)
14:00:34 jaypipes edleafe: not sure, frankly.
14:00:36 edleafe IOW, if they enter a string where an integer is required, how should that be handled
14:00:42 jaypipes edleafe: I would say yes?
14:00:51 edleafe jaypipes: that was my instinct
14:00:58 openstackgerrit Merged openstack/python-novaclient master: strip the remote prefixes from the release note branch specifiers https://review.openstack.org/471082
14:00:59 edleafe but cdent disagrees
14:01:06 edleafe or at least wonders about that
14:01:08 jaypipes edleafe: not user-friendly, but then again, it's extra_specs, so wild west.
14:02:00 edleafe jaypipes: so if they specify a custom resource class that doesn't start with CUSTOM_, raise an error and be done with it?
14:07:04 gibi jaypipes, efried, mriedem: technically legacy notification payload could contain null values. But in this particular case progress was never null in the legacy notification
14:07:19 gibi jaypipes, efried, mriedem: so makit it null now might break the consumer
14:07:35 efried gibi Is this code path *only* used for legacy notifications?
14:07:56 gibi efried: let me doublecheck
14:09:46 efried So the only reason we need to preserve this particular behavior is for backward compatibility. In which case we should keep it defaulting to ''.
14:10:31 gibi efried: the null_safe_int is only used for legacy. the versioned part directly copy the progress field from the Instance and the versioned notification defines progress as nullble too
14:11:43 gibi so yes, the only reason I wouldn't like to send null in the legacy notification as it was never null before
14:12:06 gibi and I have no way to signal (except a release note) that now progress can be null in the legacy notification
14:13:11 gibi jaypipes: you vote for sending null in https://review.openstack.org/#/c/455778/3/nova/notifications/base.py@394 , don't you think it will cause trouble for the consumer?
14:13:36 gibi jaypipes: or it is enough to add a release note to signal such a change
14:13:47 efried I have another suggestion. Commenting in the change set...
14:14:06 jaypipes gibi: I'm kinda of the opinion "just see if anything breaks" :)
14:14:36 gibi jaypipes: so you mean if consumer is broken then they will file a bug and we will revert the change? :)
14:15:00 gibi efried: I appreciate your review!
14:15:30 jaypipes gibi: possibly :) or rather a better scenario would be to have a functional test that attempts to load the legacy notification when null_safe_int() is removed and see if something blows up
14:16:22 openstackgerrit Merged openstack/python-novaclient master: Error out if nic auto or none are not alone https://review.openstack.org/470000
14:16:42 gibi jaypipes: that something that loads the notification in the functional test is not exists today so if I create it now then I definitely make it so that it won't blow up because of progress can be null
14:16:56 gibi jaypipes: however I don't know if that is a meaningfull test :)
14:17:38 jaypipes gibi: I'm not sure either :)
14:18:38 mriedem if the end consumer expects it to not be null, then us testing whatever we change it to internally doesn't help much
14:18:56 gibi mriedem: exactly
14:19:32 gibi mriedem: would you be OK with making progress null and signal that with a reno?
14:20:04 efried gibi jaypipes added a comment.
14:20:10 mriedem what is this holding up?
14:20:12 mriedem why do we care?
14:21:29 gibi it is come up as part of the bugfix https://review.openstack.org/#/c/421981/ but it doesn't hold it up any more
14:22:31 gibi mriedem: so we can ignore the problem if we want
14:24:08 mriedem gibi: jaypipes: efried: commented with my 2 cents,
14:24:14 mriedem ultimately i say abandon
14:24:20 mriedem this isn't worth the time spent discussing it
14:25:13 mriedem especially not when we could be bikeshedding over flavor extra spec validation!
14:25:41 openstackgerrit Matt Riedemann proposed openstack/nova master: Cleanup the plethora of libvirt live migration options https://review.openstack.org/471834
14:25:50 gibi mriedem: I got your point. This is a side track really
14:26:02 gibi mriedem: so I'm not against droping it
14:27:54 sdatko hey mriedem
14:28:20 openstackgerrit Maciej Kucia proposed openstack/nova-specs master: WIP Extended PCI alias https://review.openstack.org/474218
14:28:37 sdatko do you remember, by change, i had issues with testing db_api in current Nova/Master?
14:28:52 mriedem sdatko: i don't
14:29:32 sdatko you recommened me to use nova.objects instead of db.flavor_create /etc.
14:30:11 mriedem efried: more on notifications if you're interested https://docs.openstack.org/developer/nova/notifications.html
14:30:36 mriedem sdatko: ok, i vaguely remember you messing around with things in the database
14:30:50 mriedem and me trying to discourage that
14:30:55 gibi Kevin_Zheng, mriedem: about state vs status we can a status based on https://github.com/openstack/nova/blob/master/nova/api/openstack/common.py#L113 I'm not against it especially if you have a use case needing it
14:30:55 sdatko yep :)
14:31:20 mriedem gibi: i'm not against it really either, as long as i understand the use case first
14:32:13 gibi mriedem: then I guess Kevin_Zheng can put up a patch with the use case in the commit message :)
14:32:48 sdatko mriedem: so i am trying to get all flavorIDs on specific host
14:33:18 mriedem gibi: you know how you're creating the new instance create payload? didn't we do that once already with some other action payload? i was thinking related to snapshots
14:33:22 mriedem but i might be misremembering
14:33:50 mriedem like,
14:33:57 edleafe alex_xu: around?
14:34:02 mriedem we needed to add the snapshot image id to the payload but didn't want to add it to every action payload
14:34:30 mriedem sdatko: flavors are not tied to a compute host, the instance is
14:34:33 mriedem and the instance has a flavor
14:34:37 mriedem associated with it
14:34:42 mriedem embedded really
14:34:52 mriedem nova.objects.Instance.flavor field
14:35:47 sdatko mriedem: in nova.db.sqlalchemy.api i have function that returns model_query(context, models.Instance.instancE_typE_id).filter(models.Instance.host == host).filter(models.Instance.deleted == 0).distinct(models.Instance.instance_type_id).all()
14:35:53 mriedem gibi: ah this https://review.openstack.org/#/c/453077/

Earlier   Later