Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-02-22
17:12:00 cdent I'm confused by this notion of user removes compute-owned capability trait. I'm guessing that "don't do that" is considered insufficient?
17:12:35 fried_rice cdent: yeah :)
17:12:37 mriedem users most often probably don't know what they should and shouldn't do
17:12:52 aspiers cdent: it's due to this optimisation https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1047
17:13:19 cdent i guess I was hoping for "most users probably don't know how to delete a trait"
17:13:31 aspiers cdent: see also https://pasteboard.co/I1Tbzp2.png (which is linked from the commit message)
17:13:31 mriedem cdent: if any code is going to mess with your externally mucked allocation stuff, it's like this https://github.com/openstack/nova/blob/27617ee1931b3240dbd0ad4c7d8ffd64cc202bc9/nova/compute/resource_tracker.py#L1241
17:14:05 mriedem cdent: which would get you an INFO message https://github.com/openstack/nova/blob/27617ee1931b3240dbd0ad4c7d8ffd64cc202bc9/nova/compute/resource_tracker.py#L1293
17:14:11 cdent mriedem: yeah, was just looking at that method
17:14:47 cdent aspiers: yeah, that's why I was trying to suggest that the os-traits should grow bigger, faster
17:14:58 fried_rice mriedem: we still haven't solved the resize-to-same-host doubling though, right?
17:15:15 aspiers cdent: makes sense
17:15:55 fried_rice mriedem: which is what your last paragraph is talking about
17:16:11 fried_rice cdent: It doesn't matter if they're in os-traits or not.
17:16:58 cdent they can't be deleted if they are in os-traits
17:17:13 cdent (that was all I was getting at, nothing more)
17:17:16 fried_rice not deleted from placement; deleted from the compute node.
17:17:17 aspiers I think they can be deleted from a resource provider
17:17:21 fried_rice yeah
17:17:42 fried_rice aspiers worded it better, "delete from placement" is too vague.
17:17:44 aspiers just not from global traits list
17:17:56 cdent oh, from the rp, I see
17:18:03 cdent yeah, just don't do that :)
17:18:05 aspiers fried_rice: the ambiguity only just became clear in my head ;-/
17:18:29 aspiers hence why it's still present in that Venn diagram I just linked
17:18:39 cdent I honestly think that we need to make the system break when people break it
17:18:46 cdent not try to fix it for them
17:18:53 aspiers shit, actually that diagram is wrong isn't it :-/
17:19:25 cdent we want failures to show as soon as possible
17:19:40 cdent not be in a situation where every 5 minutes system a removes a trait and every 7th minute something puts it back
17:19:40 aspiers yeah I'm totally OK with the "if you do that you get to keep all the broken pieces" approach
17:19:49 aspiers good point
17:20:35 mriedem fried_rice: yes, i look at this bug every couple of weeks and then fall into a pit of depression
17:20:41 mriedem because all solutions out of this bug are bad
17:21:13 fried_rice mriedem: Seems like if you're removing that piece of the comment, you should add a TODO to fix that bit. Assume there's a bug associated with it, yah?
17:21:28 mriedem fried_rice: see the related bug at the bottom of the commit message
17:22:06 mriedem i thought about adding something like, NOTE(mriedem): This actually causes bug 1790204. - but the fix for that bug might not actually involve changing that code
17:22:07 openstack bug 1790204 in OpenStack Compute (nova) "Allocations are "doubled up" on same host resize even though there is only 1 server on the host" [High,Triaged] https://launchpad.net/bugs/1790204
17:22:22 mriedem i can add it if it's helpful though
17:22:48 mriedem also, having said that, i don't think we hit that code on resize anymore
17:23:06 mriedem see, "(10:47:20 AM) mriedem: i imagine this is all dead code since queens or at least rocky https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L140"
17:23:39 mriedem that's only called if the instance consumer has allocations prior to scheduling, which it shouldn't for resize https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1595
17:23:47 mriedem because we swapped those to the migration record in conductor prior to calling the scheduler
17:24:06 mriedem i think _move_operation_alloc_request only happens during evacuate now
17:24:21 mriedem and you thankfully can't evacuate to the same host :)
17:24:50 sean-k-mooney we will proably get a bug request for that at some point
17:24:59 sean-k-mooney but its true
17:25:06 sean-k-mooney and it should never be possible
17:25:13 fried_rice can we prove it and just remove the code path?
17:25:15 mriedem you can't evacuate to a host that is down
17:25:31 mriedem and you can't evacuate while the service is up
17:25:50 mriedem fried_rice: we can't remove it because of evacuate
17:26:03 fried_rice not the method, just that condition, right?
17:26:09 fried_rice maybe I misunderstood what you said.
17:27:12 mriedem current design ideas on the resize to same host bug is in https://bugs.launchpad.net/nova/+bug/1790204/comments/14
17:27:13 openstack Launchpad bug 1790204 in OpenStack Compute (nova) "Allocations are "doubled up" on same host resize even though there is only 1 server on the host" [High,Triaged]
17:27:15 fried_rice I thought you said: you can only hit this method during evacuate, and you can't evacuate to same host, so `elif not new_rp_uuids` should never happen
17:27:18 mriedem which is built on an earlier comment from leakypipes
17:27:50 mriedem oh yeah this https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L172
17:27:57 mriedem umm, i think that's probably true
17:28:10 mriedem so rather than me remove that comment, just remove the code
17:28:24 fried_rice meaning L169-179 can be reduced to L170-171
17:28:43 fried_rice but only if we can prove that we only call this method for evacuate.
17:28:57 mriedem i'll fart with it
17:29:14 mriedem any opinions on the brain dump in https://bugs.launchpad.net/nova/+bug/1790204/comments/14 are also appreciated
17:29:15 openstack Launchpad bug 1790204 in OpenStack Compute (nova) "Allocations are "doubled up" on same host resize even though there is only 1 server on the host" [High,Triaged]
17:29:28 mriedem because there have been several "what if we did this?" talking to myselfs in that bug report by now
17:32:19 cdent a) i'm glad mriedem is looking into this and playing the what if game, b) i'm sad that we have some many what ifs in this part of the code, c) let's rewrite the whole thing!
17:32:57 mriedem if we rewrote the whole thing, we'd just (d) regress some other shit corner case we didn't think about
17:33:34 mriedem the retrospective on this is probably that we are not good about testing at-capacity hosts
17:33:51 mriedem our functoinal tests are always building and migrating to hosts with near infinite capacity
17:33:56 cdent regressions is how we create new contributors :D
17:35:07 mriedem our public cloud ops team is super anal about packing as much as possible, so that's how they noticed this
17:36:06 melwitt any new contributors want to write at-capacity test coverage? :P
17:36:44 mriedem well we have https://review.openstack.org/#/c/619123/ for this bug at least
17:36:49 melwitt (either way, we need it)
17:37:06 mriedem not really sure we have new contributors
17:37:12 mriedem we have old grizzled contributors
17:37:14 melwitt yeah. I was just kidding
17:37:20 mriedem aspiers is new i guess
17:37:22 mriedem full of hope
17:37:29 mriedem (new to nova)
17:37:42 melwitt haha (it's true)
17:37:51 temka So you're saying we should break him by making him convert mox to mock?
17:38:01 mriedem no i never suggested anyone do that
17:38:05 mriedem i actively lobbied against that
17:38:08 mriedem to no avail
17:38:40 cdent too old. too grizzled. to no avail.
17:38:46 temka mriedem, btw, I think we need to put the brakes on https://review.openstack.org/#/c/636210/9 for now - the changed unit tests are failing in the patch above it: http://logs.openstack.org/05/634605/16/check/openstack-tox-lower-constraints/83c2d08/testr_results.html.gz
17:38:56 temka Is my -W enough?
17:39:04 mriedem nope
17:39:12 mriedem you'll have to change the commit message or something to pull it from zuul
17:39:59 kashyap mriedem: Since you politely implied `git blame` on the version bump patch ... :-) I'd want to certinly tell you that the failure was _not_ related to it.
17:40:57 openstackgerrit Artom Lifshitz proposed openstack/nova master: [DERP] Remove _legacy_dict methods https://review.openstack.org/636210
17:40:58 openstackgerrit Artom Lifshitz proposed openstack/nova master: Add migration param to check_can_live_migrate_destination https://review.openstack.org/634605
17:40:58 openstackgerrit Artom Lifshitz proposed openstack/nova master: Introduce live_migration_claim() https://review.openstack.org/635669
17:40:59 openstackgerrit Artom Lifshitz proposed openstack/nova master: Use live_migration_claim() to check dest resources https://review.openstack.org/634606
17:41:00 openstackgerrit Artom Lifshitz proposed openstack/nova master: New objects to transmit NUMA config from dest to source https://review.openstack.org/634827
17:41:00 openstackgerrit Artom Lifshitz proposed openstack/nova master: LM: Make dest send NUMAMigrateData to the source https://review.openstack.org/634828
17:41:00 openstackgerrit Artom Lifshitz proposed openstack/nova master: LM: update NUMA-related XML on the source https://review.openstack.org/635229

Earlier   Later