Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-04
19:40:36 mriedem mdbooth: i was looking at where _get_volume_config is used and it's used in quite a few places in the libvirt driver, like live migration, swap volume, and generally anywhere we build a guest xml, so also for resize/cold migrate,
19:40:52 mriedem and in a lot of those places, we don't have a volume to know if it's multiattach or not to set the conf.shareable flag,
19:41:19 mriedem so despite it not being great, stashing the multiattach value in the connection_info which gets saved in the bdm is about the best we can do right now
19:41:39 mriedem because all of these places in the driver that call _get_volume_config are using the connection_info off the bdm
19:53:41 sdague mriedem: nice
19:53:58 sdague mriedem: I'd totally believe it, we do extra dumb stuff in there
19:54:11 mriedem substring match is a bit slower, as expected, but still much better than doing it in nova
19:54:17 sdague mriedem: yep
19:54:28 mriedem 4000ms w/o the patch, 400ms with the patch and no substring, about 900ms with the patch and substring match
20:03:37 mriedem cdent: edleafe: it's your favorite game show, "IS IT 400 OR 409?!"
20:04:01 mriedem so, say i'm trying to attach a multiattach volume to an instance and the compute is too old to support multiattach,
20:04:13 mriedem is that a 400, or a 409? knowing that at some point the compute might be upgraded to support multiattach.
20:06:14 cdent the state of the compute is out of sync with the desired state, but you made a valid request (had it been in the right state), so I'd go with 409
20:06:28 mriedem artom: remember how we punted for tagged attached on a shelved offloaded instance because we couldn't tell if the host supported it?
20:06:39 mriedem cdent: ok that's what i was thinking too
20:06:54 cdent \o/
20:07:09 mriedem artom: but we don't make that same distinction in the API for tagged bdms with boot from volume
20:07:17 mriedem can you remember why?
20:08:52 mriedem artom: my guess was "you could reschedule and attempt to hit a host that does support device tagging" but we raise BuildAbortException so we don't even reschedule in that case
20:13:40 openstackgerrit melanie witt proposed openstack/nova master: Add access_url_base to console_auth_tokens table https://review.openstack.org/334614
20:13:40 openstackgerrit melanie witt proposed openstack/nova master: Optionalize instance_uuid in console_auth_token_get_valid() https://review.openstack.org/481700
20:13:41 openstackgerrit melanie witt proposed openstack/nova master: Add ConsoleAuthToken object https://review.openstack.org/320063
20:13:41 openstackgerrit melanie witt proposed openstack/nova master: Add periodic task to clean expired console tokens https://review.openstack.org/325381
20:13:42 openstackgerrit melanie witt proposed openstack/nova master: Use ConsoleAuthToken object to generate authorizations https://review.openstack.org/325414
20:13:42 openstackgerrit melanie witt proposed openstack/nova master: Convert websocketproxy to use db for token validation https://review.openstack.org/333990
20:15:29 edleafe yeah, the request isn't malformed; the system can't support it
20:16:12 artom mriedem, yeah, I think your guess is correct
20:16:14 cdent support it _right now_
20:16:44 mriedem artom: ok, so that's not really any different from punting when trying to attach a volume to a shelved offloaded instance with tags
20:16:58 mriedem artom: i'm only asking b/c we're going to have a similar situation with multiattach volumes
20:17:31 mriedem trying to decide if we should support attaching a multiattach volume to a shelved offloaded instance - same with bfv, either way it might fail once we get to the compute and you're dead
20:18:08 mriedem at some point in the future, which no one will probably ever work on, we could do a scheduler filter for this stuff to make it pick a host which supports the thing you need
20:18:17 artom With the full 100% knowledge of every bit of context about multiattach, I'd vote to be consistent and refuse it for shelved offloaded
20:18:19 mgagne if running mitaka, is legacy v2 API still used or is it dead code?
20:18:39 artom Because we have no idea what host it'll end up on
20:18:43 artom When it's unshelved
20:18:51 mriedem artom: the same is true for boot from volume
20:19:09 artom mriedem, eh, how so?
20:19:13 mriedem but we allow tagged bdms with bfv
20:19:33 mriedem when you create a server, or unshelve a server, they both go through the scheduler to pick a host,
20:19:46 mriedem you have an equal chance in either scenario of picking a host that doesn't support the capability
20:20:05 mriedem so the fact we allow tagged bdms with bfv but not shelved offloaded instances is inconsistent
20:20:12 artom mriedem, how is bfv different from normal volume tagging? IIRC the api just checked service level, to see if the cloud was fully upgraded to support it
20:20:17 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: Attach and detach encryptors during swap_volume https://review.openstack.org/531233
20:21:31 mriedem artom: ok yeah that's the one difference then, is that we check in the api when creating a server if the computes are new enough to handle tagged bdms
20:21:43 mriedem we could easily have done the same for shelved offloaded attach
20:22:03 artom mriedem, so there's this 2 x 2 matrix of stuff
20:22:10 artom we have tagged boot / tagged attach
20:22:11 mriedem that's basically what i was saying we'd do in those two cases for multiattach too - check the min compute service version and fail if computes aren't upgraded yet
20:22:20 artom And compute manager supports it / virt driver supports it
20:22:57 artom For tagged boot, we used the service level check to see if compute manager supports it
20:23:12 artom For tagged boot/virt driver... I forget how we handled it
20:23:21 mriedem we just fail in the compute
20:23:24 mriedem and raise BuildAbortException
20:23:50 artom Ah, right, hopefully rescheduling to a virt driver that supports it
20:23:57 mriedem no, we don't reschedule
20:24:09 mriedem BuildAbortException means abort, don't reschedule
20:24:14 artom Ah, ok
20:24:28 artom Yeah, that's not awesome, but no other way of doing it I guess
20:24:31 mriedem BuildRescheduledException means kick edleafe in the head a few times
20:24:43 artom And then for tagged attach...
20:25:05 artom I think they were all RPC calls down to the compute
20:25:13 artom So virt driver support was straightforward
20:25:25 artom Except for shelved offloaded, which was a cast
20:25:28 artom (Obviously)
20:25:28 mriedem yeah that one is easy
20:25:35 mriedem unless the server is shelved offloaded
20:25:42 artom So that one we decided to just fail in the API
20:25:52 mriedem in the case of shelved offloaded, we don't do anything with the compute b/c there is no compute
20:25:58 artom Instead of having the unshelve fail like, 3 years later
20:26:13 mriedem yeah, i'm saying, i think at a minimum we could do the same for shelved offloaded as we do for tagged boot which is check the service version
20:26:39 mriedem we don't reschedule on an unshelve failure, but we don't reschedule on a tagged boot failure either
20:26:43 mriedem so it's basically the same
20:26:57 artom One is definitely more immediate than the other
20:27:22 artom Though I guess we if we relay the exception properly, it's not a massive deal
20:27:22 edleafe mriedem: so it seems that the bug I found was the result of a race
20:27:32 edleafe The call to retry on https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4087 runs the retry before the 'finally:' clause reight below it gets hit.
20:27:44 edleafe If I add the call the self._revert_allocation() before L4087, the test I wrote passes.
20:27:53 artom Like, if nova show unshelved-instance has 'you unshelved on a virt that doesn't support tagging' somewhere in there (but more cloudy) it can work
20:28:14 mriedem edleafe: ah just like the thing we hit in the other patch
20:28:23 mriedem need to cleanup the allocation before casting to conductor
20:28:56 mriedem artom: we'd record a fault
20:28:58 mriedem that's about it
20:29:00 edleafe mriedem: do you have a fix for the caching scheduler? Or should I do that?
20:29:16 artom mriedem, is that obvious to the user?
20:29:18 mriedem edleafe: https://review.openstack.org/#/c/531220/
20:29:31 mdbooth mriedem: My patch series removes it from swap_volume, btw, but not surprised by the others.
20:29:32 mriedem artom: not really, the fault's traceback is only available to admins
20:29:55 mriedem mdbooth: swap_volume is the one case where i don't think this matters because we don't rely on the conf.shareable attribute
20:29:55 artom mriedem, so give this lack of feedback to the user, it's probably better to fail fast in the API, no?
20:30:23 mriedem artom: maybe
20:30:29 mriedem i can see the argument for being consistent
20:30:34 mdbooth mriedem: Ok, so how can we flag this as a second connection_info wart?
20:30:47 mriedem mdbooth: 2nd as in device_path is #1?
20:30:52 artom mriedem, what's the issue we see with multiattach though?
20:31:03 mdbooth I mean there's this and also device_path
20:31:13 edleafe mriedem: ah, I just saw the test patch
20:31:19 mdbooth mriedem: Right, yeah. What you said :)
20:31:21 artom I haven't followed closely, so it may or may not be worth it to dump all the context on me for whatever my opinion is worth ;)
20:31:35 mriedem artom: nothing - i'm just weighing options since we don't have the api plumbed in for all of the multiattach stuff

Earlier   Later