| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-30 | |||
| 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 | |
| 19:25:46 | sean-k-mooney[m] | which made sense when the intent was to allow update with config drive | |
| 19:25:48 | dansmith | another interesting wrinkle in that case, | |
| 19:25:55 | dansmith | is that we'll set that flag even if the compute is way too old, | |
| 19:25:57 | sean-k-mooney[m] | since it was lazy | |
| 19:26:16 | dansmith | then they reboot, nothing happens.. then 6mo later when the operator upgrades that compute .. poof, user data changes | |
| 19:26:33 | sean-k-mooney[m] | yes but when the compute is eventually updated it will regenerate the config on the next reboot | |
| 19:27:48 | dansmith | right, but that could be many months later | |
| 19:27:56 | dansmith | which could be quite confusing for someone, | |
| 19:28:15 | dansmith | especially if they wrote something to test, tried to set it via hard reboot, it seemed to allow it but never worked, shrugged and went off, | |
| 19:28:24 | dansmith | then 6mo later it changes suddenly... | |
| 19:28:38 | dansmith | that user_data script might have been untested, they couldn't test it, so they assumed no harm | |
| 19:29:08 | sean-k-mooney[m] | it could. so orginally the problem of how/when to regnerate the conf dirver came up a few months ago after the spec review | |
| 19:29:27 | dansmith | I upgraded my +0 to -1: https://review.opendev.org/c/openstack/nova/+/816157 | |
| 19:29:32 | sean-k-mooney[m] | and at the time the ideia of the dirity flag was disucsed to allow this | |
| 19:29:59 | sean-k-mooney[m] | its obvious we should have intoduced the rpc bump now | |
| 19:30:15 | dansmith | yeah I understand the "lazy do it later" aspect, but if it's broken and doesn't get honored right away, that "later" being a half year or more seems like more harm than good | |
| 19:30:25 | sean-k-mooney[m] | but orgianlly i tought that lazy rebuild was actully desirebale | |
| 19:30:39 | sean-k-mooney[m] | ack | |
| 19:30:51 | dansmith | lazy is good as long as it's not TOO LAZY :P | |
| 19:31:53 | sean-k-mooney[m] | the rebuild series has both a compute service bump and rpc bump right | |
| 19:32:22 | dansmith | yes | |
| 19:32:30 | sean-k-mooney[m] | so eihter way it will have to be updated when this is adressed | |
| 19:32:38 | dansmith | yes | |
| 19:32:50 | sean-k-mooney[m] | so 6.1 would be for update_userdata and 6.2 for rebuild | |
| 19:33:02 | dansmith | yeah | |