Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-05
19:20:45 efried Yes, actually, utils.py looks like it should have been earlier regardless. client/report could just be flattened. I think the only reason these are split is to make the review smaller.
19:20:49 efried reviews
19:20:49 dansmith it's likyes
19:20:54 efried which is probably also my fault.
19:21:09 dansmith it's like the "uh, add the conf knob and all the other shit I forgot about" patch
19:21:35 efried if it was just the former, it would be okay IMO
19:22:00 dansmith there's no reason to land the filter and the knob separately
19:22:03 dansmith there is reason to land the plumbing separate from the filter/knob
19:22:15 dansmith and it's fine to add the docs in a separate patch, which could slip past FF even
19:22:41 efried yeah, I just don't actually see any "plumbing" to speak of in that last patch
19:22:54 dansmith well, like you said, it's stuff that should have been earlier
19:23:16 dansmith the utils change, the placement version requirement bump, etc
19:23:55 efried okay, so again for the sake of being able to give shilpa a clear message, would it be acceptable to move the .py bits (including conf knob) of the last patch into the second-last patch, and leave the last patch for docs/reno?
19:24:27 efried because rn I don't think she understands that there's an action to be taken, based on her response.
19:25:44 dansmith I think that the sorted change in utils is probably its own patch, with tests so that it's easy to validate which test is confirming that, then yes the filter, knob,
19:26:00 dansmith isn't the report client bit actually already late?
19:26:27 dansmith meaning we already have code landed to put the not-member-of stuff in the qparams, we just won't ever run that code nor ask for the right version?
19:26:30 efried dansmith: it's not necessary until there's code that can hit the placement request with forbidden agg syntax
19:26:37 dansmith if so, that would also be a "whoopsie" pre-patch, IMHO
19:27:14 dansmith efried: mixing that up is not great to me.. if there's code that, if called, could generate a query that needs a newer version but it won't be provided,
19:27:17 dansmith that was a mistake
19:27:25 dansmith but whatever
19:27:39 efried yeah, could be perceived as "we can't hit this code so it doesn't matter" regardless of the reason we can't hit it, but I see your point.
19:28:00 dansmith anyone else could land a patch right now that calls that code in such a way that it won't generate a sensible placement request
19:28:07 dansmith and since it's kinda library/client code, it should be landed in a way that makes it work if called, IMHO
19:28:35 efried ack
19:28:36 dansmith so, to summarize,
19:28:41 dansmith if it were me, I would ask for:
19:29:03 dansmith 1. A patch to fix up all the "oops I forgot" stuff in the placement client section, which is a chunk of the last patch currently
19:29:12 dansmith 2. A patch to add the filter and its knob
19:29:42 dansmith 3. Docs (and reno) at the end, which is the remaining bit of the last patchI think (if you subtract some of the random changes that are in there like renaming a test)
19:30:14 mriedem artom: some comments/questions on patches 2 and 3, onto the big one https://review.opendev.org/#/c/634606/
19:31:04 artom mriedem, cheers! I have to pick up kids from school in 15, so my responses will have to wait later tonight, and probably into tomorrow morning
19:32:20 efried dansmith: okay. I think I could probably do that shuffle and still +2 without losing sleep.
19:32:37 dansmith efried: agree
19:35:42 mriedem artom: yeah nothing major so far, just +1 with "i'm still going, but answer before i +2"
19:37:11 dansmith mriedem: presume you've seen the CI job logs that shows this working too.. it's handy to trawl that looking for things to line up
19:37:26 dansmith and also, sean-k-mooney did some manual testing yesterday that was meaningful to me, with edge cases and such
19:38:06 artom "+1 I'm still going" is all we can hope from life
19:43:50 mriedem dansmith: yes i was https://zuul.opendev.org/t/openstack/build/d51f91efa937411a9179b930eff3ab08/log/controller/logs/screen-n-cpu.txt.gz#2199
19:44:50 efried rebase only...
19:44:53 openstackgerrit Eric Fried proposed openstack/nova master: Nova object changes for forbidden aggregates request filter https://review.opendev.org/671072
19:44:53 openstackgerrit Eric Fried proposed openstack/nova master: DB API changes to get non-matching aggregates from metadata https://review.opendev.org/671074
19:44:54 openstackgerrit Eric Fried proposed openstack/nova master: Add a new request filter to isolate aggregates https://review.opendev.org/671075
19:44:54 openstackgerrit Eric Fried proposed openstack/nova master: Enable request filter isolate_aggregates https://review.opendev.org/667952
19:47:17 mriedem dansmith: there is no up-call in this https://review.opendev.org/#/c/656594/
19:47:22 mriedem that code is only ever called from the api
19:47:49 dansmith mriedem: is it?
19:48:00 dansmith I thought we called down to the compute which did this
19:48:04 mriedem no
19:48:42 mriedem maybe nova-net, idk about that, nor really care about nova-net
19:49:05 mriedem this is just a proxy call from nova-api to neutron
19:49:08 dansmith sure, I just thought we still had to do FIP associate on the compute, like for LB or something
19:49:26 mriedem http://codesearch.openstack.org/?q=network_api%5C.associate_floating_ip&i=nope&files=&repos=
19:49:31 mriedem i guess mogan cares
19:50:43 dansmith okay, well, I did say I hadn't chased all the plumbing
19:51:15 dansmith I think the thing that led me there, aside from artom's comment,
19:51:34 dansmith was that you're saying "if we don't have access to the API DB" which can't happen without nothing else working, if it's just in the api
19:52:00 dansmith and the log message to that effect
19:52:43 dansmith the other change you reference was to handle calling code that could be either api-level or inside the cell
19:52:59 dansmith but if this is only ever at the API, then I don't think there's any need to catch that situation
19:53:05 dansmith and if you do,
19:53:10 dansmith it's not info-level, it's error-level,
19:53:20 dansmith along with all the other tracebacks the api would be throwing if you even got that far :)
19:56:43 mriedem ok, like i said, the CantStartEngineError thing came up in denver i think, but it's not written down so i can't remember the details for sure - i don't need it, and agree it shouldn't happen - and if it does, we're more f'ed than just this, so i'm happy to remove that in a follow up when i fix that comment in the test
19:57:11 mriedem or, if i'm backporting, just nack it and i can remove that now
19:57:28 dansmith ack, commenting
19:58:55 dansmith mriedem: done. left my +2, you can decide to fix or fup
19:59:07 dansmith for backport, I'd think it's worth fixing before it lands, since it's like 24 hours away :)
20:04:12 mriedem to be clear,
20:04:14 mriedem "So, if you're going to catch it and log, I'd say do so at error and then re-raise the exception like all the other API code would."
20:04:18 mriedem you're OK with just removing that catch right?
20:05:16 dansmith yes, better to just remove it, imho
20:05:27 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Find instance in another cell during floating IP re-association https://review.opendev.org/656594
20:06:38 dansmith mriedem: you doing it now? I'll stand by to fast approve if so
20:07:35 mriedem no, still going through this big numa lm patch
20:07:44 mriedem just got through the commit message...
20:08:47 openstackgerrit Eric Fried proposed openstack/nova master: Nova object changes for forbidden aggregates request filter https://review.opendev.org/671072
20:08:48 openstackgerrit Eric Fried proposed openstack/nova master: DB API changes to get non-matching aggregates from metadata https://review.opendev.org/671074
20:08:48 openstackgerrit Eric Fried proposed openstack/nova master: Add a new request filter to isolate aggregates https://review.opendev.org/671075
20:08:49 openstackgerrit Eric Fried proposed openstack/nova master: Docs for isolated aggregates request filter https://review.opendev.org/667952
20:09:11 efried dansmith: gotta go on next chauffeur run, but I think that's what we were shooting for ^
20:10:16 efried basically half of the .py changes needed to be in the first patch, the other half in the third.
20:11:12 efried guess I should have done that field rename while I was in there, sec...
20:15:53 openstackgerrit Eric Fried proposed openstack/nova master: Nova object changes for forbidden aggregates request filter https://review.opendev.org/671072
20:15:53 openstackgerrit Eric Fried proposed openstack/nova master: DB API changes to get non-matching aggregates from metadata https://review.opendev.org/671074
20:15:58 openstackgerrit Eric Fried proposed openstack/nova master: Add a new request filter to isolate aggregates https://review.opendev.org/671075
20:15:58 openstackgerrit Eric Fried proposed openstack/nova master: Docs for isolated aggregates request filter https://review.opendev.org/667952
20:16:13 mriedem totally unrelated to the numa live migration stuff, but can this fail the 'claim' for sriov pci devices on the dest host? https://github.com/openstack/nova/blob/f7f5e1846c7b19aa05817df7f3c4345819db413f/nova/compute/manager.py#L6491
20:16:16 mriedem and if so, how does it fail?
20:17:07 mriedem because there is a very specific set of exceptions that will trigger a reschedule to an alternate host https://github.com/openstack/nova/blob/f7f5e1846c7b19aa05817df7f3c4345819db413f/nova/conductor/tasks/live_migrate.py#L482
20:17:23 mriedem so if that stuff doesn't result in MigrationPreCheckError we'll fail the live migratoin b/c the first host claim failed
20:17:26 mriedem sean-k-mooney: ^
20:18:41 openstackgerrit Dan Smith proposed openstack/nova master: Find instance in another cell during floating IP re-association https://review.opendev.org/656594
20:18:44 mriedem trying to follow that pci claim code is going down a rabbit hole
20:18:51 dansmith mriedem: ^
20:20:50 mriedem want to hit that test comment while you're updating?
20:43:26 openstackgerrit Merged openstack/nova master: neutron: refactor nw info cache refresh out of associate_floating_ip https://review.opendev.org/678300
20:56:31 mriedem oooo https://github.com/openstack/nova/blob/f7f5e1846c7b19aa05817df7f3c4345819db413f/nova/scheduler/filter_scheduler.py#L173-L187 - if it weren't for USES_ALLOCATION_CANDIDATES we could burn all of that queens compat code out of the filter scheduler
20:57:51 mriedem artom: dansmith: ok comments in the big one https://review.opendev.org/#/c/634606/75

Earlier   Later