| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-13 | |||
| 17:13:52 | bauzas | JayF: so you basically wanna revert the whole series ? | |
| 17:13:53 | JayF | but that is a very rough line | |
| 17:14:11 | sean-k-mooney | lets not do that just yet | |
| 17:14:13 | JayF | bauzas: I think that's what we have to do. We'd need an SDK change, and there's a sniff of a bug in the Ironic API handling too, which is what pushed me to revert | |
| 17:14:23 | sean-k-mooney | if the specific call we need are functional then i woudl not revert | |
| 17:14:36 | bauzas | sean-k-mooney: we are on the edge of RC1 | |
| 17:14:47 | JayF | the entire basis of ironic/nova node sharding is that we have the ability to query nodes limited by shard | |
| 17:14:51 | bauzas | and we highlighted to the marketing about the ironic features | |
| 17:14:54 | JayF | on ironic side we index on shard to make that a fast query | |
| 17:15:10 | JayF | and so if SDK can't add that shard query, doing late filtering or anything like that basically guts the purpose of the change | |
| 17:16:05 | dansmith | if there's any question, we should revert for sure | |
| 17:16:07 | sean-k-mooney | ok its using the sdk | |
| 17:16:09 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/ironic/driver.py#L616 | |
| 17:16:23 | JayF | Yeah I've been on a zoom w/ johnthetubaguy troubleshooting this since like 7am local time, 3 hours | |
| 17:16:25 | JayF | and julia in here too | |
| 17:16:29 | dansmith | we should leave the deprecation in place but we probably need to adjust the text that says to use the other thing | |
| 17:16:46 | JayF | dansmith: please please lets not remove peer_list until shard lands though :/ | |
| 17:16:49 | bauzas | dansmith: jay is even proposing to revert the whole stack | |
| 17:17:05 | JayF | bauzas: my care about peer_list is this: we need a single slurp release with peer_list + sharding for migration | |
| 17:17:06 | sean-k-mooney | we are not removing peer_list until D | |
| 17:17:08 | dansmith | bauzas: right, which is probably best | |
| 17:17:13 | JayF | I have no concerns with deprecating peer_list now, but if we remove it in D | |
| 17:17:19 | JayF | then SLURP users don't get a migratory release | |
| 17:17:40 | sean-k-mooney | yes they do the migratiory release is C | |
| 17:17:58 | bauzas | okay, then I'm preparing the reverts | |
| 17:18:08 | sean-k-mooney | it will be deprected in B and C and then removed in D C will have shard supprot and peer_list support | |
| 17:18:25 | JayF | bauzas: I'm extremely sorry for this, we should've had devstack testing lined up as a prereq | |
| 17:19:14 | bauzas | JayF: this is not about being sorry, I prefer we found the bug before we deliver | |
| 17:19:22 | bauzas | so thanks for having spotted it | |
| 17:19:24 | sean-k-mooney | i would prefer to just using update the docs hostely rather then revert | |
| 17:19:34 | JayF | bauzas: that's why I made darn sure I was hooking up devstack before release time | |
| 17:19:57 | dansmith | nova should have expected a devstack job showing it working before merging, so the fault lies with us all | |
| 17:20:16 | sean-k-mooney | dansmith: also true ... | |
| 17:20:27 | JayF | dansmith: I literally just said that on the zoom call I'm still on | |
| 17:20:57 | dansmith | yup, I didn't review the later patches so I didn't even know we were missing that | |
| 17:21:18 | dansmith | but this is a good demonstration of why we should (and usually do) require such things | |
| 17:21:29 | bauzas | agreed | |
| 17:21:32 | dansmith | at least we found it before release | |
| 17:22:16 | sean-k-mooney | JayF: i assume laning the requried change in sdk was aslo rejected right | |
| 17:22:23 | JayF | it's already landed in master | |
| 17:22:31 | JayF | but we saw concerning behavior from the /v1/nodes?shard=blah Ironic API endpoing | |
| 17:22:37 | JayF | which is why I hit the revert button | |
| 17:22:47 | sean-k-mooney | ok but it has not release yet right | |
| 17:22:53 | JayF | I want it to land; but I don't want to break people and my confidence in this feature functioning is extremely low | |
| 17:22:57 | JayF | sean-k-mooney: That's been out since A | |
| 17:23:06 | JayF | sean-k-mooney: so I likely need to validate that broken behavior and fix it | |
| 17:23:25 | JayF | sean-k-mooney: or it's possible it's not broken; but I need to check that and I don't think RC is the time to be answering questions like that | |
| 17:24:23 | sean-k-mooney | ack | |
| 17:24:29 | dansmith | indeed. | |
| 17:24:41 | sean-k-mooney | it sound like there are enough things in flight that we wont knwo for a while :( | |
| 17:25:21 | opendevreview | Sylvain Bauza proposed openstack/nova master: Revert "Make compute node rebalance safter" https://review.opendev.org/c/openstack/nova/+/894944 | |
| 17:25:22 | opendevreview | Sylvain Bauza proposed openstack/nova master: Revert "Limit nodes by ironic shard key" https://review.opendev.org/c/openstack/nova/+/894946 | |
| 17:25:22 | opendevreview | Sylvain Bauza proposed openstack/nova master: Revert "Add nova-manage ironic-compute-node-move" https://review.opendev.org/c/openstack/nova/+/894945 | |
| 17:25:23 | opendevreview | Sylvain Bauza proposed openstack/nova master: Revert "Deprecate ironic.peer_list" https://review.opendev.org/c/openstack/nova/+/894947 | |
| 17:25:49 | dansmith | bauzas: we need to leave the deprecation in place, are you going to do a new one to deprecate with different wording? | |
| 17:26:35 | bauzas | dansmith: I thought JayF said "unplug the whole stack since we don't want to deprecate until shards exist" | |
| 17:26:54 | dansmith | bauzas: no, we should deprecate now, but not remove until shards is in place | |
| 17:27:02 | JayF | My opinion on deprecation is null; my opinion on *removal* is "after there has been at least one SLURP with shard+peer_list in a release together" | |
| 17:27:21 | dansmith | yup | |
| 17:27:48 | sean-k-mooney | +1 | |
| 17:27:50 | bauzas | o ok | |
| 17:27:55 | dansmith | bauzas: good with ninja approvals on the reverts right? | |
| 17:28:11 | sean-k-mooney | i mean i can also just hit them | |
| 17:28:11 | bauzas | dansmith: I think it's even documented in our docs | |
| 17:28:24 | sean-k-mooney | bauzas: it is but not really for this | |
| 17:28:34 | sean-k-mooney | the fast revert poicly does allow it however | |
| 17:28:40 | bauzas | well, this is a broken piece | |
| 17:28:50 | bauzas | fast reverts apply to this | |
| 17:29:02 | dansmith | bauzas: yep, just confirming.. did the first three | |
| 17:29:08 | bauzas | so | |
| 17:29:18 | bauzas | I need to look at the deprecation patch | |
| 17:29:21 | bauzas | to see the wordings | |
| 17:30:16 | dansmith | specifically we need to keep something like "Running multiple nova-compute processes that point at the same conductor group is now deprecated" | |
| 17:30:31 | dansmith | and the conf rename and the deprecation reason there | |
| 17:30:38 | dansmith | however we do that is fine | |
| 17:31:01 | bauzas | I just did read the whole patch | |
| 17:31:08 | bauzas | and I think I can abandon my revert | |
| 17:31:19 | bauzas | the deprecation just says the truth | |
| 17:33:11 | bauzas | dansmith: JayF: remind me my SLURP knowledge | |
| 17:33:22 | bauzas | if we say 'we deprecate in B' | |
| 17:33:41 | bauzas | that means we can't hardly remove in C, provided ops wouldn't have had the memo | |
| 17:33:44 | sean-k-mooney | if we deprecate in be we still have to keep it to have the deprecateion released in C | |
| 17:33:45 | dansmith | we also need to deprecate in C and then we can remove in E, assuming C has the shards as usable | |
| 17:33:57 | JayF | Yeah, that matches my understanding. | |
| 17:33:59 | sean-k-mooney | so first removal is D regardless of when we deprecate | |
| 17:34:11 | bauzas | yeah | |
| 17:34:16 | dansmith | right, we could remove in D | |
| 17:34:25 | dansmith | first slurp it can be missing from is E | |
| 17:34:30 | bauzas | we have a couple of deprecations this cycle | |
| 17:34:40 | bauzas | https://docs.openstack.org/releasenotes/nova/unreleased.html#deprecation-notes | |
| 17:34:48 | sean-k-mooney | yep which is fine | |
| 17:34:51 | bauzas | so I'll ensure we don't remove anything next cycle | |
| 17:35:01 | sean-k-mooney | we will be removing thing next cycle | |
| 17:35:03 | sean-k-mooney | just not those | |
| 17:35:27 | sean-k-mooney | we have deprecated things from A and older that we can remove | |
| 17:35:46 | sean-k-mooney | that is ok because A was a slurp | |
| 17:36:04 | sean-k-mooney | so if it was deprected in A we can remove if it was deprected in B we cant until D | |
| 17:36:26 | sean-k-mooney | ok im going to call it a day | |
| 17:36:30 | sean-k-mooney | o/ | |
| 17:37:56 | bauzas | JayF: dansmith: updated the cycle highlights in order to remove any occurrence of the shards https://review.opendev.org/c/openstack/releases/+/894213 | |