Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-05
15:45:32 dustinc efried: I’d really like to get it landed in train and am working towards that. I hope to get it reviewable again by eod today.
15:46:14 efried dustinc: I would like that too, but unfortunately I don't feel like it's close enough given how much other stuff we're trying to cram in in the next week.
15:48:05 efried dustinc: not because you did anything wrong, just because there has been more to review than reviewers have been able to keep up with, and this is one that slid off the bottom.
15:48:07 dansmith efried: https://review.opendev.org/#/c/671072/11/nova/scheduler/utils.py
15:49:21 efried dansmith: ack. You da boss. shilpasd ^
15:49:42 dansmith efried: you just saw a wall of text and gave in without reading didn't you?
15:50:12 efried no, I read it. I don't agree, but it's more important to you than to me and I want to see it move forward. So let's do that.
15:50:36 dansmith well, that's constructive
15:50:39 efried dansmith: tbc, what you want to see is the field in Destination named forbidden_aggregates rather than isolated_aggregates. Everything else is okay?
15:52:19 dustinc efried: I understand. I will keep working on it anyway but if the review bandwidth isn’t there then go ahead and punt it to U. Thanks for your time spent reviewing it so far.
15:52:59 dansmith efried: I think the set is kinda upside down, which I assume you also saw from the later ones, but what I'm gathering is that by asking me to review, you wanted me to either just approve or only look shallow for things that physically won't work instead of things that might bite us in the future
15:53:14 dansmith efried: so maybe I should just drop my -1 and you find someone looking to do _that_ kind of review?
15:55:03 efried dansmith: Again, multitasking heavily right now, so I haven't read all of whatever reviews have been done today. I don't want you to rage quit. If there are other nontrivial issues you've called out, then those should be addressed.
16:24:45 mriedem gibi: so close https://review.opendev.org/#/c/680394/2 but there are missing tests for the new _reschedule condition
16:52:26 mriedem gibi: a few easy things to address on the 2 after that as well
16:56:04 openstackgerrit raphael.glon proposed openstack/nova master: Ironic driver: fix when entering rebuild while already in error https://review.opendev.org/680468
17:04:40 openstackgerrit Dustin Cowles proposed openstack/nova-specs master: Spec: Provider config YAML file https://review.opendev.org/680471
17:06:59 mriedem Ironic driver: fix when entering rebuild while already in error https://review.opendev.org/680468 is another duplicate of https://review.opendev.org/#/c/523559/ which is pretty old so maybe we should get that in?
17:07:08 mriedem melwitt: ^?
17:08:35 melwitt mriedem: thanks for the reminder, I was thinking about that the other day. I do want to review that. been dreading re-loading of context on that one
17:10:16 mriedem i think given the amount of time, the review it's had, the duplicates, and the fact it was proposed by an operator of an ironic deployment and this fixes their issue, and it's so narrow in focus, it's pretty low risk to just pull the trigger
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 I think I've deferred all the obvious deferrals at this point. https://blueprints.launchpad.net/nova/train
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: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 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

Earlier   Later