Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-02-14
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
19:21:30 mriedem then mel's patch goes on top of that and changes the test to show the volume is properly unreserved
19:21:59 mriedem the cinder fixture we have should be able to give you the volume state tracking and API support for figuring that out
19:24:56 mnaser https://git.openstack.org/cgit/openstack/nova/tree/nova/tests/functional/regressions/test_bug_1675570.py
19:25:03 mnaser well this is nice to have, heh
19:26:13 openstackgerrit Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585
19:33:02 mriedem mnaser: yeah you could probably build on that with a bfv scenario, and nix the shelve offload part
19:36:18 openstackgerrit Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585
19:37:36 mriedem dansmith: couple small docs issues and then i'm +2
19:38:50 mriedem i really like the idea of using this for multiattach bfv
19:39:07 mriedem assuming people don't hate the idea of storing the compute driver capabilities as traits
19:39:23 mriedem we can also use it for tagged bfv
19:39:27 mriedem which artom should like
19:40:17 artom I hope I wouldn't be the only one
19:40:30 artom It seems that sort of thing has come up for other problems
19:40:39 dansmith mriedem: jaypipes was also saying this will let us clean up the pci request -> nets mapping thing, which sounds nice
19:40:44 dansmith I have to go dig back into that stuff to see though
19:41:23 openstackgerrit Dan Smith proposed openstack/nova-specs master: Add placement-req-filter spec https://review.openstack.org/544585
19:45:10 openstackgerrit Jackie Truong proposed openstack/nova master: Add trusted_certs to instance_extra https://review.openstack.org/537897
19:45:10 openstackgerrit Jackie Truong proposed openstack/nova master: Add trusted_certs object https://review.openstack.org/489408
19:45:10 openstackgerrit Jackie Truong proposed openstack/nova master: Implement certificate_utils https://review.openstack.org/479949
19:45:11 openstackgerrit Jackie Truong proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204
19:49:51 mriedem jackie should be in irc
19:50:02 openstackgerrit melanie witt proposed openstack/nova-specs master: Re-propose convert consoles code to use objects framework https://review.openstack.org/543662
19:53:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove old flavor_get db api method https://review.openstack.org/544621
19:54:44 dansmith mriedem: related to ^, I'll remove aggregate-in-cell-db compatibility before I add the methods to AggregateList that I need
19:54:55 dansmith just so you don't do it if you were looking for deprecations to remove
19:59:00 mriedem dansmith: i had just been slowly working on this list https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L385
19:59:05 mriedem last thing i removed was the flavor compat
19:59:26 mriedem or maybe it was keypairs, don't remmeber
19:59:30 dansmith mriedem: okay well, I've got the two aggregate ones in my sights
19:59:44 mriedem ok wfm,
19:59:50 mriedem we can definitely remove these old newton compat things
19:59:55 mriedem s/compat/migration/
20:00:02 dansmith yeah
20:02:23 openstackgerrit melanie witt proposed openstack/nova master: Add periodic task to clean expired console tokens https://review.openstack.org/325381
20:02:24 openstackgerrit melanie witt proposed openstack/nova master: Use ConsoleAuthToken object to generate authorizations https://review.openstack.org/325414
20:02:24 openstackgerrit melanie witt proposed openstack/nova master: Convert websocketproxy to use db for token validation https://review.openstack.org/333990
20:03:46 mriedem anyone want to send this home and the one below it? https://review.openstack.org/#/c/544066/ - the cinder admin guide patch for multiattach is dependent on this
20:04:00 mriedem i think we should probably backport that to queens too
20:07:44 mnaser hey .. is this a bug in the cinder fixtures? https://github.com/openstack/nova/blob/master/nova/tests/fixtures.py#L1390-L1394 .. shouldn't it modify volume['attach_status'] and not volume['status']?
20:07:59 mnaser so it should be volume['status'] = 'in-use' if has_attachment else 'available'
20:08:07 mnaser volume['attach_status'] = 'attached' if has_attachment else 'detached'
20:08:09 mriedem jogo: can you remove your -2 on this? https://review.openstack.org/#/c/159382/1

Earlier   Later