| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-15 | |||
| 14:11:25 | bauzas | anyway, we're diverting a lot now from real work | |
| 14:11:39 | bauzas | it's more an hallways discussion or a PTG one if you prefer | |
| 14:11:52 | bauzas | but I'm very sad to not have time to help more Nova | |
| 14:12:07 | bauzas | like for example, doing upstream bug triage | |
| 14:28:22 | mriedem | dansmith: i'm +1 on the down cell series up through the api change https://review.openstack.org/#/c/591657/ just waiting on zuul results if you want to step through those today | |
| 14:29:09 | belmoreira | mriedem: dansmith: does it make sense? https://bugs.launchpad.net/nova/+bug/1816034 or I'm missing something? | |
| 14:29:10 | openstack | Launchpad bug 1816034 in OpenStack Compute (nova) "Ironic flavor migration and default resource classes" [Undecided,New] | |
| 14:30:26 | dansmith | mriedem: cool, I meant to get to them yesterday and got distracted | |
| 14:31:15 | dansmith | belmoreira: mriedem probably remembers better than I, but there was something recently about this | |
| 14:38:38 | mriedem | belmoreira: and you have this enabled yes? https://review.openstack.org/#/c/609043/ | |
| 14:38:49 | mriedem | which is probably why the ironic driver isn't reporting standard resource class inventory | |
| 14:44:41 | mriedem | i wonder if we should just set IronicDriver.requires_allocation_refresh = not CONF.workarounds.report_ironic_standard_resource_class_inventory | |
| 14:45:07 | mriedem | because if we're not going to report the standard inventory we shouldn't try to put allocations for those standard inventory resource classes | |
| 14:46:19 | openstackgerrit | Merged openstack/nova master: Libvirt: do not set MAC when unplugging macvtap VF https://review.openstack.org/624842 | |
| 14:46:30 | belmoreira | mriedem: is set to False | |
| 14:47:05 | mriedem | yeah so i think simply setting IronicDriver.requires_allocation_refresh = not CONF.workarounds.report_ironic_standard_resource_class_inventory would fix the issue | |
| 14:47:26 | mriedem | err, | |
| 14:47:28 | mriedem | nix the 'not' | |
| 14:48:42 | mriedem | that's probably easier than mucking with the _pike_flavor_migration code in stable branches since that code is all gone in stein | |
| 14:49:45 | belmoreira | mriedem: not sure if I'm following. What you are suggesting is to disable IronicDriver.requires_allocation_refresh | |
| 14:50:06 | mriedem | correct, which is what you said you did as a workaround in the bug | |
| 14:52:01 | mriedem | unless that will regress the fix for bug 1724589 somehow | |
| 14:52:02 | openstack | bug 1724589 in OpenStack Compute (nova) pike "Unable to transition to Ironic Node Resource Classes in Pike" [High,Fix committed] https://launchpad.net/bugs/1724589 - Assigned to Matt Riedemann (mriedem) | |
| 14:54:09 | belmoreira | mriedem: we are fixing this in our instances_extra because we will need to recreate the allocations/RP. To make sure RP will not not change uuid in a sharded nova-compute setup with ironic | |
| 14:54:31 | belmoreira | otherwise allocations will not be recreated | |
| 14:54:46 | mriedem | you mean for old instances | |
| 14:54:53 | mriedem | otherwise the scheduler would create the allocations | |
| 14:55:10 | belmoreira | yes, for old instances | |
| 14:57:00 | belmoreira | because I would like to have https://review.openstack.org/#/c/571535/ | |
| 14:58:16 | mriedem | and it looks like http://git.openstack.org/cgit/openstack/nova/tree/nova/scheduler/client/report.py#n136 will do the right thing and filter out any allocations from the flavor that are 0 | |
| 14:58:40 | mriedem | so requires_allocation_refresh can remain true and only report the custom node resource class allocation | |
| 14:58:56 | mriedem | and the driver will only report custom resource class inventory since CONF.workarounds.report_ironic_standard_resource_class_inventory=False | |
| 14:59:22 | belmoreira | yes | |
| 14:59:38 | mriedem | so i guess this whole thing was missed when the code was dropped | |
| 14:59:39 | mriedem | https://github.com/openstack/nova/blob/stable/rocky/nova/virt/ironic/driver.py#L554 | |
| 14:59:45 | mriedem | "This code can be removed in Queens, and will need to be updated to also alter extra_specs to zero-out the old-style standard resource classes of VCPU, MEMORY_MB, and DISK_GB." | |
| 14:59:52 | mriedem | heh, "remove this code, but also change it" | |
| 15:00:34 | belmoreira | I guess existing ironic deployments will hit this when they upgrade to rocky. The impact depends in their expectations and config | |
| 15:01:20 | mriedem | yeah so CONF.workarounds.report_ironic_standard_resource_class_inventory=False actually causes a problem | |
| 15:01:26 | mriedem | fixes one issue, exposes another | |
| 15:03:24 | mriedem | dansmith: jaypipes: jroll: edleafe: do you remember why we didn't just zero out the standard resource classes on embedded instance.flavor for ironic instances in the first place? | |
| 15:03:29 | mriedem | i.e. https://github.com/openstack/nova/blob/stable/rocky/nova/virt/ironic/driver.py#L554 | |
| 15:03:56 | edleafe | mriedem: IIRC, it was for display purposes | |
| 15:04:14 | edleafe | the flavor would show what the user would get | |
| 15:04:39 | mriedem | but can't we still have that *and* override by setting resources:VCPU=0? | |
| 15:05:45 | mriedem | https://docs.openstack.org/nova/latest/user/flavors.html | |
| 15:05:49 | mriedem | "Custom resource classes and standard resource classes to override" | |
| 15:05:56 | mriedem | yeah | |
| 15:05:57 | mriedem | resources:CUSTOM_BAREMETAL_SMALL=1 resources:VCPU=0 | |
| 15:06:03 | edleafe | That was how it was supposed to be done. Keep the values in the flavor, and zero out in extra_specs | |
| 15:06:40 | mriedem | ok i don't know why that standard resource class 0 override wasn't done at the same time as the embedded instance.flavor migration once the node was reporting a custom resource class | |
| 15:07:16 | mriedem | plus that's what the ironic docs tell operators to do https://docs.openstack.org/ironic/latest/install/configure-nova-flavors | |
| 15:07:41 | edleafe | mriedem: https://docs.openstack.org/ironic/latest/install/configure-nova-flavors describes it pretty well | |
| 15:07:55 | edleafe | jinx | |
| 15:08:45 | dansmith | mriedem: yeah, for display and to avoid triggering any of our other magic behaviors with a value is zero, like root size | |
| 15:09:21 | mriedem | sure but we can override via the extra spec | |
| 15:09:24 | mriedem | that seems to be the missing piece | |
| 15:09:43 | mriedem | now whether or not the code that's reporting allocations actually looks at that override... | |
| 15:09:50 | dansmith | right, the point being.. leave the real values for display, override what we actually ask for in the resource overrides | |
| 15:11:23 | mriedem | ok i think this is where the override happens https://github.com/openstack/nova/blob/stable/rocky/nova/scheduler/utils.py#L366 | |
| 15:22:06 | mriedem | belmoreira: ok i'm working on a patch which you can take a look at - knowing this isn't all totally fubar will require a functional test though given how tightly coupled all of this is between the virt driver, resource tracker and scheduler | |
| 15:37:07 | belmoreira | mriedem: thanks | |
| 15:38:03 | mriedem | the commit message on this is going to be fun | |
| 15:38:37 | mriedem | "first belmiro lost a finger here, and then he blew off a toe here" | |
| 15:39:52 | belmoreira | as you may noticed I upgraded nova-compute for ironic only today :) as been an interesting day | |
| 15:40:50 | belmoreira | the next issue is: https://bugs.launchpad.net/nova/+bug/1816086 | |
| 15:40:51 | openstack | Launchpad bug 1816086 in OpenStack Compute (nova) "Resource Tracker performance with Ironic driver" [Undecided,New] | |
| 15:41:01 | fried_rice | uh oh | |
| 15:41:50 | fried_rice | I thought I fixed that. belmoreira are you using latest master? | |
| 15:42:09 | mriedem | he's upgrading ironic queens to rocky i assume | |
| 15:42:11 | mriedem | for compute services | |
| 15:42:22 | mriedem | so they would have had to backport your patches | |
| 15:42:22 | fried_rice | oh, that'd do it | |
| 15:42:25 | fried_rice | yeah | |
| 15:42:55 | belmoreira | I'm using rocky (back ported the patches that we discussed in the past) | |
| 15:43:23 | melwitt | ||
| 15:45:13 | belmoreira | fried_rice: so what I'm saying in the bug doesn't make sense anymore? | |
| 15:45:32 | fried_rice | belmoreira: Backported which patches, though? | |
| 15:45:51 | belmoreira | we couldn't cherrypick the patches in a clean way, we may have missed something | |
| 15:45:53 | fried_rice | belmoreira: Latest master has changes that made sure to only update the ironic node being touched. | |
| 15:46:05 | fried_rice | let me find the patch that did that... | |
| 15:47:20 | fried_rice | belmoreira: https://review.openstack.org/#/c/615677/ | |
| 15:49:01 | fried_rice | belmoreira: I think if you picked that up prior to about PS17, you would have the ironic explosion behavior you're seeing. But after that it *should* be fixed. | |
| 15:49:06 | jaypipes | gibi: hmm, couldn't my algorithm work if we just sorted the request groups in descending order of amount of resources being requested in the group? | |
| 15:49:24 | jaypipes | gibi: then the "greedy" approach would work fine, no? | |
| 15:49:40 | fried_rice | belmoreira: and I know you guys were picking up that series at various points along the way | |
| 15:49:44 | gibi | jaypipes: if only one resource class would be used then yes, but if you have two resource classes requested then which one would sorted first? | |
| 15:50:23 | fried_rice | jaypipes: I didn't feel strongly enough to say so in the review, but fwiw I liked gibi's algorithm as is. | |
| 15:50:24 | jaypipes | gibi: do we have tests that show the behaviour or your algorithm is correct for such situations? | |
| 15:50:24 | belmoreira | fried_rice: thanks, I'm checking | |
| 15:50:35 | tssurya | fried_rice: porting was done after that patch got merged | |
| 15:50:41 | jaypipes | fried_rice: it's difficult to read and understand, IMHO. | |
| 15:50:44 | fried_rice | tssurya: roger that, thanks. | |
| 15:51:03 | fried_rice | jaypipes: I don't disagree, but I'm not sure yours is more readable/understandable. (It is to you because you formulated it.) | |
| 15:51:15 | jaypipes | fried_rice: as is most things around multiple use_single_provider request groups, FWIW | |
| 15:51:22 | gibi | jaypipes: it worth to step through https://review.openstack.org/#/c/616239/26/nova/tests/unit/objects/test_request_spec.py@1305 | |
| 15:52:05 | fried_rice | tssurya: How far up the series did you port? | |
| 15:52:41 | gibi | jaypipes: there group1 could be fit first to 3 RPs but finally only the RP #3 leads to solution | |
| 15:54:18 | jaypipes | fried_rice, gibi: this all just leaves an odd taste in my mouth. | |
| 15:54:44 | jaypipes | fried_rice, gibi: a taste that says "oh, we're totally leaking the placement selection details out of the placement API" | |