Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-06-14
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 sdatko yep :)
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: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/
14:36:13 gibi mriedem: there are actions with specific payload like https://github.com/openstack/nova/blob/master/nova/notifications/objects/instance.py#L121
14:36:40 mriedem gibi: this is what i was thinking of https://review.openstack.org/#/c/453077/2/doc/notification_samples/instance-snapshot-end.json@71
14:36:45 sdatko and it seems to work; however, i am trying to write unit test for that, but i receive "oslo_db.exception.DBNonExistentTable: (sqlite3.OperationalError) no such table: instances"
14:36:48 mriedem we added a more specific payload,
14:36:53 mriedem and i was checking to see what we did with the version
14:37:03 mriedem so that we are being consistent with InstanceCreatePayload
14:37:18 gibi mriedem: ahh I see. You are askin about the version number
14:37:23 mriedem yes
14:37:29 mriedem i want to be consistent when we rename payloads
14:37:36 mriedem consistent in what we do about the version i mean
14:37:40 mriedem does it stay the same or increment
14:37:51 openstackgerrit Maciej Kucia proposed openstack/nova-specs master: WIP Extended PCI alias https://review.openstack.org/474218
14:38:03 mriedem gibi: looks like it's being consistent and not incrementing the version https://review.openstack.org/#/c/453077/9/doc/notification_samples/instance-snapshot-end.json
14:38:37 gibi mriedem: in case of snapshot_id we did increase as not just renamed the class but added the new field
14:38:42 Kevin_Zheng mriedem gibi I will ask for detailed usecase and see if it is persuasive enough
14:38:48 gibi mriedem: but in the create case we had a separate patch to rename
14:39:01 gibi mriedem: so there we kept the version
14:39:02 mriedem gibi: then isn't this wrong? https://review.openstack.org/#/c/453077/9/doc/notification_samples/instance-snapshot-start.json@72
14:39:06 mriedem shouldn't that be 1.3?
14:39:06 openstackgerrit Maciej Kucia proposed openstack/nova-specs master: WIP Extended PCI alias https://review.openstack.org/474218
14:39:28 gibi mriedem: yes, that is wrong
14:39:39 mriedem cool :)
14:39:41 mriedem -1!
14:39:54 gibi mriedem: thanks for founding it :)
14:39:54 mriedem Kevin_Zheng: thanks
14:41:09 mriedem sdatko: is this for something you're planning on contributing to nova in the community? or just working on separately downstream? because if it's the latter, i don't really have time to help with forking nova.
14:41:39 gibi mriedem: I will be more explicit in the https://review.openstack.org/#/c/453667/ as well about keeping the same version in case of pure rename and bumping it if also content is changed in the same step

Earlier   Later