Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-02-14
16:25:43 mnaser the "Task pause timed out" is a heat thing so i'll look into why it didn't attempt to delete before retry
16:26:15 mriedem so if heat deletes the failed instance, nova can clean up the volume, but that's probably where we need https://review.openstack.org/#/c/340614/ i think
16:27:07 mnaser mriedem: i think heat's normal behaviour is to just set the stack to failed
16:27:11 mnaser and not retry
16:27:29 mnaser googling "Task pause timed out" yields nothing so that's a nice start, but we're in heat problem land
16:28:12 mriedem ok, but even if heat did try to delete the instance before retrying, the 2nd attempt will likely fail because the volume is already reserved because of the bug that https://review.openstack.org/#/c/340614/ is trying to fix
16:30:42 cfriesen does anyone know off the top of their head which operations would cause an instance with a qcow2-backed rootfs (that normally has the glance image file as a backing file) to get flattened?
16:31:30 mnaser mriedem: yeah that's ideal, looks like melwitt was doing work on that, i can pick it up and work on whats missing if melwitt is busy with other things
16:32:28 mriedem mnaser: it's ready for review, just needs review
16:32:36 mriedem that patch has a long history
16:32:44 mriedem of merging, breaking hte gate, getting reverted, repeat
16:33:07 mnaser mriedem: there was a comment suggesting removing instance.host is None that's why i asked
16:33:34 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove old flavor_access_get_by_flavor_id db api method https://review.openstack.org/539720
16:33:35 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove old flavor_destroy db api method https://review.openstack.org/539729
16:33:35 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove old flavor_get_by_flavor_id db api method https://review.openstack.org/539733
16:33:36 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove old flavor_get_by_name db api method https://review.openstack.org/544379
16:47:03 cfriesen mriedem: do you know of any work in progress to deal with https://bugs.launchpad.net/nova/+bug/1583145 or https://bugs.launchpad.net/nova/+bug/1644248 ?
16:47:05 openstack Launchpad bug 1644248 in OpenStack Compute (nova) ocata "duplicate for #1583145 Nova incorrectly tracks live migration progress" [High,In progress] - Assigned to Matt Riedemann (mriedem)
16:47:06 openstack Launchpad bug 1644248 in OpenStack Compute (nova) ocata "Nova incorrectly tracks live migration progress" [High,In progress] - Assigned to Matt Riedemann (mriedem)
16:50:39 jaypipes mriedem_afk, dansmith: https://review.openstack.org/#/c/539712/ is pretty straightforward.
16:59:41 dansmith melwitt: mriedem_afk isn't around for the cells meeting, we talked to tssurya and belmiro earlier and neither are online now, so I assume we're canceling
17:13:14 cdent jaypipes: if you're chasing easy wins: https://review.openstack.org/#/c/536784/
17:52:29 openstackgerrit Merged openstack/python-novaclient master: Updated from global requirements https://review.openstack.org/544282
18:09:23 mnaser so it looks like this bug wasn't visible because we had old disabled but inactive pre-pike compute nodes
18:09:35 mnaser removing them made the minimum pike and landed us in this situation
18:15:00 openstackgerrit Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585
18:16:48 mnaser "The missing 'reserve_volume' call is added to the BFV flow. In case of build failure the volume will be locked in 'attaching' state until the instance in ERROR state is cleaned up." -- i guess the problem here is that it doesn't account for scheduling failures :x
18:31:25 openstackgerrit Merged openstack/nova master: Updated from global requirements https://review.openstack.org/544277
18:32:31 openstackgerrit Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585
18:32:51 mriedem_afk mnaser: i want to say that "so it looks like this bug wasn't visible because we had old disabled but inactive pre-pike compute nodes" was also a bug fix
18:33:57 mriedem mnaser: oh nvm we excluded forced_down services, but we do include disabled services in that min version check
18:34:13 mriedem i was thinking of this so it looks like this bug wasn't visible because we had old disabled but inactive pre-pike compute nodes
18:34:16 mriedem oops
18:34:19 mriedem https://github.com/openstack/nova/commit/c79770e615799cd4457ac603dcad4fb3452fe2bc
18:35:52 mnaser mriedem: yeah so right now i'm not too sure why this occurs because it looks like if it fails to schedule, it means when nova attempts to delete it, it does a local delete, which calls '_local_cleanup_bdm_volumes'
18:36:06 mnaser or maybe im wrong?
18:36:38 mnaser because if it fails to schedule, its in cell0 so deletes will be a local delete based on what i think i understand from the code
18:36:39 mriedem mnaser: presumably that's why melwitt has https://review.openstack.org/#/c/340614/
18:38:36 mriedem i think the problem is that we do find the error'ed instance in cell0,
18:38:45 mriedem and delete it in the api, but return early and don't cleanup bdms or ports
18:39:08 mriedem here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1805
18:39:16 mriedem and then we exit here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1809
18:42:23 mnaser mriedem: i dont know if logically this is clean but i was thinking maybe cleanup bdms in the _bury_in_cell0 codebase?
18:42:48 mnaser because that will also release the volume right after failure, rather than until the volume is deleted
18:43:30 mriedem yeah idk, we end up with a lot of cleanup volume logic strewn all over nova then
18:43:35 mriedem b/c we already have some of this in nova-compute too
18:43:39 mriedem and api already
18:44:00 mnaser yeah thats what was making it a bit iffy for me is the fact that now multiple components are handling reserve/cleanup (which is probably how we ended up in this place)
18:47:48 mnaser mriedem: i think what you mentioned isn't the case, because that 'return' happens if there is an instance.host, and in this case, the error'd instance probably doesnt have one set if it failed to be scheduled
18:48:14 mnaser but thats what i think based on what little understanding of the code
18:48:29 melwitt if the instance fails to schedule, it shouldn't have done anything with the volumes yet, right?
18:49:13 mnaser melwitt: the api layer calls cinder reserve volume when it gets the request
18:49:35 melwitt mnaser: oh :\
18:50:12 mnaser melwitt: not sure if your patch fixes it (or if its technically another bug), but this was my write up - http://paste.openstack.org/show/672535/
18:50:42 mriedem mnaser: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1787
18:51:02 mriedem we only go down that route if the instance doesn't have a host yet
18:51:16 mnaser mriedem: my bad, the comment above it tricked me
18:51:35 mriedem yeah, it's confusing, the "Normal delete should be attempted." means "skip this weird block"
18:52:03 mriedem so i think that's likely the issue, is we get the instance from cell0, and delete it in this block of code and return w/o dealing with volumes/ports
18:52:15 mriedem granted we shouldn't have any ports if it's never been scheduled to a host
18:52:21 mriedem as ports are created/bound in compute
18:52:31 mriedem but volumes is definitely a gap here
18:52:32 mnaser mriedem: i think so too, because if i follow the rest of the code, it should technically do a local_delete which does clean up bdms
18:52:40 mriedem yup
18:55:22 mnaser catch-22... if compute reserves volume, it means api layer won't give the error right away.. if api reserves volume, get an error right away but have to get it cleaned up later
19:01:12 mnaser im not even sure if this issue exists in queens
19:01:32 mnaser looks like it creates an attachment rather than reserve a volume
19:02:47 mriedem dansmith: so i think i get it, but https://review.openstack.org/#/c/544585/
19:03:17 mriedem mnaser: it does exist in queens, because we don't delete the attachment, same thing as not unreserving
19:03:26 mriedem a volume with an empty attachment is in 'attaching' status just like reserve
19:03:44 dansmith mriedem: in my playing around code, I have a note to self to go find that key and see if I can use it :)
19:03:45 melwitt mnaser: with my patch, I think you'd still see the volume stuck in 'attaching' until you delete the instance
19:04:09 mriedem melwitt: yes you would b/c we don't cleanup volumes in conductor
19:04:10 melwitt without the patch, even after you delete the instance, the volume would be stuck. with the patch, the volume should be released
19:04:14 melwitt after deleting
19:06:05 mnaser melwitt: so _has_been_scheduled(instance) would return true (because vm_state == ERROR), which means that we would enter that `if not has_been_scheduled:` conditional. however, as mriedem pointed out, in your patch, line #1809 it returns so it doesn't follow the rest of the code
19:06:45 mnaser which means it never makes it to the code under it which does clean ups etc (if i understand correctly)
19:07:30 mriedem it should be fairly easy to have a functional test to recreate that
19:07:50 melwitt mnaser: if _has_been_scheduled is True, then it would not enter 'if not _has_been_scheduled'
19:08:02 mnaser gah
19:08:04 mriedem the functional tests that melwitt updated in that patch likely aren't doing boot from volume
19:08:10 mnaser these programming 101 mistakes
19:10:15 melwitt I have boot from volume unit tests, but need to check if any of the functional tests covering boot from volume could be enhanced to check this
19:11:31 melwitt originally I hadn't updated any functional tests in the patch, something changed recently to where we needed to manually start the consoleauth service. not yet sure what caused that change
19:11:34 mriedem we don't have many bfv functional tests, but a couple
19:11:48 mriedem local delete will try to delete console auth entries
19:12:03 mriedem since you weren't going down that path before your change, that's why those tests didn't need it
19:12:27 mriedem here is an easy example test https://review.openstack.org/#/c/543257/
19:12:29 melwitt yeah, I mean, in previous revisions I didn't need it
19:12:46 mnaser i think `test_delete_error_state_with_no_host` covers this case, which makes sure that local_delete is actually being called (and local delete does clean up bdms)
19:13:33 mriedem mnaser: that's a unit test
19:13:48 mriedem we really should have a functional regression test for this, given the various service interactions and cell0 stuff
19:14:08 mnaser is there an example of functional regression tests involving other services (i.e. cinder)
19:14:17 mriedem https://review.openstack.org/#/c/543257/
19:14:17 mnaser i dont mind trying to write one if that helps
19:16:17 mnaser melwitt: if you have other things at hand, i can write a functional regression test for this right now
19:17:12 melwitt mnaser: sure, sounds good, thanks for the help
19:17:40 mnaser ok perfect, i'll work on it locally and then i can tack it on top of the change
19:21:02 mriedem ideally the regression test patch comes before the fix,
19:21:13 mriedem to show that the volume is still 'attaching' even after we delete the instance

Earlier   Later