| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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 | |
| 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") | |
| 18:45:35 | jaypipes | cdent: no | |
| 18:45:48 | cdent | straight answer \o/ | |
| 18:45:55 | jaypipes | :) | |
| 18:46:27 | edleafe | cdent: I have an inventory of 0 Rolls Royces | |
| 18:46:35 | cdent | omg, so do I! | |
| 18:47:28 | mriedem | dansmith frequently claims to have an inventory of 0 fucks | |
| 18:48:01 | mriedem | you know placement is getting traction when you can use it in casual conversation | |
| 18:48:34 | mriedem | sdague: i'm +2 on johnthetubaguy's policy docs spec now https://review.openstack.org/#/c/433010/ | |
| 18:48:39 | mriedem | it's pretty straight-forward | |
| 18:49:09 | openstackgerrit | Ed Leafe proposed openstack/nova master: virt: implement get_inventory() for Ironic https://review.openstack.org/441544 | |
| 18:49:10 | openstackgerrit | Ed Leafe proposed openstack/nova master: Refactor placement fixtures https://review.openstack.org/446122 | |
| 18:49:10 | openstackgerrit | Ed Leafe proposed openstack/nova master: WIP - add some functional tests for placement https://review.openstack.org/446123 | |
| 18:49:40 | edleafe | jaypipes: looks like your ironic inventory patch was rebased | |
| 18:50:18 | edleafe | cdent: the last one above ^^ is the attempt at functional tests for placement | |
| 18:50:31 | cdent | looking edleafe | |
| 18:55:02 | cdent | the other method you want, for inventory setting, is _update() | |
| 18:56:45 | cdent | edleafe: of your could kill two birds with update_usage() | |
| 18:59:27 | edleafe | cdent: stepping into a meeting, but will dig into that after | |
| 18:59:41 | cdent | sorry for your luck | |
| 19:00:02 | edleafe | cdent: hey, only 2 today! | |
| 19:00:15 | cdent | great success? | |
| 19:17:48 | jaypipes | edleafe: did you change anything in the patch you just rebased? | |