| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-09-13 | |||
| 16:39:22 | JayF | bauzas: johnthetubaguy and I are on a call right now trying to find alternatives | |
| 16:39:27 | JayF | bauzas: I assure you I am the saddest panda | |
| 16:40:19 | bauzas | gmann: prelude just arrived, but maybe we should remove the first bullet point items | |
| 16:42:08 | bauzas | JayF: we still have time to remove the shards from the highlights and remove the conf options, if you want MHO | |
| 16:42:23 | bauzas | or just revert the 3rd patch tbc | |
| 16:42:34 | JayF | that is the worst of all options, I think we have a chance of getting a cleaner fix | |
| 16:42:35 | bauzas | I can hold RC1 just for that | |
| 16:42:49 | bauzas | JayF: a clean fix can't be a client change | |
| 16:43:02 | JayF | we are trying to manipulate it to do what we want w/o the client change | |
| 16:43:06 | bauzas | it's a long-term solution, but this can't happen in the timeframe we hav | |
| 16:43:08 | JayF | that patch is up as the known-working example | |
| 16:43:57 | bauzas | well, the latest patchset is just a noop but yeah | |
| 16:44:24 | gmann | bauzas: noted, will wait for that | |
| 16:44:36 | bauzas | the fact is, I'm surprised we're facing this at the very end, but urgency requires me to do the post-mortem after RC1, not now | |
| 16:45:06 | bauzas | but I'd enjoy any very quick fix | |
| 16:45:08 | dansmith | bauzas: I think it makes sense to communicate the "not super tested" nature of it if we're going to leave it in the prelude, personally | |
| 16:45:11 | bauzas | trust me about it | |
| 16:45:25 | dansmith | given what we normally expect for validation of headline features like that | |
| 16:45:41 | bauzas | dansmith: yeah, and trust me, I can even remove it from the prelude | |
| 16:45:59 | bauzas | actually the highlights worry me much | |
| 16:46:12 | bauzas | since they will be used by the marketing folks for the marketing fest | |
| 16:46:32 | bauzas | fortunately, this isn't merged yet, I'm gonna put a -W until we clarify the state | |
| 16:54:10 | bauzas | JayF: I'll drop now for 2.5 hours but I'll come back | |
| 16:54:24 | bauzas | in case you have options | |
| 17:12:41 | sean-k-mooney | JayF: bauzas i think we use ironic client of rthe shard stuff no? | |
| 17:12:50 | JayF | bauzas: I am convinced a revert is the best path. I'd personally prefer even reverting the peer_list deprecation; but if we leave that deprecation in, can I get some assurance that we won't *remove* peer_list until sharding lands? | |
| 17:12:54 | sean-k-mooney | JayF: stephenfin had a seriese ot move ironic to the sdk | |
| 17:12:55 | JayF | sean-k-mooney: we use a mix of client and sdk | |
| 17:13:09 | sean-k-mooney | ok but we didnt land the changes to move to sdk only | |
| 17:13:12 | JayF | sean-k-mooney: which is part of why this was complex to figure out what was going on | |
| 17:13:20 | JayF | https://etherpad.opendev.org/p/nova-sharding-rca I've created this | |
| 17:13:24 | sean-k-mooney | ok | |
| 17:13:27 | JayF | putting notes from our technical research for follow-ups | |
| 17:13:34 | sean-k-mooney | so are we using ironic client for this or not? | |
| 17:13:46 | JayF | all node fetches in Ironic for node listing use sdk | |
| 17:13:50 | JayF | most node updates/writes use client | |
| 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 | |