| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-14 | |||
| 14:16:02 | dansmith | smcginnis: maybe cdent is the person to do that? | |
| 14:16:25 | cdent | smcginnis: yeah, I can look shortly, in the middle of a meeting, and then got to write a quick test, but then happy to look | |
| 14:16:36 | smcginnis | cdent: Perfect, thanks! | |
| 14:16:37 | dtantsur | dansmith: see my comments on 492964, you may be underestimating how "interesting" our driver is :) | |
| 14:20:37 | cdent | dansmith, dtantsur : as I recall the mismatch between inventory used and real inventory is hard to reconcile at the time of allocations because the allocations want to be based on the flavor and injecting and “oh by the way this is ironic, just consume everything” is complex so easier to change the inventory. (all of which is what led to customer resource class CUSTOM_IRON_SUPERMAN etc) | |
| 14:21:05 | dansmith | cdent: it's not a thing placement needs to handle, | |
| 14:21:14 | dansmith | it's a thing we should arrange for in our reporting | |
| 14:21:32 | cdent | ? then I must have missed a detail | |
| 14:22:17 | dtantsur | dansmith: I also don't get it a bit.. where exactly do you suggest to make the change? | |
| 14:22:40 | dtantsur | dansmith: we can either change the inventory in the ironic driver OR change how nova talks to placement somewhere on an upper level, no? | |
| 14:23:51 | dansmith | dtantsur: I'm replying hang on a sec | |
| 14:23:55 | dtantsur | sure, thanks | |
| 14:26:12 | dansmith | dtantsur: I'm saying nova should be either reporting node size instead of flavor to ironic for the _allocation_ instead of reporting a smaller node while an instance is booted there, | |
| 14:26:29 | dansmith | but you're correct that we don't have a way for the ironic driver to override that at the moment | |
| 14:26:49 | dtantsur | right, this is the problem. I agree that your suggested approach is much cleaner | |
| 14:26:59 | dansmith | I want to talk to jay about this before we proceed and I think we've got some time here | |
| 14:27:43 | dansmith | dtantsur: if people are using the exact filters today, then just continuing to report the size of the node even when an instance is booted there is fine, right? | |
| 14:27:53 | dansmith | because we'll report full inventory and they will consume it all | |
| 14:28:02 | dansmith | only if you have tiny flavors and big nodes would we have a problem | |
| 14:28:09 | dtantsur | dansmith: yes, this is ok | |
| 14:28:19 | dansmith | I feel like we could maybe just reno that and say that moving to RC is the solution which you have to do anyway | |
| 14:28:53 | dtantsur | dansmith: moving to RC also does not work without this patch, because we used to not report RC for deployed nodes | |
| 14:29:05 | dansmith | we need a patch for sure, I get that | |
| 14:29:18 | dtantsur | I can split it into two patches, if you would like: to fix RC and to fix reporting of everything else | |
| 14:29:26 | dansmith | I just want that patch to report consistent inventory regardless | |
| 14:30:06 | dansmith | if you want to split, then the split should be: 1. Keep reporting inventory even if instances are booted there, and 2. report _flavor_ as the inventory if an instance is booted | |
| 14:30:11 | dansmith | #2 is the thing I have a problem with | |
| 14:30:18 | dansmith | #1 I'm fine with | |
| 14:30:20 | dansmith | make sense? | |
| 14:30:58 | dtantsur | dansmith: reporting VCPU from node instead of flavors will break everyone who does not use exact filters (e.g. tripleo) | |
| 14:31:25 | dtantsur | in this case, I'd report only custom resource classes as #1, and leave vcpu/... for #2 | |
| 14:31:41 | dansmith | how does it break non-exact flavor users? | |
| 14:32:01 | dansmith | only if you have flavors so small that you could fit more than one per node right? | |
| 14:32:14 | dtantsur | dansmith: right, which is not an uncommon case in my experience | |
| 14:32:42 | dtantsur | maybe I'm biased by dealing with TripleO, but the common approach there is to use flavor as a declaration of minimum properties | |
| 14:33:10 | dtantsur | https://github.com/openstack/instack-undercloud/blob/master/instack_undercloud/undercloud.py#L1373 | |
| 14:33:26 | dansmith | if we hit that we'll reschedule and pick another, right? | |
| 14:33:45 | dansmith | but that's kinda what I meant about documenting that fact and recommending an immediate move to RC | |
| 14:34:08 | dtantsur | yeah, I'm wondering how unpleased the people are going to be, if we increase their retry count e.g. twice | |
| 14:34:17 | dansmith | anyway, I'm surprised jay isn't here yet, so I expect he will be soon, let's just hold off a bit as he might have some sneaky idea | |
| 14:34:27 | dtantsur | tripleo maximum is IIRC 30, so it's not a hard failure :) | |
| 14:34:42 | dansmith | maximum what? reschedule? | |
| 14:34:48 | dtantsur | yep | |
| 14:34:51 | dansmith | oof :) | |
| 14:35:12 | dtantsur | the double assignment problem was very frequent some time ago (maybe it's still is) | |
| 14:35:25 | dtantsur | and we have a big issue (well, misfeature) in ironicclient: we retry HTTP Conflict | |
| 14:35:29 | dansmith | you know, I'm not sure why we never solved this problem with a "number of instances == 0" filter instead of some of this other craziness | |
| 14:35:55 | dansmith | like, filter that includes nodes with no instances booted on them | |
| 14:36:17 | dtantsur | yeah.. or extend get_inventory to clearly indicate something like "I cannot accept more nodes, because I won't, go away) | |
| 14:36:27 | dtantsur | yeah, I got the idea | |
| 14:36:28 | dansmith | well, that's what RC does basically | |
| 14:36:39 | dtantsur | well, true :) | |
| 14:37:04 | dtantsur | except that an ironic node can be unavailable for other reasons, e.g. it's being cleaned or in a power management fault | |
| 14:37:14 | dtantsur | which is something we have no way of cleanly expressing | |
| 14:37:41 | dansmith | yeah, I still think that's a different thing than "no inventory" that we should express separately | |
| 14:37:44 | dansmith | hey, look, it's jaypipes | |
| 14:37:51 | dtantsur | wowwowwow | |
| 14:38:00 | dtantsur | jaypipes: welcome to the party, be our guest :D | |
| 14:38:12 | dansmith | jaypipes: I wanna discuss an ironic thing with you.. I have a meeting in 20 minutes, but maybe we could hangout after that? | |
| 14:39:44 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Add attachment_complete call to volume/cinder.py https://review.openstack.org/493323 | |
| 14:39:45 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Tweak connection_info translation for the new Cinder attach/detach API https://review.openstack.org/493324 | |
| 14:39:45 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 14:42:11 | dansmith | heh | |
| 14:42:22 | dtantsur | :( | |
| 14:49:32 | cdent | smcginnis: I got run and fetch a car that’s been fixed, but will be back in about 30 mins to look at that grenade thing | |
| 14:50:27 | smcginnis | cdent: Sounds good, thanks. | |
| 15:19:12 | openstackgerrit | Hesam Chobanlou proposed openstack/nova master: add online_data_migrations to nova docs adding cli documentation for online_data_migrations to clarify when the command is complete. https://review.openstack.org/493442 | |
| 15:23:12 | openstackgerrit | Hesam Chobanlou proposed openstack/nova master: add online_data_migrations to nova docs adding cli documentation for online_data_migrations to clarify when the command is complete. https://review.openstack.org/493442 | |
| 15:56:45 | dansmith | dtantsur: https://hangouts.google.com/call/kls2hwod6nhprahlf5mj5i3qveu | |
| 15:56:51 | dansmith | (if interested) | |
| 15:57:00 | dtantsur | gimme a few minutes | |
| 16:00:09 | dansmith | jaypipes: https://review.openstack.org/#/c/492964/5 | |
| 16:01:57 | smcginnis | cdent: We may have figured out the failure, testing now. Devstack config with setting NOVA_USE_MOD_WSGI. | |
| 16:34:02 | cdent | smcginnis: that’s what I was going to look for, so that sounds likely. Took me a lot longer to get back than expected: a nearby music festival was letting out. traffic. very traffic. | |
| 16:34:58 | smcginnis | cdent: I love working from home now and not having to deal with that daily. | |
| 16:35:14 | cdent | yeah, me too, which makes me more sensitive to it when I do... | |
| 16:36:31 | dtantsur | dansmith, jaypipes, this concerns me: https://github.com/openstack/nova/blob/master/nova/virt/ironic/driver.py#L319-L321 | |
| 16:36:54 | dtantsur | it may mean that we'll allow users to get "more resources" by PATCHing node.properties.. | |
| 16:48:04 | dansmith | dtantsur: I don | |
| 16:48:13 | vdrok | dtantsur: hrm, so it seems we'll have vcpus=0 and vcpus_used=something during deployment | |
| 16:48:28 | dansmith | dtantsur: don't really know what the whole PATCH thing is, nor the behavior of node.properties | |
| 16:48:30 | dtantsur | vdrok: this has to be fixed, but that's not the biggest problem | |
| 16:48:47 | dtantsur | dansmith: this is where we take inventory from. tl;dr it can be changed from API at any moment | |
| 16:49:59 | dansmith | dtantsur: by the admin yes? | |
| 16:50:02 | dtantsur | yes | |
| 16:50:21 | dansmith | dtantsur: so ironic discovers it but admin can override? | |
| 16:50:21 | dtantsur | it's not even absolutely crazy: they can power the instance down, and install more RAM in it.. | |
| 16:50:33 | dtantsur | dansmith: the admin sets it initially, but they can change it | |
| 16:51:29 | vdrok | dtantsur: also if we'll be reporting the resources as used if vcpus=0 in get_inventory, does it mean we'll be doing for maintenance'd nodes too? | |
| 16:52:19 | vdrok | ditto for bad power state | |
| 16:52:28 | dtantsur | yes, this is fine | |
| 16:52:38 | dtantsur | we're mostly concerned about reporting wrong inventory for active nodes | |
| 16:54:09 | vdrok | but then, even if it changes, this is all a part of the same periodic task? like, properties changed, _node_resource sees new value, and uses it in get_inventory | |
| 16:54:23 | vdrok | *and it gets used in | |
| 16:55:51 | dtantsur | right, and the Placement sees free resources to schedule on >_< | |
| 16:56:13 | dtantsur | may be not a huge deal, unless the change all of cpus, memory and disk at the same time | |
| 16:56:32 | dansmith | dtantsur: well, it's not a problem at all once RC is in place right? | |
| 16:56:48 | dtantsur | dansmith: as long as all flavors are using it - yes | |
| 16:57:12 | dtantsur | maybe I should not worry about it too much. just document it as a known issue.. | |
| 16:57:38 | dansmith | known issue, which goes away in queens when we require RC for all ironic scheduling | |