| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-02-27 | |||
| 13:16:31 | sean-k-mooney | tkajinam: but i would supprot tryign to focus on this early next cycle | |
| 13:18:11 | tkajinam | sean-k-mooney, thank you ! | |
| 13:33:52 | sean-k-mooney | just level settign with folks. while im not there yet im gettign close to my context swtiching limit so i likely wont have tiem to do many more reviews today at least not until i take a short break to reset | |
| 13:34:10 | sean-k-mooney | i see two feature that are clsoe to mergign that we might be able to get over the line | |
| 13:34:31 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/938604 and https://review.opendev.org/c/openstack/nova/+/938405 | |
| 13:34:59 | sean-k-mooney | so schduler hints in server show and the managed flag for pci passhtough | |
| 13:35:33 | sean-k-mooney | i will try to look at the follow up for pci live migratoin however im not sure ill have time to properly review that today but ill try | |
| 13:36:01 | gibi | ack | |
| 13:36:30 | gibi | the managed flag is approved going through the gate, the live-migratable series is under review by my at the moment | |
| 13:37:57 | sean-k-mooney | im too cold on the vtpm serices to meanining fully look at that today and honetly it has enough WIP patches that i dont think it viable currently | |
| 13:38:11 | sean-k-mooney | is there anything else i shoudl have on my radar or do you think those 3 are it | |
| 13:39:19 | sean-k-mooney | i dont consider bumping the min libvirt version to be a feature https://review.opendev.org/c/openstack/nova/+/942871 so i think that can merge between FF and rc1 | |
| 13:39:33 | sean-k-mooney | so im treating that as a lower priority | |
| 13:40:00 | gibi | yeah I think vtpm slips to F | |
| 13:40:52 | sean-k-mooney | ok im going to step away for 20 mins then and ill try an take a look at the migration series when i return | |
| 13:41:04 | gibi | while there are some raw edges in the live-migratable series it feels pretty close to me. I don't know how much brainpower bauzas has to look at that today | |
| 13:48:01 | bauzas | gibi: I'll look at it after some meeting | |
| 13:48:16 | gibi | ack (sending good wibes) | |
| 13:57:07 | tkajinam | I know everyone is busy but if you can spare some moments to review this specless bp then that'd be awesome https://review.opendev.org/c/openstack/nova/+/845660 | |
| 13:57:46 | tkajinam | I just rebased it to make sure it works with the latest code and likely need to leave before CI posts the latest result | |
| 13:58:05 | bauzas | tkajinam: my hard effort for today will be to purge the status etherpad and you're part of it :) | |
| 13:58:19 | tkajinam | \o/ | |
| 14:00:36 | tkajinam | this conflicts with the sev-es patches but I'm fine with pushing the sev-es work to next cycle and let that multipath change go first, given the complexity of sev-es series | |
| 14:01:46 | bauzas | thanks for telling the priorities | |
| 14:26:49 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Add new option to enforce multipath volume connections https://review.opendev.org/c/openstack/nova/+/845660 | |
| 14:29:06 | tkajinam | os-brick made a breaking interface change and that is part of the latest release. ugh | |
| 14:37:19 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Add new option to enforce multipath volume connections https://review.opendev.org/c/openstack/nova/+/845660 | |
| 15:01:07 | sean-k-mooney | tkajinam: looks like its small enough to add to the review list | |
| 15:01:14 | sean-k-mooney | for today | |
| 15:01:23 | sean-k-mooney | but i guess we will see | |
| 15:15:58 | sean-k-mooney | tkajinam: +2 with comments | |
| 15:16:20 | dansmith | tkajinam: I had a couple comments but didn't -1 | |
| 15:16:26 | dansmith | tkajinam: if you fix them I'll be +2+W | |
| 15:19:07 | bauzas | tkajinam: I have one concern https://review.opendev.org/c/openstack/nova/+/845660/15/nova/virt/libvirt/driver.py#1177 | |
| 15:19:08 | sean-k-mooney | dansmith: replied im not agaisnt InvalidConfiguration but it think its subtlly diffent | |
| 15:20:08 | bauzas | tkajinam: you were litterally speaking about how os-brick can break some API by a new major version, I'm a bit afraid of us pulling a hard dependency on some osbrick submodule at some driver initialization | |
| 15:20:32 | sean-k-mooney | dansmith: so if its updated im ok with that but i generally think of InvalidConfiguration as stricly configuring conflicting options or missign requried ones based on the current set values | |
| 15:21:09 | sean-k-mooney | dansmith: but i dont think i would be confused if this was InvalidConfiguration as an operator either | |
| 15:22:02 | dansmith | sean-k-mooney: I replied | |
| 15:22:51 | dansmith | bauzas: only if multipathd is configured as required and you could sidestep that problem by changing it to not if you had to for a package conflict | |
| 15:23:07 | dansmith | bauzas: to me, the more of this kind of stuff we delegate to brick (no matter where it is in their module hierarchy) the better | |
| 15:23:28 | dansmith | if they break their api and thus us, then so be it, like any other library | |
| 15:23:36 | bauzas | dansmith: sure, that's not the fact that we delegate to brick at startup | |
| 15:23:57 | bauzas | that's about calling a specific brick connector module to get that | |
| 15:24:01 | dansmith | it's that it's using a given driver | |
| 15:24:02 | sean-k-mooney | we also dont have a connecto obejct to use | |
| 15:24:11 | bauzas | while connectors do that already | |
| 15:24:12 | dansmith | sure, but the only thing that would break that should be a requirements change | |
| 15:24:14 | sean-k-mooney | we woud need to constract athat for a given voluem first | |
| 15:24:44 | bauzas | tbc, I haven't -1 the patch | |
| 15:24:51 | sean-k-mooney | bauzas: what i would prefer to do is add a privsep funciton in nova that just does what the current so brick funtion does | |
| 15:25:01 | bauzas | but I'm debating the alternatives, since I'm not at all a brick expert | |
| 15:25:07 | dansmith | sean-k-mooney: I'm very -1 on that | |
| 15:25:20 | bauzas | sean-k-mooney: sorry but I'm like dan | |
| 15:25:23 | sean-k-mooney | well i just dont like having to constuct rootwap calls | |
| 15:25:26 | sean-k-mooney | in nova | |
| 15:25:28 | bauzas | I don't want nova to do brick things | |
| 15:25:31 | dansmith | sean-k-mooney: when brick gets changed because multipathd changes, we'll never update our copy and we'll break | |
| 15:25:35 | dansmith | that's a terrible idea, IMHO | |
| 15:25:39 | sean-k-mooney | if there was a high level fonction that didnt need that it woudl be fine | |
| 15:25:44 | dansmith | bauzas: right, this is brick's domain, even if it's ugly | |
| 15:25:48 | bauzas | that's my point | |
| 15:25:55 | bauzas | I'm ok with calling brick | |
| 15:25:59 | sean-k-mooney | i dont have an issue using brick | |
| 15:26:04 | bauzas | I'm a bit torn with calling brick on a specific subdomain | |
| 15:26:22 | sean-k-mooney | i just dislike that we have to do the utils.get_root_helper() part | |
| 15:26:39 | dansmith | sean-k-mooney: oh you want to privsep wrap the brick call? that's different than what you suggested | |
| 15:26:49 | bauzas | sean-k-mooney: https://github.com/openstack/os-brick/blob/6e83ac6eeee8f3a4b3265a4e927dca1bc190e088/os_brick/initiator/connectors/iscsi.py#L523 | |
| 15:27:00 | bauzas | we could just call supports_multipath | |
| 15:27:01 | sean-k-mooney | dansmith: no i want brick to update the fucntio to use prvisep internall | |
| 15:27:04 | sean-k-mooney | and we just call it | |
| 15:27:12 | bauzas | that's what I wrote in my comment | |
| 15:27:22 | dansmith | bauzas: that's a connector object though | |
| 15:27:31 | bauzas | instead of using what looks to me an internal brick API | |
| 15:27:59 | bauzas | dansmith: doesn't it doing the same thing ? | |
| 15:28:01 | sean-k-mooney | bauzas that need us to constuct a connector which whil we could proably fake normlay need connection info form cinder | |
| 15:28:02 | dansmith | sean-k-mooney: okay that's fine, but.. don't we call brick things all over the place? are they all wrapped in privsep? | |
| 15:28:12 | bauzas | ah | |
| 15:28:14 | sean-k-mooney | dansmith: no | |
| 15:28:15 | dansmith | bauzas: we don't have a connector object though.. and we wouldn't here | |
| 15:28:30 | dansmith | sean-k-mooney: which no? :) | |
| 15:28:35 | sean-k-mooney | i was punting on that i just wanted a commnt to be added to know we need to clean this up in the future | |
| 15:28:41 | bauzas | I wish we had then a more abstract API to consume | |
| 15:29:04 | bauzas | but I'm cool with merging it as it is, provided we document it may break :) | |
| 15:29:05 | dansmith | sean-k-mooney: comment works :) | |
| 15:29:14 | bauzas | heh, jinxed | |
| 15:29:20 | sean-k-mooney | i was ok if not supper happy with multipath_running = linuxscsi.LinuxSCSI.is_multipath_running ... | |
| 15:29:36 | sean-k-mooney | a comment woudl be nice i just didnt want to ask for a respin for that | |
| 15:29:39 | dansmith | bauzas: I don't think that's a private brick api | |
| 15:29:41 | sean-k-mooney | on its own | |
| 15:30:03 | dansmith | bauzas: it's underscored in the connector usage, but it's api is not underscored | |
| 15:30:17 | bauzas | you're right | |
| 15:30:44 | bauzas | anyway, I'm cool with shipping as it is, hence my +1 but I'd appreciate if we could comment out that call | |
| 15:30:49 | bauzas | comment s/out// | |
| 15:31:31 | bauzas | (english and all your adverbs) | |
| 15:35:13 | sean-k-mooney | tkajinam: so tl;dr dansmith was right and if you adress there intial comemnts i think we are all ok with your patch | |
| 15:35:14 | dansmith | sean-k-mooney: just to circle back.. I'm not sure InvalidConfiguration should really inherit from Invalid, but raising Invalid itself defaults to "400 Bad Request" because that's really for HTTP Invalid.. I know he's overriding the message, | |
| 15:35:22 | dansmith | but it's just not what Invalid should be used for, IMHO | |
| 15:36:21 | sean-k-mooney | your right i was orgianly considerign if it shoudl be a RuntimeError but decied not to suggest that | |
| 15:36:40 | sean-k-mooney | partly because i was nto sure RuntimeError woudl abort startup | |