| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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: Make os-availability-zones know about cells https://review.openstack.org/442163 | |
| 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:35 | openstackgerrit | Dan Smith proposed openstack/nova master: Make conductor ask scheduler to limit migrates to same cell https://review.openstack.org/438025 | |
| 18:03:35 | openstackgerrit | Dan Smith proposed openstack/nova master: Set instance.availability_zone whenever we schedule https://review.openstack.org/446053 | |
| 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 | mriedem | where does that fail? | |
| 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 | 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: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 | |
| 18:40:41 | mriedem | ok | |
| 18:41:18 | edleafe | cdent: the bug lgtm | |
| 18:41:34 | johnthetubaguy | melwitt: ah, thats going to be tomorrow, but I should make sure I hit that again | |
| 18:41:35 | mriedem | heh, "insufficiently robust" | |
| 18:41:38 | cdent | edleafe: I think it add at least one, and maybe two things for your code | |
| 18:42:07 | edleafe | cdent: yeah, I'll look into that as soon as I can push the functional stuff | |
| 18:42:15 | cdent | ✔ | |
| 18:42:31 | melwitt | johnthetubaguy: it's not urgent, just wanted to remind you about it. thanks :) | |
| 18:42:48 | mriedem | cdent: in the bug report, "* reserved and total need a minimum, probably of 0 and 1 respectively" | |
| 18:42:53 | mriedem | reserved should not be a min of 1 | |
| 18:43:12 | mriedem | oh wait, i see | |
| 18:43:15 | mriedem | "respectively" | |
| 18:43:57 | cdent | I included that "probably" in there because I wasn't sure if there was a use case for total having a min of 0 | |
| 18:44:21 | cdent | I think, however, we decided that anything which is inventory total 0 is inventory that should not be reported | |
| 18:44:22 | mriedem | total nothingness | |
| 18:44:39 | mriedem | i think once you hit 0 you actually get to nirvana | |
| 18:45:21 | cdent | however, now that I think about it: jaypipes: should it be possible to set an existing inventory to 0 capacity for some reason? ("this thing is here but not really") | |