| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-06 | |||
| 15:50:06 | dansmith | which is, like, not a good sign, | |
| 15:50:11 | belmoreira | yeah, cell0 I defined | |
| 15:50:13 | dansmith | but I'm not sure why, especially since you said it works | |
| 15:50:36 | dansmith | like, 100% fail on anything that hits nova | |
| 15:50:40 | dansmith | which seems hard to believe | |
| 15:55:22 | belmoreira | dansmith: I'm running this patch already. Haven't detected any issue yet. However, the functionality that we expose is very limited. Maybe that's why. | |
| 15:55:24 | huanxie | Hi jaypipes, I have updated the reno to make it precise and please help review it again https://review.openstack.org/#/c/451641/ Thanks a lot :) | |
| 15:55:45 | mriedem | belmoreira: can't even build an instance it looks like | |
| 15:55:47 | jaypipes | huanxie: already +2d. | |
| 15:55:48 | dansmith | belmoreira: I dunno, this seems to fail everything all the time, but I can't really see any errors in the logs | |
| 15:55:49 | mriedem | times out waiting to go to ACTIVE | |
| 15:56:17 | huanxie | Many thanks jaypipes :) | |
| 15:56:21 | belmoreira | dansmith: but, as you said, not a good sign :) Will keep eyes open on this | |
| 15:56:58 | dansmith | well, anyway, I don't want to spend too much time on it, but unless we figure that out, we can't merge it | |
| 15:57:09 | belmoreira | dansmith: definitely create/delete instances is working for us with this patch | |
| 15:57:44 | dansmith | although that's ocata I guess | |
| 15:57:45 | belmoreira | but I'm running newton | |
| 15:57:49 | dansmith | oh, newton | |
| 15:58:07 | dansmith | well, still, I'd expect to see an error somewhere if this was actually blowing something up | |
| 15:58:19 | belmoreira | in pike I will not need this patch | |
| 15:59:12 | openstackgerrit | Merged openstack/nova master: Add a new check to volume attach https://review.openstack.org/525622 | |
| 15:59:43 | mriedem | dansmith: looking at the instance create flow, if cellsv1 the api will create the instance before casting to build_instances in conductor, | |
| 15:59:49 | mriedem | build_instances in conductor creates the instance mapping | |
| 16:00:13 | mriedem | well, api creates the instance mapping | |
| 16:00:38 | mriedem | conductor gets the host mapping for the chosen host and pulls the cell mapping from that host mapping to set on the instance mapping | |
| 16:00:41 | mriedem | then deletes the build request | |
| 16:00:53 | mriedem | ah, | |
| 16:01:03 | mriedem | and if the build request is already destroyed when conductor tries to do it, | |
| 16:01:07 | mriedem | conductor deletes the instance mapping | |
| 16:01:11 | mriedem | which the api relies on i think | |
| 16:01:40 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L599-L605 | |
| 16:01:43 | dansmith | oh you think we're deleting the build request early enough that it just nukes the instance when it tries because it assumes the user did it? | |
| 16:02:34 | mriedem | maybe | |
| 16:02:46 | mriedem | and when getting the instance from the api, we'll pull it from the top cell https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2221 | |
| 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 | |