| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-05 | |||
| 18:25:43 | efried | Surely that's only the case for a non-resize-y move | |
| 18:25:44 | dansmith | we recalculate some of it, and what we recalculate has drifted a LOT over time | |
| 18:25:57 | dansmith | which is my point, | |
| 18:26:00 | efried | mm, "some of it". | |
| 18:26:07 | dansmith | even if we blow it away currently, | |
| 18:26:26 | dansmith | we're storing the values as "isolated aggregates" and not "isolated (at the time of boot which was six months ago) aggregates" | |
| 18:26:35 | dansmith | "forbidden" doesn't imply a reason | |
| 18:26:55 | dansmith | they were forbidden, they may no longer be forbidden for reasons related to this request or config, etc | |
| 18:27:38 | efried | I understand the issue now, thanks. Though I still fail to see how the word choice 'forbidden' vs 'isolated' has any power to clarify. Seems like a distinction without a difference, certainly not one that gives any hint about this persistence-vs-not issue. | |
| 18:28:28 | efried | not recalculating aggregate isolation would be a bug for sure, regardless of what we called it. | |
| 18:28:32 | dansmith | forbidden implies what we're going to do with it... exclude those aggregates | |
| 18:28:39 | efried | I would possibly even go so far as to say that not recalculating *any* request filter would be a bug. | |
| 18:28:48 | dansmith | isolated implies *why they are forbidden, which is based on a point-in-time configuration | |
| 18:29:57 | efried | I'm normally the pedantic, specific-word-choice guy, but I don't see it, sorry. So I'm sticking with my earlier story: if it makes the difference to you, it's fine with me. | |
| 18:30:07 | dansmith | in other news, while looking for a reference for you, I realize that we now exclude destination from the reqspec when serializing, so it won't get persisted | |
| 18:30:20 | dansmith | good thing we had a discussion instead of you just pushing until I agree to something eh? | |
| 18:30:21 | efried | oh, that's nice :) | |
| 18:30:38 | efried | dude, that was never happening, you're making that part up. | |
| 18:30:46 | dansmith | shall I quote you? | |
| 18:30:48 | efried | please do | |
| 18:30:52 | dansmith | I mean seriously | |
| 18:30:56 | efried | I am serious | |
| 18:31:17 | dansmith | *eyeroll* | |
| 18:31:17 | efried | I at no time asked or even implied that you should roll over and approve something you disagreed with. | |
| 18:31:23 | dansmith | no, that's not what I said | |
| 18:31:40 | dansmith | I said *you* were just going to ask for the change so I'd agree to it without discussion | |
| 18:31:50 | dansmith | which you said a few minutes ago, and earlier this morning | |
| 18:32:18 | efried | oh, yeah, true story, I was avoiding what I saw as a bikeshed over a name choice | |
| 18:32:18 | dansmith | which is not the point of review and of course not the reason I'd ask for a change, just to be appeased | |
| 18:33:06 | dansmith | so, here's my other reason for still thinking this is the wrong name for it | |
| 18:33:06 | efried | because I didn't (and still don't) see how 'isolated' and 'forbidden' mean different things in this context *other* than nova ux vs placement api terminology. | |
| 18:33:25 | dansmith | presumably we're going to need to forbid aggregates for other reasons in the future, for other nova features unrelated to aggregate isolation right? | |
| 18:33:43 | mriedem | efried: dansmith: i dropped and maybe you already talked about this, or maybe it doesn't matter, but request spec's requested_destination isn't persisted | |
| 18:34:00 | dansmith | heh | |
| 18:34:01 | dansmith | yes, we've covered that already :) | |
| 18:34:04 | mriedem | it used to be in the long ago and caused all sorts of problems | |
| 18:34:11 | dansmith | yup | |
| 18:35:13 | dansmith | the destination object is all about what requests we're making to the scheduler and placement downstream of it, and since it's rpc, putting something in there means that's the interface and changing it later requires a version bump | |
| 18:35:23 | efried | dansmith: that's an interesting point, yes. I'm not fully swapped in on the code still, but I would think that would be a good reason to have the Destination field have 'isolated_aggregates', so that we can add 'frobnicated_aggregates' later and merge all of those into 'forbidden_aggregates' for the placement call. | |
| 18:35:50 | dansmith | so if we ever think conductor may ask to forbid an aggregate for any other reason, we'd have to shove it into "isolated_aggregates" to make that happen, or add another field with the same purpose | |
| 18:36:05 | efried | the latter, yes, IMO | |
| 18:36:16 | efried | no? | |
| 18:36:30 | dansmith | efried: why? the scheduler doesn't need to know why to exclude aggregates, it just needs to know that it should | |
| 18:36:52 | efried | well | |
| 18:37:12 | dansmith | AZs are aggregates, what if I were to ask next cycle for a "disabled" flag on AZs? all we would need to do is tell scheduler (and thus placement) that AZ aggregate $uuid should be forbidden | |
| 18:37:19 | efried | gimme a sec to make sure I'm thinking of the right object hierarchy here... | |
| 18:37:28 | dansmith | it's not isolated in the notion that nova's new isolated aggregate feature means, it's something else | |
| 18:38:48 | efried | ...so IMO what we want to be working toward is to have the placement-isms represented all and only in the RequestSpec.requested_resources field. | |
| 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 | artom | mriedem, yep, shamelessly reviewing your bugfixes to quary favour | |
| 18:39:24 | dansmith | yup, agree there | |
| 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 | 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 | |