| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-05 | |||
| 19:19:16 | dansmith | then you can add the filter and the knob, | |
| 19:19:22 | dansmith | and docs could be in there or in a following patch | |
| 19:19:36 | dansmith | the last patch is a jumble of things | |
| 19:19:44 | dansmith | some of which are like rebase noise or something | |
| 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: ^ | |