| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-31 | |||
| 13:31:16 | opendevreview | John Garbutt proposed openstack/nova master: Limit nodes by ironic shard key https://review.opendev.org/c/openstack/nova/+/886980 | |
| 13:35:13 | bauzas | dansmith: could you save me a couple of mins ? do you remember top of your head which helper method in o.vo can do a deep copy of a list of objects ? | |
| 13:35:28 | bauzas | copy.deepcopy() can't work correctly | |
| 13:36:09 | bauzas | and obj_clone() is IIRC only for single objects | |
| 13:36:11 | bauzas | but I can try | |
| 13:38:14 | dansmith | bauzas: um, not sure there is anything other than obj_clone | |
| 13:38:24 | bauzas | then that's weird | |
| 13:39:16 | dansmith | you could obj_from_primitive(obj_to_primitive()) | |
| 13:39:16 | bauzas | https://paste.opendev.org/show/b1nUZLuwjL1jJ0eFsTA5/ | |
| 13:39:40 | bauzas | so basically the containers aren't identical | |
| 13:40:00 | dansmith | well, if it made a copy of the objects inside then bdm1 is not in the copy list by definition | |
| 13:40:15 | bauzas | damn, right | |
| 13:40:37 | bauzas | it's a deepcopy, the reference is different | |
| 13:41:33 | dansmith | I'll say again, I really don't think there's anything wrong with modifying the list in place | |
| 13:41:40 | bauzas | auniyal: you know what ? instead of removing items from a copied list, just add items into a new empty list at start | |
| 13:41:42 | auniyal | copy.copy worked | |
| 13:41:43 | dansmith | in fact it seems *more* right to me to do so | |
| 13:41:57 | dansmith | please don't copy.copy() an object | |
| 13:42:06 | auniyal | enpty list - not of BlockDeviceMapping | |
| 13:42:10 | auniyal | is it okay | |
| 13:42:46 | auniyal | this works | |
| 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. | |