| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-14 | |||
| 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 | mnaser | i dont mind trying to write one if that helps | |
| 19:14:17 | mriedem | https://review.openstack.org/#/c/543257/ | |
| 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: Implement certificate_utils https://review.openstack.org/479949 | |
| 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: Add trusted_certs to instance_extra https://review.openstack.org/537897 | |
| 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: Convert websocketproxy to use db for token validation https://review.openstack.org/333990 | |
| 20:02:24 | openstackgerrit | melanie witt proposed openstack/nova master: Use ConsoleAuthToken object to generate authorizations https://review.openstack.org/325414 | |
| 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 | |
| 20:08:29 | mnaser | my little functional test so far is failing because: HTTP exception thrown: Invalid volume: volume 'f8580fcb-b22d-4e50-b69a-d3d91f8f3a6b' status must be 'available'. Currently in 'detached' | |
| 20:08:33 | mriedem | mnaser: yes | |
| 20:08:51 | mnaser | ok, ill fix that first | |
| 20:08:53 | mriedem | mnaser: same problem here https://github.com/openstack/nova/blob/master/nova/tests/fixtures.py#L1574 | |
| 20:09:14 | melwitt | mnaser: is that what I proposed here? https://review.openstack.org/#/c/539758/ | |
| 20:09:28 | mnaser | melwitt: exactly that | |
| 20:09:38 | mnaser | ill rebasemy work on that, thank you | |
| 20:09:42 | mriedem | lemme look, we could rebase quick | |
| 20:09:50 | melwitt | I ran into the same problem recently working on func tests | |
| 20:09:58 | mnaser | melwitt: do we want to add in-use for volume status too? | |
| 20:10:22 | mnaser | but that might be a bit more work | |
| 20:10:36 | mnaser | because im not sure what the status is when using multiattach (in-use or available?) | |
| 20:10:51 | melwitt | mnaser: we might. I didn't try to go further with changing the fixture | |
| 20:10:59 | mriedem | i just left that comment | |
| 20:11:05 | mriedem | if there are attachments, the status should be at least 'in-use' | |
| 20:11:11 | melwitt | once it's attached, it should be 'in-use' from what I understand | |
| 20:11:18 | mriedem | it might not be technically correct, but it's more correct than saying 'available' | |
| 20:11:25 | mnaser | yeah, makes sense | |