| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-05 | |||
| 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 | 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 | |