| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-30 | |||
| 18:32:51 | dansmith | gibi: as you said, we might as well keep working on the top patch until it gets closer to the bottom, so ^ | |
| 18:46:03 | gibi | dansmith: sure, I abandoned mine | |
| 18:46:18 | gibi | and put the +2 back to the top of bfv | |
| 18:55:55 | dansmith | gibi: I'm about to commit some comments on the user_data patch, | |
| 18:55:58 | dansmith | can you have a look? | |
| 18:57:51 | dansmith | gibi: specifically this: https://review.opendev.org/c/openstack/nova/+/816157/comments/f31f7593_ff5dd9fc | |
| 18:58:03 | dansmith | isn't this just using instance sysmeta to avoid an RPC bump? | |
| 18:58:21 | dansmith | and thus it's side-stepping any RPC or object versioning we'd normally have for something like this? | |
| 18:58:39 | dansmith | rebuild is basically growing a new feature, and we need to pass it a flag, | |
| 18:58:57 | dansmith | but instead of the flag and version bump, we're stashing it in instance sysmeta | |
| 18:59:09 | dansmith | which means we can't fail the call with "sorry we're pinned to RPC 6.1 so we can't do that right now" | |
| 18:59:13 | dansmith | sean-k-mooney: ^ | |
| 19:01:21 | sean-k-mooney[m] | the orginal idea was to allow updating the user data and then regenerate the config drive the next time we reboot the instance | |
| 19:01:49 | dansmith | but that's not how it's implemented now right? | |
| 19:01:50 | sean-k-mooney[m] | although i think we changed our mind and blocked update with config drive | |
| 19:01:57 | dansmith | right | |
| 19:02:23 | sean-k-mooney[m] | so im not sure why we are blocking it now | |
| 19:02:41 | sean-k-mooney[m] | but that is why it was done that way | |
| 19:02:42 | dansmith | why does hard reboot not just always regenerate config drive? | |
| 19:02:48 | sean-k-mooney[m] | no | |
| 19:02:56 | sean-k-mooney[m] | it does not do it today at all | |
| 19:03:05 | dansmith | I'm saying.. why not just make it regenerate always | |
| 19:03:14 | dansmith | instead of the dirty flag | |
| 19:03:44 | dansmith | then you get to say it's best-effort, based on compute and virt support for doing so | |
| 19:03:47 | sean-k-mooney[m] | that would need to pull the data form the db but i guess we could | |
| 19:04:05 | dansmith | so? | |
| 19:04:19 | sean-k-mooney[m] | just saying that the side effect | |
| 19:04:24 | dansmith | the way it is right now, you've basically created a shadow RPC interface with no versioning which also accumulates in the DB | |
| 19:04:26 | sean-k-mooney[m] | i think we wanted to aovid that | |
| 19:05:13 | sean-k-mooney[m] | im trying to think if there is any other downside to always doing it | |
| 19:05:34 | sean-k-mooney[m] | we added a trait to signel if the backend supprots regenerating it | |
| 19:06:07 | dansmith | yeah, which also requires that we ask placement if *we* support a thing, which seems kinda odd :) | |
| 19:06:26 | sean-k-mooney[m] | well its a compute capablity trait | |
| 19:06:32 | sean-k-mooney[m] | we have several like that | |
| 19:06:43 | dansmith | but we don't have to check placement for that right? | |
| 19:07:32 | sean-k-mooney[m] | am i dont think its in the api db so normally i think we do check placement | |
| 19:07:59 | dansmith | the ones that we use for scheduler filtering make sense of course, but I thought we wrote them somewhere we could get at them ourselves | |
| 19:08:00 | dansmith | anyway | |
| 19:08:08 | dansmith | the shadow RPC interface seems much worse to me | |
| 19:08:29 | sean-k-mooney[m] | i dont thikn its in the compute nodes table so im not sure where they would be | |
| 19:09:03 | sean-k-mooney[m] | i guess we were not really thinking of it as a rpc interface | |
| 19:09:11 | sean-k-mooney[m] | just some metadta on the instance | |
| 19:09:22 | sean-k-mooney[m] | but i see your point | |
| 19:09:25 | dansmith | well the test is, that if you ran this under grenade, | |
| 19:09:34 | dansmith | you'd allow the reboot with the new user data, but it wouldn't get honored | |
| 19:09:57 | sean-k-mooney[m] | for an un upgraded compute | |
| 19:10:01 | sean-k-mooney[m] | hum | |
| 19:10:11 | sean-k-mooney[m] | ya your right | |
| 19:10:13 | dansmith | so you go to a lot of work to return a fail to the API call if it's not honor-able, but then you'll quietly say "got it" and send it off to a compute that will ignore it :) | |
| 19:10:20 | sean-k-mooney[m] | so we would also neeed a compute service bump | |
| 19:10:27 | sean-k-mooney[m] | and min version check | |
| 19:10:37 | dansmith | not really, | |
| 19:10:43 | dansmith | the rpc pinning will handle that for you | |
| 19:11:09 | sean-k-mooney[m] | if we changed the hard reboot api with a new paramter | |
| 19:11:11 | dansmith | the auto pin will stick to the minimum supported version, and the rpcapi.py will raise if it can't send at v6.1 so you can error the api call | |
| 19:11:14 | dansmith | right | |
| 19:11:15 | sean-k-mooney[m] | or always rebuit as you said | |
| 19:11:30 | dansmith | if we always rebuild, then we need a service version and check, | |
| 19:11:40 | dansmith | because then you're assuming the compute will do a thing that it might not | |
| 19:11:53 | dansmith | letting rpc handle it is simpler and more direct | |
| 19:11:54 | sean-k-mooney[m] | yep that why i was thinink the check initally | |
| 19:12:06 | sean-k-mooney[m] | ya fair point | |
| 19:12:20 | sean-k-mooney[m] | how do you want to proceed | |
| 19:12:51 | dansmith | I dunno, it sucks that this is going to bump the bfv one too because it already touches rpc :/ | |
| 19:12:53 | dansmith | but this also seems very wrong | |
| 19:13:32 | dansmith | I can probably bang out the RPC change pretty quick | |
| 19:13:45 | dansmith | but it might push either of these into FFE territory, | |
| 19:13:48 | sean-k-mooney[m] | i feel like this is not the first time we have done it this way. but that does not me it was right before | |
| 19:14:09 | dansmith | well, I hope we haven't done this before | |
| 19:14:33 | dansmith | this is the whole reason we have all the rpc, object, and service version plumbing | |
| 19:14:39 | dansmith | for this exact sort of thing | |
| 19:14:49 | sean-k-mooney[m] | not via system metadata | |
| 19:14:59 | sean-k-mooney[m] | im actully thinking about a bug fix we did | |
| 19:15:09 | sean-k-mooney[m] | where we needed to fix it and backport | |
| 19:15:22 | sean-k-mooney[m] | but that was becausse we could not change the rpc | |
| 19:15:38 | sean-k-mooney[m] | i dont think we have done this in a new feature | |
| 19:15:55 | dansmith | well, it kinda depends on the mechanics as to whether or not that was okay or not | |
| 19:16:07 | dansmith | this is basically a new flag to an RPC call, so side-stepping it is pretty bad | |
| 19:16:25 | dansmith | if it's the other way around, like compute decorating an instance so the controllers can see something, that's a bit different | |
| 19:17:55 | dansmith | I'm guessing there's no tempest test for this and thus no hope that even luck would get us a test fail on a grenade job? | |
| 19:18:11 | sean-k-mooney[m] | we stashed a flag in the port porfile in the migration vif object https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L81-L92 | |
| 19:18:12 | dansmith | I mean, it'll clearly break, I just mean to demonstrate it | |
| 19:18:19 | dansmith | because if we did, this couldn't even merge | |
| 19:18:40 | sean-k-mooney[m] | ther is no tempest test for this no | |
| 19:19:11 | dansmith | that seems to me to be the "should we merge this as-is".. if we can easily write a valid tempest test that would fail on the grenade job, that's pretty bad | |
| 19:20:46 | sean-k-mooney[m] | i dont think it would be that hard to test this although i have not really done much with tempest. lets see if there is a simiar test we can modify | |
| 19:21:39 | dansmith | no, | |
| 19:21:44 | dansmith | I'n not saying we should waste time on a test | |
| 19:21:59 | dansmith | I'm saying we *could* easily write a legit test that *will* fail on a grenade job because this is broken | |
| 19:22:14 | dansmith | that should be our *semantic test* for "is this something we should merge" | |
| 19:23:02 | dansmith | sean-k-mooney[m]: this also has a bit of a race sort of condition in it | |
| 19:23:11 | dansmith | if I do a hard reboot with new user data, | |
| 19:23:20 | sean-k-mooney[m] | ah right i think you confinced me already that we should not merge it as is | |
| 19:23:22 | dansmith | the API will write the user data to the instance record, then go to make the rpc call | |
| 19:23:31 | dansmith | if rabbit is down, or the compute is down, the rpc call will fail, | |
| 19:24:33 | dansmith | well, I guess it will still see the dirty flag in that case, but only if they do another hard reboot | |
| 19:24:44 | dansmith | point being, the pre-update of the user data is presumptuous | |
| 19:24:45 | sean-k-mooney[m] | yes | |
| 19:24:55 | dansmith | not easy to resolve that at this point though | |
| 19:25:03 | sean-k-mooney[m] | the dirty flag was intneded to say regenerate the config drive the next time you hard reboot | |