| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-03 | |||
| 19:28:56 | mriedem | i was thinking about this https://github.com/openstack/nova/blob/master/nova/conf/placement.py#L31 | |
| 19:29:02 | sdague | mriedem: yeh | |
| 19:29:13 | mordred | yah - I'd argue to not have that | |
| 19:30:09 | mordred | internal and public are known interfaces. nova should reflect urls based on public to API consumers and should use internal to talk to other services - we shouldn't make operators tell nova to do that- if they don't want an internal url, they should just register the same url twice with keystone | |
| 19:30:10 | sdague | mordred: well, we tried to get away with be prescriptive with placement | |
| 19:30:12 | mordred | but I'm unreasonable | |
| 19:30:19 | sdague | and people got mad | |
| 19:30:23 | mordred | of course they did | |
| 19:31:02 | sdague | mordred: so, lots of deployments only use public | |
| 19:31:09 | sdague | or at least some do | |
| 19:31:17 | sdague | and just route correctly | |
| 19:31:24 | mordred | yah. that would be more smarter | |
| 19:31:40 | sdague | is there anyone else with a newton cloud? | |
| 19:32:20 | mordred | dunno - I only know vexxhost is because I was chatting with mnaser about a config-drive issue over the weekend | |
| 19:32:48 | mordred | but given the way novaclient works here, it's highly unlikely many real-world folks are broken by this issue, fwiw | |
| 19:34:06 | mordred | I mean - honestly rather than fixing it - just returning a json dict in the body with the image_id in it would be a win - those urls aren't really used much in the rest of the overall api flow | |
| 19:34:24 | mordred | and the first thing you're going to do with this as a user is make a call to glance to get the image dict by id | |
| 19:34:43 | mordred | (since you have to poll that for status to know when it's done) | |
| 19:35:00 | mriedem | mordred: yeah that's why i said i was surprised, | |
| 19:35:10 | mriedem | because we just return ids for things like this in response bodies | |
| 19:35:15 | mordred | so it likely _could_ be fixed with a microversion easier than actually fixing the internal/public glance url thing | |
| 19:35:18 | mriedem | but, changing the response will be a microversion | |
| 19:35:22 | mordred | mriedem: yah. I was very surprised myself | |
| 19:35:46 | mordred | mriedem: as you might can tell from my comment: https://review.openstack.org/#/c/452566/2/shade/openstackcloud.py | |
| 19:36:25 | mriedem | ha | |
| 19:36:32 | mriedem | so dramatic | |
| 19:36:54 | mordred | well - it was a sunday and I managed to break a server by making the api call against nova | |
| 19:37:03 | mordred | because it _ALSO_ apparently turns the server off to do the snapshot | |
| 19:37:15 | mordred | so I might have been in a bad mood at the time | |
| 19:37:29 | mriedem | live snapshot is a thing if libvirt is new enough and you've configured nova-compute to attempt it | |
| 19:37:40 | mriedem | we disabled live snapshot way back when b/c it resulted in 25% failure rate in the gate | |
| 19:37:59 | mriedem | you as the api user have no idea if it's live or cold though | |
| 19:38:04 | mriedem | since it's config drive on the compute | |
| 19:38:08 | mriedem | *driven | |
| 19:38:40 | mordred | \o/ | |
| 19:38:52 | mriedem | were you surprised that it was a cold snapshot? i see we don't mention that in the api ref | |
| 19:39:30 | mordred | well - I shouldn't have been - I know we don't use snapshots for any purpose in infra because it used to make the servers unresponsive for potentially hours - but I think I had just assumed that was some Xen/Rackspace thing | |
| 19:39:44 | mordred | but yes - I was totally unprepared for it to be a COMPLETELY unsafe thing to run | |
| 19:40:08 | mordred | that left me with a VM in a down state for an unknowable period of time | |
| 19:40:28 | mordred | and with no way that I could find for me to say "oh, crap, dude, nevermind - I need the VM back please" | |
| 19:40:58 | mriedem | build a confirm prompt into shade :) | |
| 19:41:26 | mriedem | we could note this in the docs though, that by default it's a cold snapshot so you should expect the guest to be offline during the snapshot operation | |
| 19:41:36 | mriedem | i'm not really sure if that's true for the other virt drivers, but it is for libvirt | |
| 19:42:03 | mriedem | sdague: didn't you have a patch to undo disable_libvirt_livesnapshot? | |
| 19:42:06 | mriedem | or change it to False | |
| 19:42:10 | mriedem | maybe it was sfinucan | |
| 19:42:13 | mordred | mriedem: yah - I think a docs patch with a warning would be a good idea | |
| 19:42:14 | openstackgerrit | Sihan Wang proposed openstack/nova master: VMware:fix get VM root device conditional verify https://review.openstack.org/402256 | |
| 19:42:32 | mriedem | mordred: we can also doc the image location header response as probably being bogus for most users, at least users of public clouds | |
| 19:42:36 | mordred | ++ | |
| 19:43:44 | sdague | I did change it to false, I thought we reverted it though | |
| 19:44:33 | sdague | oh, I guess it's still off by default | |
| 19:44:47 | sdague | we should probably change that | |
| 19:46:45 | mordred | mriedem: https://bugs.launchpad.net/nova/+bug/1679285 | |
| 19:46:45 | openstack | Launchpad bug 1679285 in OpenStack Compute (nova) "Create Server Snapshot returns incorrect Image Location" [Undecided,New] | |
| 19:47:00 | mriedem | thanks | |
| 19:51:00 | mriedem | so how to fix this, i'm not sure if we just do the service catalog thing for this right now, or change the response in a microversion (which we can't backport) | |
| 19:51:08 | mriedem | it's super latent so i'm not sure we need to backport a fix, | |
| 19:51:11 | mriedem | we could just doc it as always sucking | |
| 19:53:45 | mriedem | sdague: ^ thoughts on which way you'd lean? | |
| 19:54:14 | mriedem | i'm not as clear on the service catalog stuff but could probably figure it out by looking at similar things with placement | |
| 19:54:34 | sdague | well, it makes it clear that once we get to newton, we probably break people by accident if they used that field | |
| 19:54:57 | mriedem | because it doesn't have v2 in the url? | |
| 19:55:02 | mordred | mriedem: for my money, I'd document it, and then add the microversion for a new response type ... I think the catalog thing is a _much_ longer fix, even though it's the right one long term | |
| 19:55:02 | mriedem | it doesn't have v1 in it either | |
| 19:55:04 | sdague | the Location: field really is supposed to have the url | |
| 19:55:13 | mriedem | unless images is an alias for v1/images | |
| 19:55:24 | mordred | yah - I think there are too many variables for Location to be correct consistently | |
| 19:55:34 | mordred | because you might be an admin who _does_ want to use the internal url | |
| 19:55:44 | mordred | and you might have requested internal urls from keystone as a user | |
| 19:55:46 | sdague | Location is HTTP spec, if we are going to use it, it has to be a real url | |
| 19:56:06 | mordred | yah. I'm saying ignore the location header and just return the image id in a json doc | |
| 19:56:24 | mordred | that's consistent and can be consumed well | |
| 19:56:25 | mriedem | right it's a question of change the response, or fix the location header and use the catalog | |
| 19:56:32 | mordred | oh - I was suggesting neither ... | |
| 19:56:35 | mriedem | i can do the former easily | |
| 19:56:51 | mordred | just _Add_ a json return dict and document that the location header is likely broken | |
| 19:56:52 | mriedem | mordred: you're saying leave the busted location header? | |
| 19:56:57 | mriedem | oh | |
| 19:57:07 | mriedem | well it's still a microversion to return a body now | |
| 19:57:11 | mordred | yah | |
| 19:57:23 | mordred | but microversions are there for a reason, right? | |
| 19:57:51 | mriedem | yeah, i just don't know if it's better to do the quick fix with a microversion now, or invest in doing the catalog lookup, which is more obscure for me personally | |
| 19:58:04 | mriedem | ksa is just a big black box and i always ask sean what to do :) | |
| 19:58:19 | mriedem | or copy his code | |
| 19:58:21 | mordred | well - I have a patch coming to maybe make it friendlier | |
| 19:59:07 | sdague | mriedem: yeh, honestly, I would say a better response body is probably the right answer | |
| 19:59:13 | sdague | and drop the Location header when we do it | |
| 19:59:20 | mriedem | sdague: i can totally do that, today | |
| 19:59:38 | mordred | ++ - I still think fixing glance interactoins to use catalog long term is the correct and healthy thing to do - but it's not going to be quick | |
| 19:59:40 | sdague | the Location header is clever, and when it was an internal location for nova, maybe really sensible | |
| 19:59:45 | sdague | mordred: yeh | |
| 19:59:46 | mordred | agree | |
| 19:59:58 | mordred | but the world it has changed since then | |
| 20:00:03 | sdague | I think the only sucky part is that the fix isn't really backportable | |
| 20:00:14 | sdague | this is basically an edge case of the v1 -> v2 bit that we missed | |
| 20:00:33 | mordred | yah - but it's also luckily not TERRIBLE to workaround as an API consumer | |
| 20:00:35 | mriedem | does /images alias to /v1/images though? | |
| 20:00:48 | mordred | I mean id = "/".rsplit(location, 1) | |
| 20:00:49 | sdague | mriedem: honestly, I don't know | |