| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 21:38:37 | dansmith | gibi might have a better handle on how much this does or does not matter | |
| 21:39:27 | melwitt | I was thinking my main concern would be whether the start/end notifications match, and also that they match the request that was issued from the user | |
| 21:39:45 | melwitt | for the first, I saw the context manager makes sure the start/end match | |
| 21:39:47 | dansmith | the context manager makes sure they match I think | |
| 21:40:45 | melwitt | for the last, I guess that's where the delete_type comes in. I was just trying to think, how someone might use this and if they're listening to notifications, they may also be looking at what requests were issued and correlating notifications with those | |
| 21:41:43 | melwitt | to get like a bundle of "what happened" which would be 1) user issued delete_type X 2) nova emitted delete_type X start 3) nova emitted delete_type X end | |
| 21:42:55 | dansmith | I'm not sure what the point of correlating the requests and the notifications really is, but we don't really know what (all) people use these for | |
| 21:42:59 | melwitt | if they're doing log collecting and correlating. anyway, that's the thing I could think of it mattering | |
| 21:43:25 | dansmith | mriedem: I guess my point is, this is demonstrably better than what we have now, I think, especially considering the quota thing behind it | |
| 21:43:46 | dansmith | in some very teensy windows of a less-mainstream feature, you might get a delete type you didn't expect | |
| 21:43:49 | melwitt | at my last company, use used the old notification driver that emits notifications to the logs. and we had those in splunk and correlated things like that, to look at when people opened bugs etc | |
| 21:45:00 | melwitt | I think what I'm saying is what the patch is doing, so it makes sense to me to keep everything saying "soft delete" if the user issued "soft delete" | |
| 21:45:57 | dansmith | well, it's not in the first case he brought up | |
| 21:46:05 | mriedem | the user doesn't technically request "soft" delete | |
| 21:46:11 | mriedem | it's all driven by config | |
| 21:46:17 | mriedem | it's totally not interoperable | |
| 21:46:55 | melwitt | erg, right. notifications probably have the request-id too right? so that's what anyone would use to correlate anyway | |
| 21:47:02 | dansmith | yeah | |
| 21:47:40 | melwitt | okay, that addresses what I was thinking | |
| 21:48:42 | mriedem | so i think i might still pull this down, add some comments, and add a test for the soft-delete case where there isn't a host but it's in a cell (any cell) and the build request was already gone | |
| 21:49:07 | mriedem | because i'm pretty sure i'm right here https://review.openstack.org/#/c/437222/7/nova/tests/unit/utils.py | |
| 21:49:17 | mriedem | then i'll rebase my stuff on top of it | |
| 21:49:23 | mriedem | and then it's pants off dance off | |
| 21:53:05 | dansmith | so, wait, | |
| 21:53:23 | dansmith | we notify delete.started..delete.ended in the api, or soft_delete.started..ended, | |
| 21:53:34 | dansmith | and then again in compute in both cases? | |
| 21:53:46 | dansmith | so if you normal delete, you get two delete.started notifications/ | |
| 21:53:59 | dansmith | and if soft delete then you get the soft one first and the regular one later? | |
| 21:59:02 | melwitt | the notifys should be limited to the local delete cases, right? | |
| 21:59:08 | melwitt | the new ones, I mean | |
| 21:59:43 | mriedem | melwitt: i think so https://review.openstack.org/#/c/437222/7/nova/compute/api.py@1866 | |
| 21:59:53 | melwitt | so they wouldn't be going on to compute to get a second notify | |
| 22:00:01 | mriedem | right | |
| 22:00:09 | mriedem | and where gibi has made this change, it's a local delete | |
| 22:00:45 | dansmith | not if we fail the last race check, but yeah | |
| 22:01:52 | dansmith | so do we ever emit a soft-delete notification normally? | |
| 22:02:00 | dansmith | meaning before all of this changed.. | |
| 22:02:12 | dansmith | only in the local delete, which is totally not a soft delete, is that the only time we emit soft-deleted.start? | |
| 22:03:43 | mriedem | it looks like it... | |
| 22:04:34 | dansmith | how stupid is that? | |
| 22:04:44 | dansmith | the only time we emit soft-deleted is when .. we didn't | |
| 22:04:48 | mriedem | down in _delete_instance in the compute manager, even if it was a soft_delete, we issue instance.delete.start | |
| 22:06:01 | melwitt | I wonder how soft-deleted notify ever came about then. because like you said, there isn't really a soft-delete from the user perspective | |
| 22:06:02 | mriedem | dansmith: that's a good point | |
| 22:06:21 | dansmith | melwitt: right, I can't think of any reason it makes sense to emit soft-deleted | |
| 22:06:39 | dansmith | except maybe in the case where we're deferring it, which we _don't_ notify about | |
| 22:08:15 | mriedem | we'll also issue soft_delete.start/end in the case that the instance is shelved_offloaded, because that's another local delete case | |
| 22:08:26 | dansmith | yeah | |
| 22:08:28 | mriedem | so yeah, seems totally pointless to even distinguish | |
| 22:08:34 | mriedem | since you can't restore it anyway | |
| 22:08:35 | dansmith | we totally cover all the cases where we're not actually soft deleting | |
| 22:08:36 | mriedem | it's mfing gone | |
| 22:09:26 | mriedem | so going to back to https://review.openstack.org/#/c/437222/7/nova/compute/api.py@1792 i don't think we should even pass delete_type there, it's not a soft_delete | |
| 22:09:40 | mriedem | i know we're talking in circles, but you could argue that we should never send soft_delete.start/end | |
| 22:09:48 | dansmith | I think I would be in favor of just never emit soft_delete | |
| 22:09:52 | mriedem | yeah | |
| 22:10:07 | mriedem | for the existing parts that do this, i'd leave that for a separate change | |
| 22:10:10 | dansmith | so for this patch, I say remove that one case where we do, | |
| 22:10:14 | mriedem | yeah | |
| 22:10:15 | dansmith | and then yeah clean up the others | |
| 22:10:18 | mriedem | cool | |
| 22:10:20 | mriedem | hey, high five | |
| 22:10:33 | dansmith | fuck you you -1d my +2, no high five for you | |
| 22:11:00 | dansmith | hmm, that sounded darker and more non-sensical ranty than I meant it to :P | |
| 22:12:17 | mriedem | i stand by my -1 | |
| 22:22:46 | mriedem | crap | |
| 22:22:58 | mriedem | all 3 speaking sessions at the summit were accepted | |
| 22:23:16 | mriedem | armax: from boot to woot | |
| 22:30:12 | armax | mriedem: I just saw the emaik | |
| 22:30:15 | armax | ||
| 22:30:29 | armax | we should probably sit down and talk about it at some point | |
| 22:34:18 | mriedem | yeah | |
| 22:37:55 | tonyb | sean-k-mooney: It's all good. As you can see I'm trying to work with you but for the most part I'm learning on my feet. It'd be cool if we had a clear direction on what level of support we have for Windows platforms. Are they for development? or just runtime? I'm treatign win32 vs cygwin much like Ubuntu vs Fedora. Anyway we don't overlap much but we can keep going back and forth on the review | |
| 22:40:28 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix missing instance.delete notification https://review.openstack.org/437222 | |
| 22:57:14 | mriedem | great, now the functional test isn't passing after i rebased on top of the notification fix | |
| 23:12:20 | openstackgerrit | Samantha Blanco proposed openstack/nova master: [WIP] Make flavor-rxtx policy more granular https://review.openstack.org/444036 | |
| 23:31:02 | mriedem | dansmith: ha | |
| 23:31:04 | mriedem | so, | |
| 23:31:19 | mriedem | the notification patch doesn't have a test that hits notify_about_instance_delete | |
| 23:31:25 | mriedem | but mine on top of it do | |
| 23:31:34 | mriedem | UnboundLocalError: local variable 'system_metadata' referenced before assignment | |
| 23:32:10 | mriedem | fixing | |
| 23:35:25 | efried | mriedem That whole initializing-a-variable-within-a-try-and-then-using-it-in-the-except-or-finally thing... | |
| 23:35:40 | mriedem | efried: i even thought about it when i was looking at it before | |
| 23:35:56 | mriedem | efried: er i mean, it was a test to see if you would have caught and you didn't! | |
| 23:36:35 | efried | mriedem Rat farts! I _did_ review that change set! | |
| 23:37:01 | mriedem | rat farts indeed | |
| 23:46:05 | openstackgerrit | Dan Smith proposed openstack/nova master: Get instance availability_zone without hitting the api db https://review.openstack.org/439754 | |
| 23:46:05 | openstackgerrit | Dan Smith proposed openstack/nova master: Teach HostAPI about cells https://review.openstack.org/442162 | |
| 23:46:06 | openstackgerrit | Dan Smith proposed openstack/nova master: Make os-availability-zones know about cells https://review.openstack.org/442163 | |
| 23:46:06 | openstackgerrit | Dan Smith proposed openstack/nova master: Change MQ targeting to honor only what is in the context https://review.openstack.org/438023 | |
| 23:46:07 | openstackgerrit | Dan Smith proposed openstack/nova master: Make scheduler target cells to get compute node instance info https://review.openstack.org/439891 | |
| 23:46:07 | openstackgerrit | Dan Smith proposed openstack/nova master: Sort CellMappingList.get_all() for safety https://review.openstack.org/443174 | |
| 23:46:08 | openstackgerrit | Dan Smith proposed openstack/nova master: Add get_by_instance_uuids() to InstanceMappingList https://review.openstack.org/443292 | |
| 23:46:08 | openstackgerrit | Dan Smith proposed openstack/nova master: Make server_groups determine deleted-ness from InstanceMappingList https://review.openstack.org/443293 | |
| 23:46:09 | openstackgerrit | Dan Smith proposed openstack/nova master: Remove Mitaka-era service version check https://review.openstack.org/442861 | |
| 23:46:12 | openstackgerrit | Dan Smith proposed openstack/nova master: Add cell field to Destination object https://review.openstack.org/438024 | |
| 23:46:13 | openstackgerrit | Dan Smith proposed openstack/nova master: Make conductor ask scheduler to limit migrates to same cell https://review.openstack.org/438025 | |
| 23:46:13 | openstackgerrit | Dan Smith proposed openstack/nova master: Add workaround to disable group policy check upcall https://review.openstack.org/442736 | |
| 23:46:14 | openstackgerrit | Dan Smith proposed openstack/nova master: Add identity helper property to CellMapping https://review.openstack.org/444038 | |