| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-05 | |||
| 17:10:33 | mriedem | like a mfing renegad | |
| 17:10:36 | mriedem | *renegade | |
| 17:11:13 | mriedem | artom: dansmith: just got done going through that bw provider migration series again, now lunch and then i swear on my sweet lucy cat's whiskers that i'll review that numa live migration series | |
| 17:12:37 | artom | mriedem, appreciated :) | |
| 17:16:03 | efried | A couple on there still that don't seem likely, but aren't obvious so I'm letting them slide off and will punt them next week. | |
| 17:16:03 | efried | I think I've deferred all the obvious deferrals at this point. https://blueprints.launchpad.net/nova/train | |
| 17:21:59 | dansmith | mriedem: thanks | |
| 17:22:12 | dansmith | mriedem: I'm sure you'll find some stuff, so just want to make sure there's time to address it | |
| 17:33:03 | donnyd | melwitt: dansmith for the record today I have only thrown 4 errors from image related tasks... usually it is about 5x that | |
| 17:33:20 | donnyd | thanks for sticking with me on it | |
| 17:33:37 | melwitt | mriedem: noted | |
| 17:34:00 | dansmith | donnyd: cool | |
| 17:34:10 | melwitt | donnyd: \o/ | |
| 17:41:15 | melwitt | mriedem: +2 | |
| 17:51:20 | mriedem | artom: you could have probably asked in irc or commented without a vote on https://review.opendev.org/#/c/656594/ | |
| 17:51:22 | mriedem | but i've replied | |
| 17:51:37 | mriedem | i see you've gone hunting for my changes... | |
| 17:57:23 | dansmith | ah yes, "I plan to backport this" is always a good reason :D | |
| 17:57:27 | mriedem | don't get me wrong, i appreciate the reviews | |
| 17:57:53 | mriedem | and lord knows artom is heavily in debt in this regard :P | |
| 17:58:01 | dansmith | artom: protip: tell him you'll +1 if he queues up a patch behind to do the right thing that needn't be backported :P | |
| 17:58:23 | mriedem | i still won't do what he's suggesting, but he can do that | |
| 17:58:28 | dansmith | haha | |
| 17:59:24 | dansmith | well, probably still a good thought anyway.. at least he's thinking of front-loading api-related stuff in the api, so I'll give him credit | |
| 17:59:31 | mriedem | not to incur debt myself, but if there are any dan's around looking for reviews that is the bug fix for the multi-cell floating IP disassociate issue i found in denver... | |
| 17:59:55 | mriedem | i agree it's not crazy | |
| 18:00:19 | dansmith | assuming you're moving onto the numa lm patches I could probably look for a reason to -1 your fix | |
| 18:00:38 | mriedem | when i don't see lee around much i need to move onto someone else to jab | |
| 18:00:45 | mriedem | yeah yeah i'm on it | |
| 18:01:32 | mriedem | btw, assuming anyone else is watching zuul, looks like it's taking around at least 5 hours to get a node | |
| 18:02:15 | mriedem | (679473,1) Handle VirtDriverNotReady in _cleanup_running_deleted_instances (17h46m/ | |
| 18:02:20 | mriedem | that seems...less than good | |
| 18:21:27 | dansmith | crikey, yeah it's backed wayy up | |
| 18:22:43 | openstackgerrit | Adam Spiers proposed openstack/nova master: Ensure non-q35 machine type is not used when booting with SEV https://review.opendev.org/680065 | |
| 18:23:03 | aspiers | sean-k-mooney, kashyap, efried: I think this one should be good now ^^^ Reworking the other few on top now ... | |
| 18:23:57 | efried | dansmith: Can you help me understand under what circumstances a Destination gets persisted and then also reused later to influence a scheduling decision? | |
| 18:24:24 | dansmith | efried: it's serialized inside the reqspec yeah? | |
| 18:24:37 | efried | dansmith: because if we're not reinvoking the scheduler with filters to recalculate isolated aggregates on a move operation, then I agree that's a problem. | |
| 18:25:19 | efried | so we calculate a reqspec once for an instance and then reuse it for move operations from that point on without reworking the stuff inside it?? | |
| 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 | efried | I at no time asked or even implied that you should roll over and approve something you disagreed with. | |
| 18:31:17 | dansmith | *eyeroll* | |
| 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 | 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:32:18 | efried | oh, yeah, true story, I was avoiding what I saw as a bikeshed over a name choice | |
| 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:06 | dansmith | so, here's my other reason for still thinking this is the wrong name for it | |
| 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 | 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? | |