| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-15 | |||
| 17:10:37 | jmlowe | I've been having a problem with nova, I'm pretty sure bugs have been reported years ago (can't find them atm) but have languished, when an instance goes into error state the underlying disk is deleted, this is exactly the wrong thing to do | |
| 17:11:02 | cfriesen | dansmith: that's the easy part. what about within the python code? | |
| 17:11:23 | dansmith | cfriesen: well, knowing what binary has the excess is a start | |
| 17:11:37 | dansmith | we'd need extra instrumentation to know which code I think | |
| 17:12:22 | mriedem | jmlowe: never heard of that | |
| 17:12:22 | jmlowe | for instance, Wednesday during the summit a user resumed a suspended instance that didn't have enough memory so qemu wouldn't start, this is eminently fixable unless nova comes along and deletes the underlying block device for the instance | |
| 17:13:36 | jmlowe | If I dig back in my logs I even have errors from nova repeatedly deleting the same rbd device after the first wrongful deletion is succeeds | |
| 17:14:55 | jmlowe | no probably not just a gross conceptual error in the state diagram but also a bad implementation | |
| 17:15:43 | jmlowe | also kind of hard to replicate reliably on a 300 compute node production system | |
| 17:15:46 | mriedem | jmlowe: what you might be thinking of i,s | |
| 17:15:51 | mriedem | resume in the libvirt driver calls this https://github.com/openstack/nova/blob/8d9eb670c75a8209901bac3b4dd94925b6b1f000/nova/virt/libvirt/driver.py#L2562 | |
| 17:16:12 | mriedem | if that fails in some unexpected way, we go here https://github.com/openstack/nova/blob/8d9eb670c75a8209901bac3b4dd94925b6b1f000/nova/virt/libvirt/driver.py#L5120 | |
| 17:16:22 | jmlowe | will also trigger on error state due to migration error | |
| 17:17:14 | mriedem | destroy_disks_on_failure defaults to False here | |
| 17:17:18 | mriedem | and isn't set to True for the resume case | |
| 17:17:40 | mriedem | so when calling cleanup, destroy_disks would be False https://github.com/openstack/nova/blob/8d9eb670c75a8209901bac3b4dd94925b6b1f000/nova/virt/libvirt/driver.py#L5042 | |
| 17:18:44 | mriedem | jmlowe: i'm confused, are you doing a migration or a resume? | |
| 17:19:08 | jmlowe | happens for both, seemingly any time an instance goes into error | |
| 17:19:10 | mriedem | because if is_shared_block_storage=True, then https://github.com/openstack/nova/blob/8d9eb670c75a8209901bac3b4dd94925b6b1f000/nova/virt/libvirt/driver.py#L1014 would delete instance files on disk | |
| 17:19:30 | jmlowe | in my case ceph backed | |
| 17:19:35 | mriedem | the instance going to error state is not a related issue for deleting the disks in the virt driver | |
| 17:20:11 | mriedem | jmlowe: which release of nova are you on? | |
| 17:20:17 | jmlowe | newton | |
| 17:20:27 | jmlowe | don't think it was doing this in mitaka | |
| 17:20:39 | mriedem | i don't really see anything in newton when resume would fail that would destroy the disks | |
| 17:21:24 | mriedem | now, i think that _cleanup_failed_start method was new in ocata and backported to newton... | |
| 17:21:47 | mriedem | yeah https://review.openstack.org/#/q/Iab5afdf1b5b8d107ea0e5895c24d50712e7dc7b1 | |
| 17:23:13 | mriedem | hmm, destroy disks wasn't passed through at that point | |
| 17:23:15 | mriedem | checking something | |
| 17:24:09 | mriedem | https://review.openstack.org/#/q/I38c969690fedb71c5b5ec4418c1b0dd53df733ec | |
| 17:24:35 | mriedem | jmlowe: do you have this fix? https://review.openstack.org/#/c/428671/ | |
| 17:24:41 | mriedem | went into newton 14.0.4 | |
| 17:25:00 | mriedem | fixed the thing introduced in 14.0.3 | |
| 17:27:27 | jmlowe_ | specifically I should have nova 14.0.3 | |
| 17:27:56 | mriedem | jmlowe_: you mean that's what you're running? | |
| 17:28:10 | mriedem | if so, you need https://review.openstack.org/#/c/428671/ | |
| 17:28:40 | jmlowe_ | yeah, was just trying to determine if that commit was in 14.0.3 | |
| 17:29:23 | jmlowe_ | and it's not | |
| 17:31:22 | jmlowe_ | *grumble* now off to go harass the rdo packagers | |
| 17:31:41 | jmlowe_ | thanks for the pointer, never would have found that myself | |
| 17:32:08 | jmlowe_ | on the other hand I was planning on going to ocata in the next week or so | |
| 18:00:49 | dansmith | edleafe: looks like maybe this https://review.openstack.org/#/c/464086 should go underneath bauzas' patch to do the claim? | |
| 18:01:36 | edleafe | dansmith: sure, it should go before the claim code | |
| 18:07:29 | dansmith | edleafe: so given the conductor direction, are you going to work on the obviously half-finished claim-in-scheduler patch at the top of that series? | |
| 18:07:47 | dansmith | edleafe: there is a big missing piece of handling the case where we fail to claim the first host It hink | |
| 18:08:20 | edleafe | Well, I was kind of told *not* to work on that one :) | |
| 18:08:26 | dansmith | oh | |
| 18:08:29 | edleafe | But yeah, I was planning on completing it | |
| 18:08:36 | edleafe | until this morning | |
| 18:08:54 | dansmith | okay.. no update since may 11 | |
| 18:09:08 | dansmith | I guess we're expecting something presently? | |
| 18:09:23 | edleafe | I'm curious, though - where do you see the claim fail logic missing? | |
| 18:10:29 | dansmith | if we fail doing _allocate_for_instance() here, we need to retry the claim for the same instance, but on the next host right? https://review.openstack.org/#/c/460455/6/nova/conductor/manager.py | |
| 18:10:45 | dansmith | L957 just moves on to the next instance, right? | |
| 18:10:46 | edleafe | dansmith: fwiw, I was on PTO May 12, and came back in town this morning to a big fat -2 | |
| 18:11:21 | dansmith | edleafe: I meant no update on bauzas' conductor claim one | |
| 18:11:29 | dansmith | edleafe: yours have been updated more recently, AFAICT | |
| 18:11:31 | edleafe | dansmith: oh, you mean bauzas' series. I thought you meant mine | |
| 18:11:39 | dansmith | nope | |
| 18:11:43 | edleafe | ah | |
| 18:11:49 | dansmith | so actually, | |
| 18:12:00 | dansmith | if we fail to claim on L957 do we just stop building everything? | |
| 18:12:04 | dansmith | I'm kinda assuming we do | |
| 18:12:04 | edleafe | bauzas did mention that he was ready with a new PS real soon | |
| 18:12:28 | dansmith | actually, maybe we don't explode, but just continue with no allocation? | |
| 18:12:29 | dansmith | eesh | |
| 18:12:48 | edleafe | ...which negates the whole point of eliminating the race, no? | |
| 18:13:23 | dansmith | well, it's just completely wrong | |
| 18:13:34 | dansmith | we can't ask compute to build an instance we didn't actually claim for :P | |
| 18:13:49 | edleafe | My series didn't (yet) account for the computes not all being updated to the latest | |
| 18:14:00 | edleafe | was hoping for guidance on that | |
| 18:14:42 | dansmith | yeah, so what bauzas has there is how to do that | |
| 18:14:47 | dansmith | checking the service version | |
| 18:15:15 | dansmith | I feel like we also probably want to communicate to the compute that it should expect the allocation to have been done already | |
| 18:15:15 | edleafe | well, yeah, but I didn't think it was right for the scheduler to know about such things | |
| 18:15:43 | dansmith | well, I dunno.. scheduler knows such things in a couple places I think, but it's definitely okay for conductor to know that | |
| 18:15:45 | mriedem | dansmith: we agreed a few weeks ago that the compute would look for existing allocations and if none found, assume the control layer didn't do that | |
| 18:15:46 | edleafe | the compute checks already, if I'm not mistaken | |
| 18:15:51 | edleafe | in bauzas' series | |
| 18:16:04 | edleafe | mriedem: bingo | |
| 18:16:10 | dansmith | mriedem: okay, well, we better be damn sure that we fix the bug here :) | |
| 18:16:28 | dansmith | because this code will not claim and just send it to the compute, destined for certain failure | |
| 18:17:07 | dansmith | the exact opposite of what it's supposed to do | |
| 18:17:10 | mriedem | because it's not retrying? | |
| 18:17:20 | mriedem | sorry i'm coming in to this halfway | |
| 18:17:38 | edleafe | dansmith: well, if you can look at the rest of my series and assume that the reqspec changes will be reversed and the list of UUIDs will be an RPC param, your feedback would be appreciated | |
| 18:17:48 | dansmith | mriedem: it totally ignores the result of "did my claim work?" | |
| 18:17:58 | mriedem | yeah | |
| 18:18:02 | mriedem | -1 it | |
| 18:18:04 | dansmith | edleafe: well, your set still does the claim in the scheduler | |
| 18:18:07 | dansmith | mriedem: I am | |
| 18:18:14 | mriedem | -1 it, faster :) | |
| 18:18:25 | edleafe | dansmith: 'zactly. | |
| 18:18:34 | edleafe | still think that's cleaner | |
| 18:19:50 | dansmith | edleafe: well, I'd rather we not have the conversation again, but as you wish:) | |
| 18:20:41 | edleafe | dansmith: my hero! | |
| 18:24:12 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add missing rootwrap filter for cryptsetup https://review.openstack.org/462348 | |
| 18:24:37 | openstackgerrit | Jackie Truong proposed openstack/nova master: Fix decoding of encryption key passed to dmcrypt https://review.openstack.org/462674 | |
| 18:32:35 | sdague | lyarwood - Merged openstack/requirements master: Revert "Update eventlet uc to 0.21.0" https://review.openstack.org/464664 | |
| 18:32:47 | sdague | I think you should be back in business, hopefully | |