Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-05
18:39:15 efried and in that case, since those are RequestGroup, absolutely we would have already funneled everything forbidden_aggregates into forbidden_aggregates.
18:39:24 dansmith yup, agree there
18:39:24 artom mriedem, yep, shamelessly reviewing your bugfixes to quary favour
18:39:39 efried The fact that anything that still needs to be translated to placement-ese lives in Destination (or elsewhere) is, to my way of thinking, tech debt.
18:39:42 mriedem favoUr?!
18:39:56 artom mriedem, *shrug* I changed it to a +1, didn't I? Given what dansmith said afterwards, looks like my concern was valid
18:40:22 artom mriedem, yes, the Queen's spelling
18:40:37 dansmith efried: because the words "forbidden" and "aggregates" in that order are forever trademarked by placement?
18:40:53 dansmith efried: call them excluded_aggregates in the Destination object and that'd also be fine
18:41:31 dansmith artom: here and I was all ready to defend you, but I can't get behind the queen and her broke-ass spelling
18:44:55 artom dansmith, thank you, you know you're my favourite
18:45:01 dansmith grr.
18:45:48 efried dansmith: but for the sake of shilpasd taking action, which will happen while we sleep, Destination.forbidden_aggregates will work for you, yes?
18:46:22 dansmith efried: I think I've already said it would
18:46:45 efried It's worth clarifying
18:46:48 efried measure twice, cut once
18:47:09 dansmith if this wasn't codified in our object schema until version 2.0, you could totally claim that arguing over naming is not worth the trouble
18:47:18 dansmith but this stuff will stick with us for a long time
18:47:50 dansmith so while you try to make it sound like I'm being unreasonably pedantic about the naming, I think I have a lot of version bumping battle scars (which nobody else has, btw) to back up my reasoning
18:48:17 dansmith (*object version bumping scars.)
18:48:48 efried dooood
18:48:59 efried I'm not trying to make it sound like you're being unreasonable
18:49:06 efried all I've said is I don't see the difference
18:49:15 efried and I'm happy to defer to your judgment.
18:49:33 efried which is why I ask you for reviews on object/RPC/etc stuff
18:50:07 aspiers efried: quick question, I want to reuse most of the fixture in test_config_kvm() https://github.com/openstack/nova/blob/master/nova/tests/unit/virt/libvirt/test_config.py#L2511 in new test I'm adding to test_designer.py - should I move it to fake_libvirt_data.py in a separate commit before reusing it, or move it as part of the commit adding the new test?
18:51:09 aspiers artom: BTW this is how our discussion with sean-k-mooney turned out if you're curious https://review.opendev.org/#/c/680065/
18:51:43 efried aspiers: is the patch you're adding just a test?
18:51:57 aspiers efried: no, it's the whole "apply SEV config" patch
18:52:15 efried yar. Then do it separately.
18:52:18 aspiers OK thanks!
18:53:09 artom aspiers, yep, logic looks good
18:53:17 aspiers artom: cool, thanks
18:53:46 aspiers artom: fairly close to having the follow-on commit ready which adds the machine type check to the driver
18:54:23 aspiers I'm using sean-k-mooney's nice idea of having the machine_type parameter in hardware.py optional, so the checking code can be reusedin both scenarios
18:54:44 aspiers took several redesigns but I think we're finally there
19:15:30 efried dansmith: moving up, the current ordering is my fault: I thought it was worse to first expose a conf opt that doesn't do anything, followed by the code that does the thing; than to introduce and unit test all the code, followed by a "master switch" that exposes it to the user in one go.
19:15:53 efried and thus the latter is the approach I've been advocating to *all* the series I've been reviewing this cycle.
19:16:00 dansmith efried: it is, I'm not suggesting to put the conf toggle first, of course
19:16:14 dansmith I'm suggesting you get all the plumbing that is in the last patch in front, and then land the filter and its knob together last
19:18:48 efried dansmith: that would effectively be combining the last two patches afaict. Which last-patch plumbing do you mean? Add the filter to the list but put an `if True: return` at the top of the filter method, and then s/True/CONF..../ in the last patch?
19:19:01 dansmith efried: nova-status, scheduler/report.py, utils.py and associated tests can all be done ahead of time
19:19:04 dansmith from the last patch
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 dansmith it's likyes
19:20:49 efried reviews
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: DB API changes to get non-matching aggregates from metadata https://review.opendev.org/671074
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:54 openstackgerrit Eric Fried proposed openstack/nova master: Enable request filter isolate_aggregates https://review.opendev.org/667952
19:44:54 openstackgerrit Eric Fried proposed openstack/nova master: Add a new request filter to isolate aggregates https://review.opendev.org/671075
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

Earlier   Later