| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-16 | |||
| 19:48:21 | dansmith | (but of course I'm fine with removing) | |
| 19:49:59 | dansmith | mriedem: ^ | |
| 19:51:08 | mriedem | https://bugs.launchpad.net/nova/+bug/1742102 | |
| 19:51:09 | openstack | Launchpad bug 1742102 in OpenStack Compute (nova) "Simple user can disable compute" [High,Confirmed] - Assigned to jichenjc (jichenjc) | |
| 19:51:15 | mriedem | see the 2nd to last comment there | |
| 19:52:39 | mriedem | trying to determine context on what happens when we call _build_failed() | |
| 19:52:43 | dansmith | mriedem: sorry I | |
| 19:52:50 | dansmith | am totally missing what hooks has to do with anything | |
| 19:52:55 | mriedem | if it's volume overquota, we don't need to count that against consecutive build failures | |
| 19:53:16 | dansmith | I get that | |
| 19:53:20 | mriedem | this is where we call _build_failed() to see if we disable the service https://github.com/openstack/nova/blob/7bdb7dbbddf9fcb4284d490bf315d6756f4015e7/nova/compute/manager.py#L2209 | |
| 19:53:32 | mriedem | at that point, we have 0 context on the actual exception that occurred | |
| 19:53:51 | mriedem | so if i wanted to inspect the BuildAbortException let's say to see if it was a volume overquota issue, i can't do that | |
| 19:54:12 | dansmith | what does that have to do with hooks? | |
| 19:54:17 | mriedem | i can't do that because, except for one base64 decode error exception, _do_build_and_run_instance doesn't raise exceptions | |
| 19:54:31 | mriedem | _do_build_and_run_instance only returns build_results.* enums which are there for hooks | |
| 19:54:36 | mriedem | https://github.com/openstack/nova/blob/7bdb7dbbddf9fcb4284d490bf315d6756f4015e7/nova/compute/manager.py#L2209 | |
| 19:55:10 | mriedem | at least that's my understanding, | |
| 19:55:22 | mriedem | so what i want is for _do_build_and_run_instance to raise it's actual failure, | |
| 19:55:32 | mriedem | so we can inspect it up the stack in _build_failed() | |
| 19:55:40 | dansmith | you know you're not actually linking me to _do_build_and_run_instance right? | |
| 19:55:41 | mriedem | which means breaking the "return build_results.FAILED" contract in _do_build_and_run_instance which is there for hooks | |
| 19:55:59 | mriedem | sorry, https://github.com/openstack/nova/blob/7bdb7dbbddf9fcb4284d490bf315d6756f4015e7/nova/compute/manager.py#L1788 | |
| 19:56:23 | mriedem | this is where we call _build_failed() https://github.com/openstack/nova/blob/7bdb7dbbddf9fcb4284d490bf315d6756f4015e7/nova/compute/manager.py#L1752 | |
| 19:57:29 | dansmith | see, I never connected hooks with those build result values | |
| 19:58:04 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: DiskAdapter parent class https://review.openstack.org/549053 | |
| 19:58:05 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300 | |
| 19:58:32 | dansmith | mriedem: what makes you think that those are there for the hooks? git history? | |
| 19:58:42 | dansmith | that said, I don't care at all about breaking that contract as you know, nor removing it | |
| 19:58:43 | dansmith | however, | |
| 19:58:50 | mriedem | i think that's what alaski told me they were for years ago when i asked | |
| 19:58:59 | dansmith | I'm not sure that unwinding all of that is a huge benefit, | |
| 19:59:14 | dansmith | and I'd say we just need another build result that is, like you said "failed, but not my fault" | |
| 19:59:35 | dansmith | that's kindof a large change to alter all that exception handling | |
| 19:59:45 | dansmith | not that it's great, but I just wouldn't want to go refactoring it myself | |
| 20:00:19 | dansmith | https://review.openstack.org/#/c/106482/ | |
| 20:00:29 | dansmith | so that's why I guess, but I had never made that connection | |
| 20:01:31 | dansmith | that is a lot more recent than I would have thought | |
| 20:01:54 | mriedem | yeah, kilo | |
| 20:01:57 | mriedem | all the andrews were in on that one | |
| 20:02:27 | mriedem | ok so i'll hack something nasty up quick and see what it looks like using the existing build_results gorp with a new enum | |
| 20:02:32 | dansmith | that was basically 2015, mitaka was early 2016 | |
| 20:05:32 | mriedem | gerrit says it's in kilo-eol | |
| 20:06:12 | dansmith | I know, it was deprecated in mitaka right? | |
| 20:13:51 | mriedem | hooks in general were deprecated in mitaka | |
| 20:14:19 | mriedem | https://github.com/openstack/nova/commit/7be56442703d071c9256267abb8acfabae642a1a | |
| 20:14:25 | mriedem | via mr dague | |
| 20:14:35 | mriedem | when he was on the "burn all extension points with fire" crusade | |
| 20:16:07 | dansmith | yeah | |
| 20:21:08 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Don't count volume over quota when disabling compute https://review.openstack.org/568953 | |
| 20:21:10 | mriedem | dansmith: ok here is the hack a mole WIP ^ | |
| 20:24:50 | dansmith | commented | |
| 20:25:10 | dansmith | there are other things, like failing to verify an image signature | |
| 20:25:18 | dansmith | which probably shouldn't count | |
| 20:25:28 | dansmith | since you could probably make that happen externally | |
| 20:26:15 | mriedem | oh there are a buttload of things | |
| 20:27:07 | mriedem | there was one i saw earlier about hitting a compute with device tags but the virt driver doesn't support devices tags (so anything besides libvirt, hyperv and xen) | |
| 20:27:20 | dansmith | hrm yeah | |
| 20:27:26 | mriedem | multiattach is in the same boat | |
| 20:27:41 | mriedem | that's why we added a policy rule on the cinder side to disable being able to make those types of volumes | |
| 20:27:53 | mriedem | if you're purely non-libvirt | |
| 20:28:22 | mriedem | or hell, if you're not using any of the supported volume types for multiattach, which in queens was only 3 out of like the 400 available cinder backends | |
| 20:34:07 | mriedem | mnaser: how much do you hate this? https://review.openstack.org/#/c/568953/ - realizing we should still be doing a volume quota check in conductor, but that could still pass during a race | |
| 20:34:36 | mriedem | dansmith: you didn't pick up on the name of my villian in the commit message | |
| 20:35:23 | dansmith | mriedem: heh, didn't even read the commit message | |
| 20:35:35 | melwitt | I liked it | |
| 20:37:24 | osnaya | Question on openstack snapshot --> Is VMS (Virtual Machine Streaming) implemented in Openstack already? Is it used underneath when a snapshot and volume snapshot created? | |
| 20:38:05 | osnaya | when you take snapshot of a live VM? | |
| 20:38:37 | osnaya | Is VMS (virtual machine streaming) implemented in Pike release? | |
| 20:41:15 | mriedem | jaypipes: if you have the number to Florida Man, i think i might have found him a soul mate across the boarder https://theconcourse.deadspin.com/woman-furiously-shits-on-floor-of-tim-hortons-throws-i-1826082960#_ga=2.237274304.1540633067.1526503254-529658380.1521051074 | |
| 20:42:20 | mriedem | osnaya: the libvirt driver supports live snapshot but you have to set a config option to enable it because it's shown to be unreliable at load with older versions of libvirt | |
| 20:43:01 | mriedem | osnaya: https://docs.openstack.org/nova/pike/configuration/config.html#workarounds.disable_libvirt_livesnapshot | |
| 20:43:28 | mriedem | bye! | |
| 20:44:56 | melwitt | mriedem: what do you mean by "we should still be doing a volume quota check in conductor, but that could still pass during a race"? if we had a check in conductor, it could pass during a race? | |
| 20:46:12 | mriedem | melwitt: yes | |
| 20:46:26 | melwitt | oh, because volumes are created by compute. okay | |
| 20:46:29 | mriedem | let's say i have volume quota of 10 (default) and spawn 10 instances in 10 separate requests | |
| 20:46:47 | mriedem | right, not min_count=10 in a single request | |
| 20:47:04 | mriedem | conductor would see usage as 0 for all 10 requests | |
| 20:47:08 | melwitt | yeah, I didn't know or forgot that volumes are created by compute | |
| 20:47:18 | mriedem | yup - we should eventually create them in conductor but... | |
| 20:47:27 | mriedem | just like we should create ports in conductor | |
| 20:47:48 | melwitt | gotcha, yeah | |
| 20:47:52 | mriedem | creating volumes in conductor would also get rid of the cross_az_attach up-call i think | |
| 20:48:42 | melwitt | I wonder why we don't do them even earlier in the api, before conductor, that would make it so we don't get ERROR state for a quota fail | |
| 20:49:08 | mriedem | because if cross_az_attach=False, and boot from volume with an existing volume, conductor can tell the scheduler we need a host in that az or fail; if nova is going to create the volume, we can create the volume in the az of the host that the scheduler picked | |
| 20:49:39 | mriedem | i used to think about creating volumes in the api, not sure why we wouldn't, we already reserve pre-existing volumes in the api | |
| 20:49:46 | mriedem | we don't create ports in the api though | |
| 20:49:54 | mriedem | i think the argument against doing it in the api is it blocks the response | |
| 20:50:02 | melwitt | ah, okay. wonder if we could just volume-update for the az case. but yeah, just thinking | |
| 20:50:06 | dansmith | mriedem: yeah and volume create can be really slow | |
| 20:50:13 | mriedem | volume create is a 202 | |
| 20:50:26 | mriedem | so nova-api would volume create, get 202 and continue to conductor | |
| 20:50:28 | dansmith | plus you want to be able to do it closer to scheduling or when you know where the instance is going to be right? | |
| 20:50:29 | mriedem | if we did it in the api | |
| 20:50:36 | dansmith | mriedem: right you could and poll later I guess | |
| 20:50:47 | mriedem | yeah we'd still do the poll for available status in compute i think | |
| 20:51:04 | mriedem | but we could catch volume over quota and az cross attach issues much earlier | |
| 20:51:23 | mriedem | i think we need to do it closer to scheduling to know which az the volume and instance have to be in if cross_az_attach=False | |
| 20:52:00 | melwitt | yeah, I was thinking if we could do a "volume update" to set the az after a volume is created but maybe that's not a thing | |