opendevreview | Takashi Natsume proposed openstack/placement master: Move implemented specs for Xena and Yoga release https://review.opendev.org/c/openstack/placement/+/853730 | 00:08 |
---|---|---|
opendevreview | Merged openstack/nova master: Unify placement client singleton implementations https://review.opendev.org/c/openstack/nova/+/852900 | 02:48 |
opendevreview | Merged openstack/nova master: Avoid n-cond startup abort for keystone failures https://review.opendev.org/c/openstack/nova/+/852901 | 02:48 |
opendevreview | Merged openstack/nova master: scheduler: Add an ephemeral encryption pre filter https://review.opendev.org/c/openstack/nova/+/760456 | 03:47 |
opendevreview | Merged openstack/nova master: Test attached volume extend actions in the nova-next job https://review.opendev.org/c/openstack/nova/+/843700 | 08:02 |
sean-k-mooney | by the way i dont know if i have explained how im using Review-Prioity +1 and +2 but im adding my +2 if im going to priotities it and i would like others too also and im adding +1 when i am going to review it and it woudl be nice if other reviewed it but im not asking other cores to go out of there way to prioritise it i.e. its a nice to have | 09:25 |
sean-k-mooney | im also not alwasy setting it, im just setting it on patches that are either imporant or have not reablly been reviewed in a while to highlight them to others | 09:26 |
sean-k-mooney | for example i dont think this backport is critical https://review.opendev.org/c/openstack/nova/+/821349 but it would be nice to land sooner rather then later so RP +1 | 09:27 |
sean-k-mooney | stephenfin: by the way im currently working on the final patch in the vdpa seriese would you have time to review the first 3. i just need to add a release note and update the docs in the last patch and its also done | 09:29 |
stephenfin | sean-k-mooney: sure (y) | 09:43 |
sean-k-mooney | stephenfin: just pushing the last patch now once i fix the commti message so it should be ready by the time you get to it | 09:44 |
opendevreview | sean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate https://review.opendev.org/c/openstack/nova/+/853704 | 09:46 |
sean-k-mooney | gibi: ^ that should be ready for your review too. if ye have comments ill respin them collectinvly once ye have complete reviewing the whole sereise later today | 09:47 |
gibi | sean-k-mooney: ack, I will do a review round on it today | 09:49 |
sean-k-mooney | oh i have to fix 1 thing in the last patch for the compute service bump so ill respin that quickly | 09:50 |
* sean-k-mooney forgot to update the unit test | 09:50 | |
gibi | (today I spent hours in a rabbit hole spreading provider mapping around the claim code, but now I'm backing out of it as it is a dead end) | 09:50 |
opendevreview | sean mooney proposed openstack/nova master: Add VDPA support for suspend and livemigrate https://review.opendev.org/c/openstack/nova/+/853704 | 09:54 |
sean-k-mooney | i think you should just need to pass them to filter_pools | 09:54 |
sean-k-mooney | i.e. if you have the provider uuid in the pool filter pools can jsut filter to the ones it the alloctiaons | 09:55 |
sean-k-mooney | although that is proably over simplfying | 09:55 |
sean-k-mooney | i woudl be tempted to extend the pci request object to carry the RP infor that it shoudl be fullfiled form | 09:56 |
sean-k-mooney | and then use that latter when we are doing the filtering /caliming | 09:56 |
sean-k-mooney | gibi: would something like ^ work better | 09:57 |
sean-k-mooney | that woudl avoid chaning the sigurtures of any of the fuctions | 09:57 |
gibi | yeah I'm on this track | 09:57 |
gibi | the filter_pools needs it | 09:57 |
gibi | and we call that from 3 different places | 09:58 |
sean-k-mooney | but you would have to update teh request objecject before calling support/consume/apply | 09:58 |
gibi | 1) during scheduling (there we hace an allocation candidate to work with) | 09:58 |
gibi | 2) during claim (there we have the request spec to work with probably) | 09:58 |
gibi | 3) during consume (that is a big rabbit hole :D) | 09:58 |
gibi | but you are right | 09:58 |
gibi | the InstancePCIRequest could carry the PR uuid | 09:59 |
gibi | _after_ the scheduler made the allocation in placement | 09:59 |
gibi | as that is then fixed | 09:59 |
sean-k-mooney | well if save the updated request object after 1 when we claim the allcoation candiate then 2 and 3 can just read it from there | 09:59 |
sean-k-mooney | yep | 09:59 |
gibi | yes | 09:59 |
sean-k-mooney | so i think that will work out cleanly in the end | 09:59 |
gibi | we did something similar for QoS with the parent_ifname tag in the InstancePCIRequest | 10:00 |
sean-k-mooney | ah yes that is indeed similar | 10:00 |
sean-k-mooney | as that narrowed the set of pools to consider | 10:00 |
gibi | the RP uuid is actually cleaner | 10:00 |
opendevreview | Merged openstack/nova stable/xena: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/853218 | 10:01 |
gibi | so eventually we can refactor the QoS path to use that too | 10:01 |
sean-k-mooney | yep | 10:01 |
sean-k-mooney | once we start populating the rp uuid in the pci device tabel and pci_stat pools | 10:01 |
sean-k-mooney | for the pci devices are you going to add a new column for the rp_uuid | 10:02 |
sean-k-mooney | or just put it in the extra info column | 10:02 |
sean-k-mooney | i woudl be tempted to do the former | 10:02 |
sean-k-mooney | for the pools i would just put it in the json blob | 10:02 |
gibi | at the moment I don't see where I need the rp_uuid from the pci device, when I will see that I can consider this | 10:03 |
gibi | for the pools it is probably the blob | 10:03 |
sean-k-mooney | ya so i dont think we will evern need to do an sql query on the pools | 10:03 |
sean-k-mooney | that will alway be processed in python | 10:03 |
gibi | I thinks so too | 10:03 |
sean-k-mooney | but the pci devices it would be nice to have them correalated with the placment rp | 10:03 |
sean-k-mooney | so it would be nice to put it at least in extra_info blob | 10:04 |
sean-k-mooney | but you possibely coudl move the fitlering to sql | 10:04 |
sean-k-mooney | i.e. have filter pools intially get the candiate devcie with an sql query | 10:05 |
sean-k-mooney | alhtogh we have the pools in memory so i dont think that actully bys you much | 10:05 |
sean-k-mooney | so really i would jsut like it in the pci device tabel fro debugging | 10:05 |
sean-k-mooney | so extra info would be fine for that | 10:05 |
gibi | yeah without the rp_uuid there you have to look up the pci address from the dev and query placement by RP name | 10:10 |
gibi | I will see how this fits together but it probably make sense to add rp_uuid to the dev | 10:10 |
sean-k-mooney | ya this is a minor convince feature so we can see where it fits when you get to it | 10:12 |
sean-k-mooney | we lived without trackign the neutron port uuid in the requster_id for years | 10:12 |
sean-k-mooney | but due to recent evnets the more info we have for debuging things like this the happier i will be | 10:13 |
opendevreview | Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682 | 10:14 |
amorin | sean-k-mooney ^ I wrote both unit + functional tests | 10:16 |
sean-k-mooney | thanks ill review it shortly | 10:17 |
amorin | I also patch one of your tests to make it work | 10:17 |
amorin | no hurries, thanks! | 10:17 |
* amorin will be afk for 1h | 10:17 | |
sean-k-mooney | ah yes test_shelve for remote managed ports | 10:18 |
opendevreview | Merged openstack/nova stable/xena: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/853219 | 10:52 |
gibi | sean-k-mooney: I did a review round on the vdpa series, the last patch needs some func test coverage for live migration but other than that I'm happy | 10:56 |
* gibi disappears for lunch | 10:57 | |
tobias-urdin | from what I understand nova's policy code restricts POST /os-external-server-events requests to only be allowed from the admin endpoint, how is that enforced? referring to issue mentioned in https://bugzilla.redhat.com/show_bug.cgi?id=1640443 | 11:02 |
tobias-urdin | i'm trying to understand if that change is still required and valid or if that fix can be reverted | 11:02 |
tobias-urdin | to using, for example, the internal endpoint instead | 11:03 |
sean-k-mooney | gibi: or right good point ill add that | 11:04 |
sean-k-mooney | tobias-urdin: the external events api is not only admin only but intended to be called only by other services | 11:05 |
sean-k-mooney | tobias-urdin: as in admin shoudl never call it directly | 11:05 |
sean-k-mooney | using the internal endpoint is fine you the calling service jsut need to use the admin clint | 11:06 |
sean-k-mooney | but old cinder did not do that properly | 11:06 |
sean-k-mooney | so if cinder have actully fixed there code to use the admin client to call the external events api instead of trying to use the user token then it can be revierted but the change in ooo was jsut a workaround for a bug in cinder | 11:07 |
sean-k-mooney | nova has never required /os-external-server-events to use the admin endpoint | 11:08 |
tobias-urdin | ah, the logic was enforced by cinder I see! that's why I couldn't figure it out, my bad | 11:08 |
tobias-urdin | thx | 11:08 |
sean-k-mooney | well not enforced they just had a bug | 11:08 |
tobias-urdin | ack | 11:09 |
sean-k-mooney | they tried to use teh users token that requsted the volume extend to send the external event | 11:09 |
sean-k-mooney | instead of using an admin token | 11:09 |
sean-k-mooney | by using the admin endpoint they bypassed the admin check | 11:09 |
sean-k-mooney | gibi: based on your questions regarding suspend and the compute service bump i realise i need a compute service bump for attach/detach. suspend can use the same one as migrate but attach/detach needs one too. so ill adress all your nit as part of that too | 11:23 |
stephenfin | sean-k-mooney: done | 11:40 |
sean-k-mooney | stephenfin: thanks ill respine them shortly | 11:48 |
gibi | sean-k-mooney: ack, good point | 11:55 |
sean-k-mooney | stephenfin: i really would prefer to use the asserts by the way | 11:59 |
stephenfin | You shouldn't though. You've already highlighted the risks of doing so | 12:00 |
sean-k-mooney | i dont want to check this at runtime but do want to prevent you breaking the tests with bad mocks/fixtures | 12:00 |
stephenfin | Then just raise a NovaException and don't handle it | 12:00 |
sean-k-mooney | but that is not the behaivor i want | 12:00 |
stephenfin | What's the difference? | 12:00 |
sean-k-mooney | or a pattern we shoudl really follow | 12:00 |
sean-k-mooney | that will cause the agent to restart potially | 12:00 |
stephenfin | so will an assert | 12:01 |
sean-k-mooney | well it wont because i know this is always set in real code | 12:01 |
stephenfin | then the exception won't do anything either | 12:01 |
sean-k-mooney | but what im trying to protect agaisnt is you create a unit test where you populate the object manually and fail to set the requried value | 12:02 |
sean-k-mooney | we already have assert in real code in nova | 12:02 |
sean-k-mooney | we dotn have many but they exists | 12:02 |
stephenfin | we do, but they really shouldn't be there and we shouldn't be adding to them | 12:03 |
stephenfin | I don't get what the issue with 'raise Exception(...)' is | 12:03 |
stephenfin | if we'll never see the AssertionError in real code then we'll never see the Exception | 12:03 |
sean-k-mooney | right but we pay the cost of checking | 12:04 |
sean-k-mooney | i can drop the check if you prefer but that is why assert exist | 12:04 |
sean-k-mooney | to help you debug and not pay any runtime cost in production code | 12:04 |
sean-k-mooney | those asserts didnt actully catch any issue in teh end but validated that that was not the issue | 12:05 |
sean-k-mooney | it wasnt the path that was wrong it was the key that was used for the path in the end | 12:05 |
stephenfin | Yeah, it's a band-aid for the lack of consistent type hinting in the code base, unfortunately | 12:06 |
stephenfin | but to answer your question, can we drop it so? | 12:06 |
sean-k-mooney | stephenfin: the fact that the dev_path is not a keyword arg is enough documentaiton for me that we require this arg | 12:06 |
sean-k-mooney | so ya ill drop them | 12:07 |
sean-k-mooney | can typing assert that something is not None by the way | 12:07 |
sean-k-mooney | or not None or '' in this case | 12:07 |
sean-k-mooney | i dont think so but it would be nice if it could | 12:07 |
stephenfin | def foo(bar: str) -> None: | 12:08 |
stephenfin | bar has to be a string. It can't be a bool, int, None or anything esle | 12:08 |
stephenfin | *else | 12:08 |
stephenfin | it won't help you with your tests though since we don't type check our tests | 12:08 |
sean-k-mooney | it can be '' | 12:08 |
stephenfin | yes, it could | 12:08 |
sean-k-mooney | ok ill drop them when i refactor | 12:09 |
stephenfin | ta | 12:09 |
sean-k-mooney | mind if i keep the ablity to disable optimiasation | 12:09 |
sean-k-mooney | ill add it to passargs | 12:09 |
sean-k-mooney | instead | 12:09 |
opendevreview | Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682 | 12:09 |
sean-k-mooney | then i can add asset when debuging and just turn it off on the command line | 12:09 |
sean-k-mooney | *passenv | 12:10 |
stephenfin | wfm | 12:10 |
sean-k-mooney | cool thanks for looking | 12:10 |
opendevreview | Arnaud Morin proposed openstack/nova master: Unbind port when offloading a shelved instance https://review.opendev.org/c/openstack/nova/+/853682 | 12:15 |
stephenfin | sean-k-mooney: regarding your earlier comments on +1 vs +2 review-priority, have you hovered over the +1 and +2 review-priority buttons? | 12:15 |
sean-k-mooney | ya | 12:15 |
sean-k-mooney | i know what the text says | 12:15 |
sean-k-mooney | the contibutor vs core promise thing is a bit weired to me | 12:16 |
stephenfin | gibi: This PCI in placement series is really well structured and pretty easy to review (especially for PCI code /o\). Nice work (y) | 12:16 |
stephenfin | oh, I read it as code | 12:16 |
stephenfin | I thought they were saying +1 means _someone_ will review it but not me | 12:17 |
stephenfin | *not necessarily me | 12:17 |
stephenfin | Oh well :) | 12:17 |
sean-k-mooney | so the idea was +1 can be set by anyone and they will review | 12:17 |
sean-k-mooney | an that is an indication that cores can use to perhaps also review it | 12:17 |
sean-k-mooney | and +2 is a commitmnet form the core reivew to review this | 12:18 |
gibi | I think +1 is not well defined for cores | 12:18 |
gibi | so sean-k-mooney you are free to use to for a "maybe" | 12:18 |
sean-k-mooney | sylvain wanted to use +1 as a way for non cores to singal to cores that something might be ready for review too | 12:19 |
sean-k-mooney | the text in my orgially patch was +1 is core review requested and +2 is core review approved but that was also problematic | 12:19 |
gibi | I think +1 for non-cores is the same as +2 for cores | 12:19 |
gibi | both is a promise | 12:19 |
sean-k-mooney | yep | 12:19 |
gibi | that I, who set it, will review the patch | 12:20 |
sean-k-mooney | im using +1 as im going to review this but not nessiarly ping other to review it | 12:20 |
sean-k-mooney | vs +2 ill review it and when im going to give my +2 ill ping others to review it too | 12:20 |
gibi | that is OK to me | 12:20 |
sean-k-mooney | i.e. i not only commit to reviewing but i also care about this not waiting for every | 12:21 |
sean-k-mooney | kindo fo like feature-liason lite | 12:21 |
gibi | stephenfin: I needed the small step in the PCI work for myself too to see what is missing :) The inventory part is self contained mostly in the new translator. The scheduling part will be less easy to read (once I write it :D) | 12:21 |
gibi | sean-k-mooney: that make sense | 12:21 |
stephenfin | gibi: I got as far as https://review.opendev.org/c/openstack/nova/+/851358 +2 on everything I think | 12:24 |
* stephenfin lunches | 12:24 | |
gibi | stephenfin: thank you, have a good one | 12:24 |
sean-k-mooney | my plan for there rest of the day is finish the vdpa seriese, review the pci serise and if melwitt has updated the encyption series review that. my plan for next week assuming vdpa is done is 100% upstream review so please ping as needed | 12:27 |
gibi | sean-k-mooney: ack. I will do another vdpa round today if needed | 12:29 |
gibi | stephenfin: thanks for noticing the TODO in https://review.opendev.org/c/openstack/nova/+/851358 I forgot it. Actually the patches above that are also ready until https://review.opendev.org/c/openstack/nova/+/850468 | 12:30 |
opendevreview | Amit Uniyal proposed openstack/nova stable/wallaby: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/853811 | 12:31 |
opendevreview | Amit Uniyal proposed openstack/nova stable/wallaby: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/853812 | 12:31 |
gibi | but it is more than fair to stop there now | 12:31 |
stephenfin | ack, I'll keep going with it this afternoon so. If you could cobble together a follow-up I can finish that | 12:32 |
gibi | stephenfin: I will hold off with the follow up until sean-k-mooney reviews it | 12:38 |
opendevreview | Amit Uniyal proposed openstack/nova master: Adds check for VM snapshot fail while quiesce https://review.opendev.org/c/openstack/nova/+/852171 | 12:42 |
*** tosky is now known as Guest532 | 12:51 | |
*** tosky_ is now known as tosky | 12:51 | |
*** tbachman_ is now known as tbachman | 13:10 | |
opendevreview | Merged openstack/nova master: Add reno for fixing bug 1941005 https://review.opendev.org/c/openstack/nova/+/853265 | 13:38 |
*** dasm|off is now known as dasm | 13:59 | |
JayF | Good morning; thanks for the reviews I've already been getting. I do have a couple of other PRs open in Gerrit I'd appreciate reviews on that are ready to go: https://review.opendev.org/c/openstack/nova/+/853529 and https://review.opendev.org/c/openstack/nova/+/853540 - thanks in advance. | 14:54 |
opendevreview | Merged openstack/nova stable/wallaby: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821349 | 14:58 |
opendevreview | Jay Faulkner proposed openstack/nova stable/ussuri: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821351 | 14:59 |
JayF | I need to re-learn my alphabet, apparently. Rebased the wrong one lol | 14:59 |
opendevreview | Jay Faulkner proposed openstack/nova stable/victoria: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/821350 | 15:00 |
opendevreview | Merged openstack/nova master: block_device: Add DriverImageBlockDevice to block_device_info https://review.opendev.org/c/openstack/nova/+/826527 | 15:34 |
gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Dropped 1 device(s) as they are on the wrong NUMA node(s) | 15:40 |
gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Dropped 1 device(s) that are not part of the placement allocation | 15:40 |
gibi | 2022-08-19 17:35:22,086 DEBUG [nova.pci.stats] Not enough PCI devices left to satisfy request | 15:40 |
gibi | ... and the NUMATopologyFilter now works with placement allocation candidates \o/ | 15:40 |
sean-k-mooney | nice | 15:40 |
gibi | there are some raw edges but the general idea seems to work | 15:41 |
gibi | now I have like 10 WIP commits locally to clean up :D | 15:42 |
sean-k-mooney | ack im sure we can flesh that out via review and or cleanups | 15:42 |
opendevreview | Merged openstack/nova master: block_device: Add encryption attributes to image and ephemeral disks https://review.opendev.org/c/openstack/nova/+/826528 | 16:23 |
opendevreview | melanie witt proposed openstack/nova master: Follow up changes for ephemeral encryption https://review.opendev.org/c/openstack/nova/+/853254 | 17:50 |
opendevreview | Balazs Gibizer proposed openstack/nova master: DNM: why I cannot set request_id on InstancePCIRequiest https://review.opendev.org/c/openstack/nova/+/853835 | 18:23 |
gibi | sean-k-mooney: if you are still around ^^ I totally don't get this | 18:24 |
sean-k-mooney | i dont think you want to set request_id you want to set requester_id | 18:30 |
sean-k-mooney | ill take a look quickly but we set this in teh neutorn module somewhere i think | 18:31 |
sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/objects/instance_pci_requests.py#L43-L44 | 18:33 |
sean-k-mooney | but actully it depens on what you want to track ther | 18:33 |
sean-k-mooney | are you tryign to add the placment request group or resouce provider there | 18:33 |
gibi | I need a unique id | 18:33 |
sean-k-mooney | well the request_id should be unique | 18:34 |
gibi | for neutron based InstancePCIRequests we generate a uuid for request_id | 18:34 |
gibi | I try to do the same for the flavor based requests | 18:34 |
sean-k-mooney | yes and we set requester_id to the neutorn port uuid | 18:34 |
sean-k-mooney | ah ok | 18:34 |
sean-k-mooney | am that should be posible | 18:35 |
sean-k-mooney | what error do you get | 18:35 |
gibi | https://github.com/openstack/nova/blob/master/nova/network/neutron.py#L2390 | 18:35 |
gibi | the test fails as the domain has no PCI device | 18:36 |
sean-k-mooney | i would guess this is a bug in our fixture | 18:36 |
gibi | https://paste.opendev.org/show/bK1kbEypC2EUHXtL2ruT/ | 18:36 |
sean-k-mooney | let me grab that patch and run it locally | 18:36 |
gibi | anyhow my brain is toasted and my wife just arrived so I have to log off. don't think too much about this issue it is late friday anyhow | 18:37 |
gibi | see you all on Monday | 18:37 |
sean-k-mooney | ok | 18:37 |
sean-k-mooney | im going to call it a day too | 18:37 |
gibi | have a nice weekend | 18:38 |
sean-k-mooney | you too ill try and look at this on monday after i rebase the vdpa patches | 18:38 |
opendevreview | Dan Smith proposed openstack/nova-specs master: WIP: Robustify Compute Node Hostnames https://review.opendev.org/c/openstack/nova-specs/+/853837 | 18:58 |
dansmith | sean-k-mooney: artom: ^ | 18:58 |
dansmith | that's a big chunk of work, which we may never do, but I thought it was probably good to document some of the things we could/should do to make this better | 18:59 |
dansmith | either to point to and say "see, too big, never going to happen" or the opposite | 18:59 |
dansmith | I've been thinking about the first work item for a long time and I think we should probably do that for safety even if we don't do any of the rest of it | 18:59 |
artom | That's kind of in the same vein as https://bugzilla.redhat.com/show_bug.cgi?id=1965419, which came up before when another customer renamed their hosts | 19:02 |
artom | Now that I think about it, it may have been that exact same KCS | 19:02 |
artom | Because it was a 10 -> 13 FFU | 19:02 |
artom | Sorry, leaking downstream here | 19:02 |
sean-k-mooney | i mean we were broken in 16.1 requirenign neutron ot add a new config option | 19:04 |
sean-k-mooney | https://bugzilla.redhat.com/show_bug.cgi?id=1900500 | 19:04 |
sean-k-mooney | resource_provider_default_hypervisor | 19:04 |
sean-k-mooney | https://github.com/openstack/neutron/commit/577217c52d677ba35ca78b87c06302d506f66ff9 and https://github.com/openstack/neutron/commit/ddf0fef28b7095724c8ba27f3275d0dad2252251 | 19:06 |
sean-k-mooney | were added to neutorn to work aorund changes in ooo | 19:06 |
opendevreview | Dan Smith proposed openstack/nova-specs master: WIP: Robustify Compute Node Hostnames https://review.opendev.org/c/openstack/nova-specs/+/853837 | 19:30 |
*** dasm is now known as dasm|off | 21:33 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!