| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 17:37:57 | lyarwood | mdbooth: yeah agreed that it's weird, I assume it's a hangover from copying BDMs the ec2 api | |
| 17:38:12 | lyarwood | from the ec2 api even | |
| 17:42:19 | mdbooth | lyarwood: We're absolutely never going to have more than 1 BDM with the same attachment_id, right? Just confirming that makes absolutely no sense. | |
| 17:44:15 | lyarwood | mdbooth: right, it's unique to along with the instance_uuid and volume_id iirc, pretty sure the spec calls that out /me checks | |
| 17:44:51 | mdbooth | lyarwood: I can't think how it would make sense. | |
| 17:45:20 | lyarwood | mdbooth: to have >1 bdms with the same attachment_id? | |
| 17:45:50 | mdbooth | lyarwood: Yes | |
| 17:45:58 | openstackgerrit | Dan Smith proposed openstack/nova master: Get instance availability_zone without hitting the api db https://review.openstack.org/439754 | |
| 17:45:58 | openstackgerrit | Dan Smith proposed openstack/nova master: Set instance.availability_zone whenever we schedule https://review.openstack.org/446053 | |
| 17:45:59 | openstackgerrit | Dan Smith proposed openstack/nova master: Make conductor ask scheduler to limit migrates to same cell https://review.openstack.org/438025 | |
| 17:46:25 | lyarwood | mdbooth: yeah, I can't think of a use case | |
| 17:46:29 | rebase | /que/close | |
| 17:47:16 | lyarwood | mdbooth: from the spec '* Volume attachment is specific to a host uuid, instance uuid, and volume uuid' | |
| 17:48:14 | mdbooth | lyarwood: Cool, so dup attachment_id would definitely be a bug | |
| 17:49:23 | lyarwood | mdbooth: yeah mriedem suggested that already but as its nullable some backends might not allow it iirc | |
| 17:49:29 | lyarwood | mdbooth: but ask away and I can take another look | |
| 17:49:46 | mdbooth | lyarwood: ah, good point | |
| 17:49:58 | mriedem | mysql is fine with it a unique constraint over a nullable column | |
| 17:50:11 | mdbooth | mriedem: Does it allow more than 1 null? | |
| 17:50:14 | mriedem | i think postgres is too, but i think both can be configured to be more strict | |
| 17:50:22 | mriedem | mdbooth: not sure about that, i'd hope not | |
| 17:50:35 | mdbooth | In that case it wouldn't work here, which is unfortunate | |
| 17:50:53 | mriedem | to be clear, i was thinking of db2 when i said the constraint thing, because of muscle memory, but i know we don't support or care about db2 | |
| 17:51:31 | mdbooth | They make debugging easier | |
| 17:52:46 | mdbooth | lyarwood: Incidentally, on the bdm.uuid thing, couldn't we mass populate that with an online migration? | |
| 17:55:19 | mdbooth | lyarwood: Same for attachment_id now that I think of it, if cinder gives us an api to create an attachment for a legacy-attached volume. | |
| 17:56:22 | lyarwood | mdbooth: I think that did come up for attachment_id but I don't think there's anything for us to call atm | |
| 17:57:25 | lyarwood | mdbooth: I can't find it at the moment but I'm sure there's also a comment against doing this for uuids btw | |
| 18:02:50 | mriedem | johnthetubaguy: i've reviewed the policy doc spec https://review.openstack.org/#/c/433010/ | |
| 18:02:56 | mriedem | johnthetubaguy: some niggles inline | |
| 18:03:04 | mriedem | see what i did there, with your word? | |
| 18:03:08 | johnthetubaguy | mriedem: looking | |
| 18:03:17 | johnthetubaguy | mriedem: I totally did :) | |
| 18:03:34 | openstackgerrit | Dan Smith proposed openstack/nova master: Get instance availability_zone without hitting the api db https://review.openstack.org/439754 | |
| 18:03:34 | openstackgerrit | Dan Smith proposed openstack/nova master: Make os-availability-zones know about cells https://review.openstack.org/442163 | |
| 18:03:35 | openstackgerrit | Dan Smith proposed openstack/nova master: Set instance.availability_zone whenever we schedule https://review.openstack.org/446053 | |
| 18:03:35 | openstackgerrit | Dan Smith proposed openstack/nova master: Make conductor ask scheduler to limit migrates to same cell https://review.openstack.org/438025 | |
| 18:09:15 | mdbooth | lyarwood: Next 3 are the meaty ones. I'll start them in the morning. | |
| 18:14:12 | mriedem | cdent: edleafe: has anyone brought up the microversion question in https://review.openstack.org/#/c/433010/ yet? | |
| 18:14:27 | mriedem | or is that restriction already in place in the object code somewhere? | |
| 18:15:14 | mriedem | sorry wrong link | |
| 18:15:18 | cdent | sorry, mriedem, I'm not sure I've got enough context | |
| 18:15:19 | mriedem | https://review.openstack.org/#/c/416752/ | |
| 18:15:31 | cdent | oh that one | |
| 18:15:33 | mriedem | normally changing the request schema validation is a microversion bump | |
| 18:15:40 | mriedem | does the object code already enforce this? | |
| 18:16:19 | cdent | mriedem: this is a case of: if you did it wrong then there would be big failures (of the things won't work at all variety) | |
| 18:16:41 | cdent | so this fix is akin to going from 500 to "actually working" | |
| 18:16:57 | cdent | it just so happens that existing client code doesn't tickle these problems | |
| 18:17:24 | edleafe | yeah, I was thinking it's more like a bug fix rather than a straight behavior change | |
| 18:18:09 | mriedem | i'm legally obligated to ask about microversions you know | |
| 18:18:19 | mriedem | certified by the great state of minnesota | |
| 18:18:37 | cdent | mriedem: everyone appreciates your assiduousness | |
| 18:19:12 | edleafe | even non-Minnesotans | |
| 18:22:08 | mriedem | next question: do we plan on backporting this fix to ocata? | |
| 18:23:38 | cdent | mriedem: I'd say it is a nice to have, probably even a good to have, but not a must have? | |
| 18:23:58 | cdent | without these changes it is possible for a careless admin to cause trouble | |
| 18:24:15 | cdent | but they have try hard | |
| 18:24:30 | mriedem | if we want to backport we should tie these to a bug | |
| 18:25:30 | cdent | weird, I thought it already was, but apparently not | |
| 18:25:49 | cdent | edleafe: I'll make a bug and you can update? | |
| 18:26:19 | edleafe | cdent: sure thing | |
| 18:29:33 | openstackgerrit | John Garbutt proposed openstack/nova-specs master: Add policy-docs spec https://review.openstack.org/433010 | |
| 18:30:24 | mriedem | cdent: edleafe: i'm also not sure about the default changes in here https://review.openstack.org/#/c/428800/ | |
| 18:30:36 | mriedem | that gets a bit weird with ironic custom resource classes that claim 1 for everything | |
| 18:30:42 | mriedem | has jaypipes taken a look at that? | |
| 18:30:47 | johnthetubaguy | mriedem: added a note about the new oslo thing, and a hacking check to ensure we use the new method that enforces the description is filled | |
| 18:30:57 | mriedem | johnthetubaguy: ok, looking | |
| 18:31:24 | jaypipes | mriedem: not yet | |
| 18:31:32 | cdent | mriedem: those only come into play if the inventory doesn't specify | |
| 18:31:48 | mriedem | cdent: right, and min_unit/max_unit aren't required, | |
| 18:31:51 | cdent | and in their old form it was possible to say a max_unit of zero, which makes no logical sense | |
| 18:31:55 | mriedem | so a lazy client could just not provide them | |
| 18:32:00 | mriedem | yeah | |
| 18:32:06 | cdent | so max int is correct | |
| 18:32:23 | mriedem | i know i'm overthinking this | |
| 18:32:31 | cdent | (even though you probably wouldn't want it to be set that) | |
| 18:32:46 | edleafe | the old default of max_unit=0 was definitely wrong | |
| 18:33:08 | mriedem | so if we shouldn't be guessing, why is max_unit optional? | |
| 18:33:36 | mriedem | oh well, | |
| 18:33:44 | edleafe | mriedem: they should be able to request as much inventory as is available, unless it's been configured to be restricted | |
| 18:33:49 | mriedem | i guess if total is maxint, and stepsize is 1, then by default you can have one unit of maxint | |
| 18:34:17 | cdent | I'm trying to make sure the bug report covers everything, including some cases not currently covered in ed's changes | |
| 18:35:22 | melwitt | johnthetubaguy: while you're still around, just a reminder that the quota counting patch is ready for review at your convenience. I think I addressed all of your comments. | |
| 18:35:59 | jaypipes | mriedem: I'm good with that patch. | |
| 18:36:17 | jaypipes | mriedem: though the total should have a minimum, as johnthetubaguy mentioned. | |
| 18:36:50 | mriedem | what happens if i don't specify max_unit, and total=1024, then i request an allocation of 2048? | |
| 18:36:54 | cdent | edleafe, jaypipes, mriedem : please review this bug, I _think_ I covered everything (including the thing jay just said): https://bugs.launchpad.net/nova/+bug/1673227 | |
| 18:36:54 | openstack | Launchpad bug 1673227 in OpenStack Compute (nova) "placement apis for inventory and allocations use insufficiently robust jsonschema" [Undecided,Triaged] | |
| 18:36:54 | mriedem | where does that fail? | |
| 18:37:12 | cdent | mriedem: that will fail at the db layer | |
| 18:37:15 | mriedem | max_unit will default to maxint, total will be 1024 and i'll request 2048 | |
| 18:37:32 | jaypipes | cdent: ++ | |
| 18:38:07 | cdent | since 2048 > (total - use * allocation.ratio) FAIL | |
| 18:38:18 | cdent | YPMV | |
| 18:38:54 | mriedem | i think also here right? https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L1043 | |
| 18:39:23 | cdent | mriedem: that's what I'm calling the db layer | |
| 18:39:42 | cdent | mriedem: but that specific line doesn't involve the total | |
| 18:39:51 | cdent | you want 1062 for that | |
| 18:40:14 | mriedem | and that's handled as a 409 in the PUT /allocations handler | |