| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 16:02:52 | dansmith | the update at top thing should really happen pretty late, after the cell sync, so I didn't think that was a problem | |
| 16:03:06 | mriedem | update at top happens for any state change doesn't it? | |
| 16:03:23 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L573 | |
| 16:03:41 | mriedem | so ^ should trigger an update at the top | |
| 16:03:44 | mriedem | which will delete the build request | |
| 16:04:01 | mriedem | and then later we'll get build request not found which will make conductor delete the instance mapping https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L604 | |
| 16:04:07 | mriedem | thinking the instance was deleted by the user during build | |
| 16:04:42 | mriedem | yup, and https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L605 means we don't cast to the compute to build the instance | |
| 16:04:43 | dansmith | yeah, but I expected it to happen late enough | |
| 16:05:52 | mriedem | i think i get a cookie now | |
| 16:06:00 | mriedem | frosted sugar with sprinkles please | |
| 16:06:01 | dansmith | weyl.. I'm not sure what to do, other than to weaken conductor's interpretation of the BR being missing if cellsv1 | |
| 16:06:21 | mriedem | can we conditionally delete the build request in cells/messaging based on the instance state? | |
| 16:07:00 | dansmith | well, which state? | |
| 16:07:06 | ildikov | mriedem: I'm at a place selling cupcakes one of which is called 'Uniporn & Rainho', sucks I can't send over one through IRC :/ | |
| 16:07:21 | mriedem | dansmith: is ACTIVE too late? | |
| 16:07:47 | dansmith | I would think, | |
| 16:07:53 | dansmith | since we might not even make it to ACTIVE | |
| 16:08:10 | mriedem | i'm not even sure why we need this | |
| 16:08:14 | dansmith | what if we do it only if save() succeeded | |
| 16:08:15 | mriedem | given https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L597 | |
| 16:08:50 | dansmith | mriedem: I think it's because that might've happened in a separate api db | |
| 16:08:57 | mriedem | we could alternatively check if we've set the cell mapping on the instance mapping, meaning we've picked a host, https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L509 but that's racy | |
| 16:09:19 | mriedem | because then conductor and cells/messaging are racing to delete the build request | |
| 16:09:30 | mriedem | but cells/messaging will only do it on the next update at top | |
| 16:09:40 | mriedem | which presumably would come after conductor has set the mapping and already deleted the build request | |
| 16:09:57 | dansmith | if they're sharing an api db, but not the cell db, | |
| 16:10:13 | mriedem | hmm, so the thing conductor deletes isn't necessarily the same thing that cells/messaging deletes? | |
| 16:10:24 | dansmith | then an update will delete the BR in the shared api db, which will then make conductor delete from the cell db, which will sync back up as a delete to the top level db | |
| 16:10:44 | dansmith | but if they're separate api dbs, | |
| 16:10:45 | dansmith | then you have to sync the BR delete up or you shadow the actual instance | |
| 16:10:52 | dansmith | since we were in the cell db when we did the BR delete | |
| 16:11:27 | dansmith | mriedem: you wanna borrow my rusty spoon when I'm done with it? | |
| 16:11:35 | mriedem | for your eyeballs? | |
| 16:11:37 | dansmith | yes | |
| 16:11:43 | mriedem | sure | |
| 16:11:46 | mriedem | send'er over | |
| 16:11:48 | dansmith | heh | |
| 16:11:55 | mriedem | pre-gooped please | |
| 16:14:10 | dansmith | mriedem: anyway, I want to fix this, but I can't really justify spending time on it ahead of other stuff on my plate | |
| 16:14:37 | dansmith | the patch is there for people to apply if they need it for that scenario, and maybe I can circle back after FF or something | |
| 16:17:05 | belmoreira | dansmith: +1, for me this patch makes sense in older versions (newton, ocata). Not sure how useful it will be in Pike, Queens. | |
| 16:17:10 | belmoreira | dansmith mriedem thanks. I will keep you posted | |
| 16:19:02 | mriedem | dansmith: edleafe: so after careful contemplation in the last 5 minutes, | |
| 16:19:08 | mriedem | i think we can unpin the Selection object patch | |
| 16:19:21 | dansmith | okay | |
| 16:19:31 | mriedem | my concerns in the top patch in the series about filtering out the hosts that have already been tried can be done in other ways using the filter_properties retry list of hosts | |
| 16:19:39 | mriedem | i.e. knowing what's already been claims | |
| 16:19:41 | mriedem | *claimed | |
| 16:20:22 | bauzas | I need to look at those Selection changes me too | |
| 16:20:55 | dansmith | gdi gerrit | |
| 16:21:38 | mriedem | jaypipes: https://review.openstack.org/#/c/495854/ if you will +W | |
| 16:23:01 | mriedem | or i can just re-approve, it was a rebase plus some minor changes | |
| 16:23:24 | mriedem | i'll just re-approve :) | |
| 16:25:33 | gibi | mriedem: I proposed a fix for bug 1735407 but I feel that it only solves part of the race. | |
| 16:25:34 | openstack | bug 1735407 in OpenStack Compute (nova) "[Nova] Evacuation doesn't respect anti-affinity rules" [Medium,In progress] https://launchpad.net/bugs/1735407 - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 16:25:37 | mriedem | efried: per your earlier question about empty vs 404 ,this was the spec discussion i was thinking of https://review.openstack.org/#/c/497713/9/specs/queens/approved/add-trait-support-in-allocation-candidates.rst@78 | |
| 16:25:37 | cdent | efried: jaypipes, mriedem and edleafe are all correct | |
| 16:25:41 | gibi | mriedem: I would appreciate your view about the possible solution I drafted in the bug report (see my last 3 comments there) | |
| 16:26:03 | efried | cdent mriedem ack | |
| 16:26:13 | cdent | the only time a 404 should happen on a collection resource is if the URL doesn’t exist (in which case it’s not a collection resource) | |
| 16:27:30 | mriedem | gibi: in general i think reducing the window is a positive step forward, despite other known limitations as pointed out | |
| 16:27:35 | mdbooth | jaypipes: Pretty sure that anywhere that uses retry without creating a transaction context would be a bug, no? | |
| 16:28:06 | mriedem | gibi: #3 in comment 11 would be a further improvement in case we still have issues | |
| 16:29:12 | gibi | mriedem: I agree. We have to see if the current fix solves the problem in the environment it was found earlier | |
| 16:29:53 | gibi | mriedem: if yes then we are OK, if no then I can try to implement option #3 | |
| 16:30:10 | sean-k-mooney2 | mriedem: dansmith when ye have a second could ye take alook at this trival update to the .gitreview file for stable/pike in os-vif https://review.openstack.org/#/c/488670/ | |
| 16:31:18 | dansmith | snagged | |
| 16:31:20 | mriedem | gibi: there is an issue in your patch | |
| 16:31:30 | mriedem | gibi: the compute can't up-call to get the request spec | |
| 16:31:35 | mriedem | b/c the reqspec is in the api db | |
| 16:31:40 | mriedem | and the compute should be isolated fromthat | |
| 16:32:02 | gibi | mriedem: that is bad | |
| 16:32:12 | mriedem | :) | |
| 16:32:25 | mriedem | it just means that you would have to plumb the request_spec through to the compute rebuild_instance method | |
| 16:32:28 | mriedem | using rpc api parms | |
| 16:32:44 | gibi | mriedem: would that mean an rpc version bump as well? | |
| 16:32:50 | mriedem | yes | |
| 16:32:56 | mriedem | granted, _validate_instance_group_policy is already doing an up-call to the api db | |
| 16:32:59 | mriedem | by getting the server groups | |
| 16:33:09 | mriedem | and that's why we have CONF.workarounds.disable_group_policy_check_upcall | |
| 16:34:40 | gibi | would it make sense doing the request spec upcall in the _validate_instance_group_policy ? | |
| 16:34:50 | gibi | in _do_validation | |
| 16:34:54 | mriedem | no, | |
| 16:35:00 | mriedem | we need fewer up-calls, not more | |
| 16:35:21 | gibi | true | |
| 16:35:22 | mriedem | https://docs.openstack.org/nova/latest/user/cellsv2-layout.html#caveats-of-a-multi-cell-deployment | |
| 16:35:28 | mriedem | ^ that list needs to shrink | |
| 16:36:20 | gibi | this would be inside the 'The late anti-affinity check' item in that list, but I agree to look at the other option instead | |
| 16:36:45 | gibi | so, if the rpc version is bumped, can I still backport the fix to stable branches? | |
| 16:36:54 | mriedem | no | |
| 16:37:57 | mriedem | gibi: given the super latent nature of this bug, | |
| 16:38:04 | gibi | it seems this bug exists from at least Mitaka | |
| 16:38:16 | mriedem | i don't think we need to add more up-calls within the validate method just to backport | |
| 16:38:44 | mriedem | the bug has existed since evacuate i assume | |
| 16:38:49 | gibi | could be | |
| 16:39:01 | gibi | so we say we only fix it in master and not on any stable | |
| 16:39:23 | mriedem | i think so | |
| 16:39:52 | gibi | OK, I will do the rpc change | |
| 16:40:06 | openstackgerrit | Mike Perez proposed openstack/nova master: Replace support matrix ext with common library https://review.openstack.org/481304 | |