| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 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? | |
| 19:40:33 | openstackgerrit | Jay Pipes proposed openstack/nova master: virt: implement get_inventory() for Ironic https://review.openstack.org/441544 | |
| 19:40:34 | jaypipes | mriedem: ok, ready for you ^ | |
| 19:43:26 | edleafe | jaypipes: nope. Mine just built on yours | |
| 19:52:52 | mriedem | jaypipes: you missed one little thing https://review.openstack.org/#/c/441544/9 | |
| 20:05:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: virt: implement get_inventory() for Ironic https://review.openstack.org/441544 | |
| 20:06:00 | mriedem | jaypipes: nvm, i got it, +2 | |
| 20:06:23 | mriedem | dansmith: ^ it's all yours now | |
| 20:07:28 | dansmith | mriedem: k in a sec | |
| 20:12:52 | efried | jaypipes Before I try to grok your blueprint and the above change set, does there exist a quick-and-dirty guide entitled "How to convert my compute driver from get_available_resource to get_inventory"? | |
| 20:16:56 | jaypipes | mriedem: thx | |
| 20:17:55 | jaypipes | efried: hi :) no, there's no guide other than looking at the above patch. | |
| 20:18:08 | mriedem | efried: it's probably easiest to just look at that ironic patch | |
| 20:18:11 | mriedem | for get_inventory | |
| 20:18:22 | jaypipes | efried: basically just needs to return a dict of inventory information for each resource class the driver understands. | |
| 20:19:09 | efried | mriedem jaypipes Okay, that's where I was going to go. Guess you guys will be reviewing the change one way or another, so you'll catch me if I eff it up... | |
| 20:19:31 | mriedem | if you eff it up the resource tracker will probably give you a KeyError | |
| 20:19:46 | mriedem | it's all self testing, like most everything in nova, rock solid | |
| 20:19:53 | efried | But of course. | |
| 20:25:11 | mriedem | wtf, why is RequestSpec.forced_hosts a list when you can only provide one value in the API when creating a server? | |
| 20:25:14 | mriedem | bauzas: ^? | |
| 20:25:38 | mriedem | please oh please don't say future proofing | |
| 20:28:48 | bauzas | mriedem: because you can provide a list AFAIK | |
| 20:28:55 | bauzas | lemme verify this | |
| 20:29:28 | melwitt | yeah, from what I find I think the scheduler hint is force_hosts as a list? | |
| 20:29:30 | mriedem | https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/servers.py#L579 is not a list | |
| 20:29:42 | mriedem | it's a scheduler hint? | |
| 20:30:21 | bauzas | okay, can't find a reasonable answer | |
| 20:30:22 | melwitt | I'll wait for bauzas to let us know what's the deal | |
| 20:31:04 | mriedem | it's not a scheduler hint that's checked in any in-tree filters | |
| 20:31:13 | melwitt | it seems like it coincides with the scheduler hint, but why it's baked into RequestSpec, I don't know the history on that | |
| 20:31:26 | bauzas | mriedem: melwitt: no, forced_hosts is in the host manager | |
| 20:31:28 | mriedem | the scheduler hint for groups? | |
| 20:31:40 | melwitt | yeah, forcing a host isn't part of any specific filter. it happens before filtering and will bypass filtering | |
| 20:31:55 | bauzas | yeah that | |
| 20:32:04 | mriedem | ok but there is also this https://docs.openstack.org/admin-guide/cli-nova-specify-host.html | |
| 20:32:32 | bauzas | mriedem: yeah, because it's a terrible UX | |
| 20:32:43 | mriedem | i know it's not part of a filter, but i'm trying to figure out how it becomes a list, | |
| 20:32:46 | bauzas | forced_hosts is set by the AZ flag | |
| 20:33:02 | mriedem | because the only thing i see that sets forced_hosts in the request spec comes through the REST API using the az parsing in ^ | |
| 20:33:02 | bauzas | mriedem: honestly, I don't remember why, lemme verify the change | |
| 20:33:39 | mriedem | those docs are also a bit wrong btw, they say "HOST is an optional parameter. In such cases, use the --availability-zone ZONE::NODE." - but they fail to mention that NODE is also an optional parameter | |
| 20:34:19 | bauzas | node was added later | |
| 20:34:26 | bauzas | because of $ironic :) | |
| 20:34:30 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add check for invalid inventory amounts https://review.openstack.org/428800 | |
| 20:34:31 | openstackgerrit | Ed Leafe proposed openstack/nova master: Add check for invalid allocation amounts https://review.openstack.org/416752 | |
| 20:34:35 | edleafe | jaypipes: mriedem: updated ^^ | |
| 20:35:21 | jaypipes | cool | |
| 20:37:19 | bauzas | mriedem: so, in between PS7 and PS8, I modified force_hosts to be a list | |