| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-26 | |||
| 18:32:29 | mriedem | but i also need to eat | |
| 18:32:34 | artom | You and everyone else | |
| 18:32:40 | mriedem | dansmith: comments on the numa live migration spec which maybe you can answer, | |
| 18:32:43 | mriedem | re: move claims | |
| 18:32:57 | artom | I think if Nikola came back today, he'd still know more than all of us combined | |
| 18:33:15 | mriedem | on that very hairy part of the code? i agree. | |
| 18:33:28 | mriedem | there are also TODOs in there from him about the move claim stuff for reize | |
| 18:33:29 | mriedem | *resize | |
| 18:34:04 | mriedem | https://github.com/openstack/nova/blob/594c653dc1a312d0364ad24c703e1a9b228133e1/nova/compute/manager.py#L3988 | |
| 18:34:41 | mriedem | anyway, turkey leftovers | |
| 18:36:05 | sean-k-mooney | mriedem: when you are back maybe you could weigh in on https://review.openstack.org/#/c/605116/6/specs/stein/approved/libvirt-neutron-sriov-livemigration.rst@111 also. | |
| 20:59:01 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove ironic/pike note from *_allocation_ratio help https://review.openstack.org/620154 | |
| 21:15:34 | mnaser | so while answering an ML post about all_tenants and friends, i found this TODO since 2015 -- https://github.com/openstack/nova/commit/be41910ac6be28060d9007778fb33766077de59b | |
| 21:15:51 | mnaser | do we just drop that part of the code at that point? given it's been uncommented for years now | |
| 21:19:40 | artom | mriedem, does the superconductor really rescheduler if the live migrations fails? I'm looking but I can't find anything in _execute, and in the conductor manager if there's a failure in _live_migrate it just sets an error. | |
| 21:20:00 | artom | Not sure it's super relevant to the spec, but for my own personal edification | |
| 21:21:39 | mriedem | mnaser: you mean drop it at this point? | |
| 21:22:19 | mnaser | mriedem: i think so? i mean it's just dead code for 5 years, do we want to muck around with microversion bumps and blah | |
| 21:23:25 | mriedem | mnaser: tbc, you're saying just ditch the commented out code since no one cares enough to change it with a new microversion | |
| 21:23:39 | mnaser | yep | |
| 21:24:29 | mriedem | shrug | |
| 21:24:33 | mriedem | i don't see anyone caring enough to change it | |
| 21:24:54 | mriedem | the fact you have to supply the all_tenants parameter to filter on project_id does always confuse me | |
| 21:25:00 | mnaser | well | |
| 21:25:03 | mnaser | the clients workaround it now.. | |
| 21:25:03 | mriedem | but at least it's documented in the api-ref | |
| 21:25:13 | mnaser | pretty sure you dont need to do that anymore with the cli | |
| 21:25:40 | mriedem | https://docs.openstack.org/python-novaclient/latest/cli/nova.html#nova-list | |
| 21:25:48 | mriedem | nova list --tenant will just implicitly add --all-tenants | |
| 21:25:59 | mriedem | if that's what you mean by workaround | |
| 21:26:20 | mnaser | yeah | |
| 21:26:41 | mnaser | https://github.com/openstack/python-openstackclient/blob/master/openstackclient/compute/v2/server.py#L1147-L1153 | |
| 21:26:44 | mnaser | same for osc | |
| 21:27:20 | mriedem | if you want to push a patch to remove the cruft, fine by me | |
| 21:27:23 | mriedem | i might even +2 that | |
| 21:27:52 | mriedem | anything to make that method smaller b/c god is it long | |
| 21:35:23 | mriedem | bnemec: have you ever heard of requests for something like a PostitiveIntOpt or PositiveFloatOpt in oslo.config? we have some options which can be set to 0.0 as the min, but really shouldn't be <= 0. | |
| 21:35:32 | mriedem | but we can't really describe that with just min | |
| 21:37:27 | dansmith | choices=range(1,1000) ? :P | |
| 21:37:36 | bnemec | mriedem: So an opt where min is a < comparison instead of a <=? | |
| 21:38:21 | mriedem | something like that | |
| 21:38:28 | mriedem | for context https://review.openstack.org/#/c/602804/9/nova/conf/compute.py | |
| 21:38:36 | mriedem | initial_cpu_allocation_ratio should never be 0.0 | |
| 21:38:39 | dansmith | we really just want a validation function parameter, right? | |
| 21:38:43 | mriedem | yeah | |
| 21:38:44 | dansmith | we wanted that for something else recently | |
| 21:38:53 | dansmith | validator=lambda str: ... | |
| 21:38:53 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Drop cruft code for all_tenants behaviour https://review.openstack.org/620165 | |
| 21:41:19 | bnemec | A validator callback seems like something we could do. | |
| 21:41:32 | bnemec | Alternatively, in this case min=0.000001 is probably also sane. | |
| 21:46:16 | bnemec | You could also create a custom type that did the validation in the constructor. | |
| 21:46:44 | bnemec | Subclass Float and put whatever logic you need in there: https://github.com/openstack/oslo.config/blob/master/oslo_config/types.py#L409 | |
| 21:47:14 | mriedem | I wasn't sure how kosher subclassing oslo.config opt types was | |
| 21:47:16 | bnemec | Then create the opt as Opt(type=MyCustomType, ...). | |
| 21:47:55 | bnemec | They're part of the public API so I'd say they're fair game. | |
| 21:49:20 | mriedem | ok yeah that's probably cleanest | |
| 21:49:25 | bnemec | They danger might be creating a completely new class as a type, which could theoretically be done, but if we ever added to the type API you might get broken. | |
| 21:49:32 | bnemec | As long as you inherit from an existing type you should be okay though. | |
| 21:49:57 | bnemec | They all descend from a single ABC. | |
| 21:50:53 | mriedem | the adam of config opts? | |
| 21:51:14 | bnemec | Indeed. | |
| 21:51:30 | bnemec | Also, I think I was wrong. You want to do validation in __call__. | |
| 21:51:40 | bnemec | That's where we're doing it in the existing types: https://github.com/openstack/oslo.config/blob/master/oslo_config/types.py#L830 | |
| 21:52:19 | mriedem | ah yeah https://github.com/openstack/oslo.config/blob/master/oslo_config/types.py#L305 | |
| 21:52:51 | bnemec | Yeah, better example. :-) | |
| 22:03:19 | mriedem | welp https://review.openstack.org/#/c/613126/ kind of blows up https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/initial-allocation-ratios.html#manually-set-placement-allocation-ratios-are-overwritten | |
| 22:03:23 | mriedem | how am i not surprised | |
| 22:05:21 | mriedem | efried: you'll have some context on ^ | |
| 22:06:52 | mriedem | i'm not exactly sure what we should do about it, outside of passing an initial flag to update_provider_tree or something gross like that...although maybe upt can check the provider tree to see if it already has inventory with allocation_ratio set, and if so, don't provide a value unless CONF.*_allocation_ratio is not None | |
| 22:09:27 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Give drop_move_claim() correct docstring https://review.openstack.org/620170 | |
| 22:09:43 | artom | mriedem, ^^ related to the numa live migration spec. I *think* I'm right, and it might clarify the confusion around where we're removing usages. | |
| 22:10:31 | mriedem | artom: totally forgot you pinged me earlier, sec | |
| 22:10:52 | mriedem | artom: superconductor does not reschedule if live migration fails, no | |
| 22:10:59 | mriedem | it reschedules if the pre-checks on the dest/source fail | |
| 22:11:34 | artom | Right, sorry, I wasn't being precise enough. Though I can't find it rescheduling *anywhere* | |
| 22:11:45 | mriedem | so here conductor asks the scheduler for a host https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L370 | |
| 22:12:01 | mriedem | in this try block it does the pre-checks for dest/source https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L385 | |
| 22:12:15 | mriedem | if that fails, blacklist the host https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L391 | |
| 22:12:16 | artom | *facepalm* | |
| 22:12:21 | artom | Got it, sorry. | |
| 22:12:28 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L366 | |
| 22:12:28 | mriedem | set host=None and hit the while again | |
| 22:12:38 | mriedem | np | |
| 22:12:42 | artom | My brain skipped over _find_destination() as an atomic thing | |
| 22:13:32 | artom | Thanks for taking the time to walk me through this :) | |
| 22:14:00 | artom | Now that comment patch, if you have time ;) The patch itself is trivial, but requires thinking to make sure I got it right. | |
| 22:14:07 | openstackgerrit | Merged openstack/nova master: Add debug logs when doubling-up allocations during scheduling https://review.openstack.org/617016 | |
| 22:14:14 | openstackgerrit | Merged openstack/nova stable/rocky: Default embedded instance.flavor.is_public attribute https://review.openstack.org/619349 | |
| 22:16:45 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Give drop_move_claim() correct docstring https://review.openstack.org/620170 | |
| 22:31:14 | mriedem | artom: ok some random comments inline | |
| 22:31:40 | mriedem | i guess drop_move_claim called from the source on confirm_resize always confuses me, | |
| 22:31:47 | mriedem | because the claim is made on the dest during prep_resize | |
| 22:31:52 | efried | mriedem: Sorry, I'm back now. We're trying to figure out how to know whether allocation ratios were defaulted by placement or conf or nova? | |
| 22:32:57 | mriedem | efried: yeah - need to determine when initial_*_allocation_ratio config should be used (first time the compute was created) and report that to placement, or if we should set allocation ratios because CONF.*_allocation_ratio is some non-default value, or if we should leave it alone because the admin set some allocation_ratio via the placement API directly | |
| 22:33:24 | mriedem | i think we can know the initial case from upt if the provider tree doesn't have those inventory resource class keys in it | |
| 22:33:44 | mriedem | otherwise only ever overwrite if CONF.*_allocation_ratio is not None | |
| 22:34:05 | efried | "if the provider tree doesn't have those inventory resource class keys" <== I don't think this can happen within upt | |
| 22:34:30 | mriedem | on initial startup, wont upt just have a root provider with no inventory on it? | |
| 22:34:35 | mriedem | the driver reports the inventory initially | |
| 22:34:41 | efried | I don't think so, because of the (legacy) code that's bootstrapping the CPU/MEMORY_MB/DISK_GB inventories before we get to upt | |