| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-31 | |||
| 13:42:47 | auniyal | https://paste.opendev.org/show/bRTqAnAkcUd9XQEhLiqP/ | |
| 13:43:30 | dansmith | please don't do that | |
| 13:43:38 | bauzas | auniyal: as dansmith said, this is wrong, you're also deleting the original object | |
| 13:43:47 | auniyal | no it didn't other failed | |
| 13:43:55 | auniyal | ack | |
| 13:46:08 | auniyal | bauzas, dansmith and return the list ? | |
| 13:46:34 | bauzas | so, I'm wrapping my head for a simple pattern | |
| 13:47:06 | bauzas | you have many possibilities : | |
| 13:47:49 | bauzas | 1/ iterate over the list, mark the ones to delete, delete them at once in a comprehension list | |
| 13:48:25 | bauzas | 2/ generate an empty object list, iterate over the list, add every single bdm that's correct | |
| 13:48:40 | bauzas | I'm in favor of 1/ | |
| 13:48:46 | dansmith | me too | |
| 13:49:12 | bauzas | 3/ you can use python's filter() with a lambda method | |
| 13:49:16 | dansmith | and I'll say, you don't even need to do the comprehension list part, | |
| 13:49:17 | bauzas | :) | |
| 13:49:26 | dansmith | just make the list of ones to delete, then go back and delete them | |
| 13:49:41 | bauzas | that too | |
| 13:49:53 | dansmith | which I believe is what he already had before we startedin on this | |
| 13:51:30 | auniyal | 1 - iterate over the list; delete(bdm) if culprit else empty_list.append(bdm); return empty_list ?? | |
| 13:52:03 | auniyal | dansmith, so delete part is not happening, | |
| 13:52:54 | auniyal | thats I am doing here - https://review.opendev.org/c/openstack/nova/+/882284/53/nova/compute/manager.py | |
| 13:52:54 | dansmith | no, don't return an empty list of course | |
| 13:53:31 | dansmith | auniyal: just return nothing, as was said there would be fine | |
| 13:53:56 | dansmith | to "make it clear that we're modifying in place". I don't think it's important to do that but if that matters to someone else, then fine | |
| 13:54:39 | dansmith | "for now i would prefer if you just drop the return value and we can adress this later." | |
| 13:54:42 | dansmith | that's all you have to do | |
| 13:54:53 | auniyal | ack, thanks dansmith | |
| 13:55:00 | bauzas | in other words, step back to PS5x-something | |
| 13:55:09 | dansmith | *facepalm* yeah | |
| 13:55:21 | bauzas | actually, not | |
| 13:55:45 | bauzas | I stupidely gave you the wrong IndexError pattern which is to drop objects in a loop | |
| 13:56:00 | dansmith | just roll from here, delete the return value and it's good right? | |
| 13:56:14 | bauzas | I'm lost between versions tbh, but yeah | |
| 14:00:52 | sean-k-mooney | delete the return and let it update in palce as it was and its fine | |
| 14:02:33 | sean-k-mooney | its not quite the same as the old patchset as that was missing assert in the unit tests i think | |
| 14:02:39 | auniyal | yeah it passing all required tests | |
| 14:02:49 | auniyal | yes, I updated tests | |
| 14:03:40 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 14:05:28 | sean-k-mooney | auniyal: you forgot the doc string | |
| 14:05:35 | sean-k-mooney | returns: BlockDeviceMappingList list object. | |
| 14:05:52 | auniyal | oh | |
| 14:07:18 | auniyal | sean-k-mooney, can you please here as well https://review.opendev.org/c/openstack/nova/+/882284/53..54/doc/source/admin/manage-volumes.rst | |
| 14:07:24 | auniyal | if its fixed as per comments | |
| 14:07:45 | sean-k-mooney | yep i already reviewd it and marked it doen | |
| 14:07:55 | auniyal | ack, thanks | |
| 14:08:05 | sean-k-mooney | the doc string is all that left so if you fix that ill add a +2 | |
| 14:08:09 | opendevreview | John Garbutt proposed openstack/nova master: Add nova-manage ironic-compute-node-move https://review.opendev.org/c/openstack/nova/+/886989 | |
| 14:08:09 | opendevreview | John Garbutt proposed openstack/nova master: Make compute node rebalance safter https://review.opendev.org/c/openstack/nova/+/887151 | |
| 14:09:21 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 14:13:47 | sean-k-mooney | dansmith: bauzas: is there any reason to not +w the first partch in the ironic shard key serise. i.e. the rename/deprecaton of peer_list | |
| 14:14:13 | bauzas | I was leaving this to the core who was +2/+W the top one | |
| 14:14:24 | johnthetubaguy | bauzas: thank you for all the reviews on that sharding chain (and after having turned the spell checker back on in my editor...) I have refreshed all those patches now. | |
| 14:15:03 | bauzas | johnthetubaguy: 've seen that, I'm now on the unified-limits nova-manage series, but I'll give yours a look before I stop | |
| 14:15:13 | sean-k-mooney | ok i was going to +w to get it moving though ci since it looks fine and both you and dan +2'd it | |
| 14:15:15 | johnthetubaguy | sean-k-mooney: I think JayF was wanting to make sure the rest of series looked plauable first. Hopefully that is the case now. | |
| 14:15:34 | sean-k-mooney | no worries we can leave it there until the rest is reviewd | |
| 14:15:40 | JayF | yeah I do not want us to deprecate peer list until we're sure sharding is going to land | |
| 14:15:40 | bauzas | sean-k-mooney: oh sorry, read too fast, I thought you were referring to amit's series | |
| 14:15:48 | sean-k-mooney | but i think we coudl merge that in this cycle even if the rest was not merged | |
| 14:15:51 | bauzas | and yeah what johnthetubaguy said | |
| 14:15:51 | JayF | do not want to have to put our operators through two migrations on how nova-computes work :) | |
| 14:16:02 | johnthetubaguy | bauzas: awesome, thank you. | |
| 14:16:21 | bauzas | JayF: I was surprised we didn't catch the ironic microversion bit, was it working when you tested ? | |
| 14:16:39 | sean-k-mooney | JayF: deprecating does not force them to migrate | |
| 14:16:40 | JayF | I tested it with my lessee patch which also raised the microversion | |
| 14:16:43 | JayF | sean-k-mooney: that's a good point | |
| 14:16:48 | bauzas | JayF: ahah ! | |
| 14:16:57 | JayF | bauzas: I am planning on dedicating a day or two during RC to testing this, migrating nodes around, etc | |
| 14:17:00 | sean-k-mooney | we will have the option until the 2024.2 release at a minium | |
| 14:17:06 | JayF | bauzas: just always forget how hectic this week in with my Ironic hat on :) | |
| 14:17:20 | JayF | s/in/is/ | |
| 14:17:22 | dansmith | oh I just +Wd it.. | |
| 14:17:25 | bauzas | JayF: I don't see at all what you mean :p | |
| 14:17:31 | dansmith | I thought it was critical to get the deprecation into place this cycle, | |
| 14:17:37 | dansmith | I haven't looked at the patches above | |
| 14:17:58 | bauzas | the two next ones are quite easy to review | |
| 14:18:08 | bauzas | the last one (the reshuffle) is the hardest | |
| 14:18:08 | sean-k-mooney | well i would like to do it this cycle but with new skip level porcess it actully has less of an impact | |
| 14:19:00 | johnthetubaguy | dansmith: FWIWI, that was my take too, tell people its broken, do active-passive if you can, keep using the old way if you must. | |
| 14:19:09 | dansmith | yeah | |
| 14:19:18 | bauzas | I thought we agreed on that even in the spec | |
| 14:19:37 | johnthetubaguy | (It might have just been in PTG chat we said that) | |
| 14:19:45 | bauzas | yeah | |
| 14:19:52 | sean-k-mooney | that why i was onderign why it had 2 +2s and no +w and why i ask | |
| 14:20:15 | sean-k-mooney | anyway im going to review the next two patches now before my next meeting | |
| 14:20:16 | dansmith | sean-k-mooney: I just forgot to hit the +W | |
| 14:20:16 | JayF | Probably because of my request, but you all are right that a deprecation won't force operator action immediately. | |
| 14:20:31 | opendevreview | Amit Uniyal proposed openstack/nova master: Delete dangling bdms https://review.opendev.org/c/openstack/nova/+/882284 | |
| 14:20:42 | johnthetubaguy | JayF: we can always tweak the wording in the release note if we don't land the second half this cycle | |
| 14:20:44 | sean-k-mooney | johnthetubaguy: JayF everythign is in place for the shards on the ironic side yes? | |
| 14:21:09 | JayF | sean-k-mooney: very yes :) We released it last cycle, specifically ahead of any of the nova work :D | |
| 14:21:11 | sean-k-mooney | im assuming so since these do not have depends on | |
| 14:21:16 | sean-k-mooney | cool | |
| 14:21:18 | johnthetubaguy | I think so, the API has been shipped. | |
| 14:21:52 | JayF | johnthetubaguy: if you wanted to test this yourself in devstack; I recently updated our doc here to have a plug+play devstack config (our old doc had bitrotted) https://docs.openstack.org/ironic/latest/contributor/devstack-guide.html#ironic-with-nova this is the config I'll start from when I play with it in RC | |
| 14:22:10 | auniyal | bauzas, it was not failing by default - missed one test :-( | |
| 14:22:15 | auniyal | its done now | |
| 14:22:35 | johnthetubaguy | JayF: I was trying to get a minion to do that, but I failed, that looks handy though :) | |
| 14:22:53 | JayF | johnthetubaguy: well, my goal was to make that doc minion-friendly ;) | |
| 14:23:30 | sean-k-mooney | JayF this is goinng to create 3 ironic vms on the host this is run on right | |