Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-28
17:45:23 openstackgerrit Eric Fried proposed openstack/nova master: Use ksa adapter for cinder client https://review.openstack.org/508345
17:45:32 efried mriedem: Let's see how that shakes out ^
17:47:08 openstackgerrit Merged openstack/nova stable/queens: libvirt: mask InjectionInfo.admin_pass https://review.openstack.org/548289
17:51:36 mriedem efried: hmm, we do need a backportable fix for bug 1752152 though
17:51:36 openstack bug 1752152 in OpenStack Compute (nova) "Attach Volume Fails with secure call to cinder" [Undecided,In progress] https://launchpad.net/bugs/1752152 - Assigned to Eric Fried (efried)
17:51:53 mriedem i also don't understand why our CI jobs don't fail with that bug
17:52:40 efried mriedem: The problem with backportability is that I can't use the _SESSION if I... don't have a _SESSION.
17:53:28 efried mriedem: It's a chicken/egg: I would have to use the CinderClient to make the request, cause that guy handles https already. But the microversion check is *before* the CinderClient is created. Soooo....
17:53:59 efried mriedem: Are you sure the CIs are using https? If it were me, I would have switched that off first thing to make things easier.
17:54:09 mriedem i thought there was a backportable way to fix this when i was posting stuff in the bug report before, but that's exited my brain so would have to look at all of this again, but in the middle of something
17:54:36 mriedem pretty sure yes
17:54:37 mriedem http://logs.openstack.org/45/508345/13/check/nova-next/aa61d86/logs/screen-c-api.txt.gz#_Mar_15_20_00_06_201395
17:54:38 efried mriedem: The backportable way would have been to s/https/http/ for the version discovery.
17:54:45 efried which is not a good solution.
17:55:35 efried mriedem: I'm not well versed on ssl etc, but isn't there a way to use https without a cert file?
17:55:50 mriedem don't konw
17:56:03 dansmith efried: no
17:56:20 mriedem another thing i mentioned as a hack workaround for backports, is use the internal cinderclient.Client.session
17:56:23 mriedem to get the version doc
17:56:30 mriedem then we replace all of that with the KSA thing in master
17:56:49 mriedem then it's just a session.get()
17:57:15 efried cinderclient.Client.session doesn't exist until we've created the client though, does it?
17:57:50 efried that's the chicken/egg
18:03:14 efried mriedem: (I know you're busy, but when you're available...) Any reason not to move the microversion check to after the client construction?
18:03:25 mriedem i think that was my idea from the bug report
18:03:31 mriedem "The alternative is on the nova side, we just construct a cinderclient Client object and use it's internal client (session) to make a request, or use a keystoneauth1 adapter to make the request."
18:05:07 efried okey, I'll see if I can work that up.
18:29:30 openstackgerrit Dan Smith proposed openstack/nova master: Add aggregates list to Destination object https://review.openstack.org/544729
18:29:30 openstackgerrit Dan Smith proposed openstack/nova master: Make get_allocation_candidates() honor aggregate restrictions https://review.openstack.org/547990
18:29:31 openstackgerrit Dan Smith proposed openstack/nova master: Add an index on aggregate_metadata.value https://review.openstack.org/555851
18:29:31 openstackgerrit Dan Smith proposed openstack/nova master: Add AggregateList.get_by_metadata() query method https://review.openstack.org/544728
18:29:32 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
18:29:33 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Honor availability_zone hint via placement https://review.openstack.org/546282
18:43:05 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
19:02:10 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
19:15:54 openstackgerrit Mathieu Gagné proposed openstack/nova-specs master: Multiple Fixed-IPs support in network information https://review.openstack.org/312626
19:17:30 openstackgerrit melanie witt proposed openstack/nova master: Convert websocketproxy to use db for token validation https://review.openstack.org/333990
19:17:31 openstackgerrit melanie witt proposed openstack/nova master: Update layout docs for running console proxies https://review.openstack.org/557489
19:20:46 openstackgerrit melanie witt proposed openstack/nova master: Convert websocketproxy to use db for token validation https://review.openstack.org/333990
19:20:46 openstackgerrit melanie witt proposed openstack/nova master: Update layout docs for running console proxies https://review.openstack.org/557489
19:21:25 dansmith mriedem: do you have a recent devstack running by chance?
19:21:28 dansmith (or anyone)
19:21:52 mriedem nope
19:23:36 dansmith okay I was asking you since I'm writing the docs you demand and I don't have a recent one myself
19:30:07 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Honor availability_zone hint via placement https://review.openstack.org/546282
19:30:08 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Documentation for tenant isolation with placement https://review.openstack.org/557490
19:33:49 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
19:33:50 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Documentation for tenant isolation with placement https://review.openstack.org/557490
19:33:50 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Honor availability_zone hint via placement https://review.openstack.org/546282
19:39:34 efried mriedem, jgriffith: https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3964 Is this here to avoid a DB lookup?
19:40:35 efried It looks to me like the guts of _check_volume_already_attached_to_instance don't actually depend on cinder microversion 3.44 -- and we're just doing this because, if 3.44 isn't supported, we couldn't possibly have >1 volume attached, so there's no need to do _check_volume_already_attached_to_instance.
19:40:43 jgriffith efried: well, no.. not really; the cinder mv can be dynamic
19:41:42 jgriffith efried: yeah, once that attachment is made we use the bdm entry for determining which flow is in use
19:42:00 jgriffith efried: sorry, I might not understand completely what you're looking for
19:42:02 mriedem efried: the point is the comment in the else block below it
19:43:08 jgriffith Oh, specifically that call right there :)
19:44:23 mriedem i'm having a hard time remembering too
19:44:28 efried Lemme make sure I have the logic right:
19:44:28 efried If multiattach not supported by cinder, proceed.
19:44:28 efried Otherwise, fail if the volume is attached.
19:44:41 mriedem 3.44 isn't about multiattach
19:44:45 mriedem it starts with https://review.openstack.org/#/c/525622/
19:44:56 mriedem with the new attachments api stuff, cinder will let us create multiple attachments between the same volume and instance
19:45:09 mriedem to enable things like live migration where the instance is attached to the volume on different hosts
19:45:24 mriedem but because of that, we found we had a bug where you could attach the same volume to the instance via the api multiple times
19:45:30 mriedem so we had to add a specific check for that
19:46:07 jgriffith mriedem: yeah, the comment actually is pretty good :)
19:46:07 mriedem before using new style attachments, cinder would not allow this
19:46:18 mriedem that's why in https://review.openstack.org/#/c/525622/ we have to rely on the bdm table
19:47:54 efried Ultimately I was trying to figure out if I could get rid of that microversion check :) I think I now understand why I can't.
19:47:55 mriedem so going back to https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3964
19:48:20 efried Which kinda sucks, because it means I'm gonna have to build up a whole cinder client just to do that microversion check.
19:48:24 mriedem if 3.44 is supported, we know we're doing the new style attachment_create, and we have to make sure the instance isn't already attached to that volume, so we call _check_volume_already_attached_to_instance
19:48:34 mriedem if 3.44 isn't available, we'll do the old style reserve flow
19:48:49 mriedem which cinder enforces you can only have the instance attached to the volume once
19:49:01 mriedem efried: yeah but that is only temporary for the backport,
19:49:12 mriedem and we can replace that stuff with ksa in master right?
19:49:34 efried mriedem: Except I still haven't figured out how to get ksa stuff working for cinder, period.
19:49:41 mriedem heh
19:49:42 mriedem well,
19:49:47 mriedem it's just code right?!
19:49:56 efried Sigh. Right.
19:50:02 mriedem sylvain's daughter will have it done by tomorrow
19:50:19 efried I know, I can't let her show me up like that. Not again.
19:54:57 dansmith cripes
19:56:11 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
20:12:46 mriedem dansmith: not sure if you care about any of the comments in https://review.openstack.org/#/c/544730/15
20:14:45 dansmith mriedem: sorry hadn't seen those
20:14:46 dansmith replied
20:18:25 mriedem i've got this nova-status check ready to go basically for the cells api service version check thing, i think i'll just munge the api log warning into the same patch
20:20:19 dansmith seems reasonable
20:38:30 openstackgerrit Dan Smith proposed openstack/nova master: Add request filter functionality to scheduler https://review.openstack.org/544730
20:38:31 openstackgerrit Dan Smith proposed openstack/nova master: Add aggregates list to Destination object https://review.openstack.org/544729
20:38:31 openstackgerrit Dan Smith proposed openstack/nova master: Make get_allocation_candidates() honor aggregate restrictions https://review.openstack.org/547990
20:38:32 openstackgerrit Dan Smith proposed openstack/nova master: Add an index on aggregate_metadata.value https://review.openstack.org/555851
20:38:32 openstackgerrit Dan Smith proposed openstack/nova master: Add AggregateList.get_by_metadata() query method https://review.openstack.org/544728
20:38:33 openstackgerrit Dan Smith proposed openstack/nova master: Add require_tenant_aggregate request filter https://review.openstack.org/545002
20:38:33 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Documentation for tenant isolation with placement https://review.openstack.org/557490
20:38:34 openstackgerrit Dan Smith proposed openstack/nova master: WIP: Honor availability_zone hint via placement https://review.openstack.org/546282
20:40:36 mikal mriedem: yeah, I can be around to interate on privsep for the next couple of weeks. Noting that both of the next two weeks are short because of easter.

Earlier   Later