| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-28 | |||
| 17:12:30 | mriedem | mgagne: probably unwritten, | |
| 17:12:33 | dansmith | mriedem: that was because we had some problem.. it was an exception but I don't recall the details | |
| 17:12:36 | mriedem | but b/c it doesn't have microversions, that's about the only option | |
| 17:13:24 | melwitt | yeah, I'm guessing cloud-init and friends don't do anything to specify a version, so if you upgrade metadata API and compat is broken, everything breaks if you haven't grabbed the latest cloud-init that can handle it | |
| 17:13:52 | mriedem | if you don't specify a version, | |
| 17:13:53 | mgagne | what's microversion? a number you can increase to indicate a change? it's already done with date, just a different format. other than a different philosophical pov, I don't see the difference. | |
| 17:14:00 | mriedem | i believe you get a versioned dict back | |
| 17:14:13 | mriedem | or maybe that's just config drive | |
| 17:14:31 | mriedem | having said all this, mikal should probably be roped in | |
| 17:14:44 | mriedem | he's an old school metadata API guy | |
| 17:14:52 | mgagne | oh, I'm not familiar with metadata api at all, I only consume configdrive which does have all date/version in there | |
| 17:14:54 | melwitt | yeah, I was about to say, we need a mikal | |
| 17:16:04 | dansmith | I don't think we do | |
| 17:16:09 | dansmith | I mean, we need him in the cosmic sense | |
| 17:16:16 | dansmith | but we need to be additive here | |
| 17:17:41 | mgagne | ok, I don't mind update spec | |
| 17:18:07 | mgagne | but this needs to be documented somewhere because I thought for years that you could break compat | |
| 17:18:55 | melwitt | agreed, we should add explanation of that under the existing comment above the version list, at least | |
| 17:19:06 | mgagne | and cloud-init consumes configdrive by date: https://github.com/cloud-init/cloud-init/blob/master/cloudinit/sources/helpers/openstack.py | |
| 17:19:53 | melwitt | I'd appreciate a sanity check from mikal since those comments about the versioning are from him | |
| 17:22:36 | dansmith | mgagne: it also has a latest, which we honor and use the latest field | |
| 17:22:48 | openstackgerrit | Merged openstack/nova stable/queens: Fix and update compute schedulers config guide https://review.openstack.org/548873 | |
| 17:23:37 | mgagne | dansmith: yes and IMO, it's like using master from git, if you want stability/predictability, don't use it. | |
| 17:30:08 | melwitt | lyarwood, dansmith: may I get reviews on this stable backport pls https://review.openstack.org/#/c/550498 for saving admin password to sysmeta | |
| 17:31:27 | melwitt | er sorry, didn't realize queens backport didn't merge yet. dansmith https://review.openstack.org/#/c/550489 instead | |
| 17:33:01 | lyarwood | melwitt: np, noted in the review for now | |
| 17:33:12 | melwitt | lyarwood: perfect thanks | |
| 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 | 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:36 | mriedem | efried: hmm, we do need a backportable fix for bug 1752152 though | |
| 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: Make get_allocation_candidates() honor aggregate restrictions https://review.openstack.org/547990 | |
| 18:29:30 | openstackgerrit | Dan Smith proposed openstack/nova master: Add aggregates list to Destination object https://review.openstack.org/544729 | |
| 18:29:31 | openstackgerrit | Dan Smith proposed openstack/nova master: Add AggregateList.get_by_metadata() query method https://review.openstack.org/544728 | |
| 18:29:31 | openstackgerrit | Dan Smith proposed openstack/nova master: Add an index on aggregate_metadata.value https://review.openstack.org/555851 | |
| 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: 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: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: Honor availability_zone hint via placement https://review.openstack.org/546282 | |
| 19:33:50 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: Documentation for tenant isolation with placement https://review.openstack.org/557490 | |
| 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 | Otherwise, fail if the volume is attached. | |
| 19:44:28 | efried | If multiattach not supported by cinder, proceed. | |
| 19:44:28 | efried | Lemme make sure I have the logic right: | |
| 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 | mriedem | before using new style attachments, cinder would not allow this | |
| 19:46:07 | jgriffith | mriedem: yeah, the comment actually is pretty good :) | |
| 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 | |