| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-30 | |||
| 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 | |
| 19:33:09 | dansmith | are you thinking be sneaky and combine? I dunno how I feel about that | |
| 19:33:30 | sean-k-mooney[m] | no | |
| 19:33:53 | sean-k-mooney[m] | 6.1 for extending hard_reboot | |
| 19:34:01 | sean-k-mooney[m] | and 6.2 for extending rebuild | |
| 19:34:08 | sean-k-mooney[m] | not cominign both into 6.1 | |
| 19:34:18 | sean-k-mooney[m] | since there in different patches that would be odd | |
| 19:34:23 | dansmith | ack, yep, that's what i'll be, and of course service versions for each bump | |
| 19:34:35 | dansmith | *it'll | |
| 19:34:39 | sean-k-mooney[m] | and we should not squash them obviously since the patches are unrelated | |
| 19:35:11 | dansmith | no, I just thought you were about to "get creative" :) | |
| 19:35:29 | sean-k-mooney[m] | hehe not with correctness | |
| 19:35:49 | sean-k-mooney[m] | out side of a bug fix… | |
| 19:36:59 | sean-k-mooney[m] | jhartkopf: ^ | |
| 19:37:09 | sean-k-mooney[m] | not sure you were following that | |
| 19:38:17 | dansmith | not online, AFAICT | |
| 19:39:00 | dansmith | ugh, | |
| 19:39:11 | dansmith | all of the virt and rpc stuff that the rebuild patches touch will conflict out | |
| 19:39:18 | dansmith | I so wish luck had landed these in opposite order | |
| 19:46:56 | sean-k-mooney[m] | by the way apprently the matix bridge does not actullly only show you the currently online people | |
| 19:47:24 | sean-k-mooney[m] | which is why jhartkopf auto completed for me even if they are not here | |
| 19:51:26 | dansmith | sean-k-mooney[m]: https://termbin.com/q3ev | |
| 19:51:32 | dansmith | I think that's basically what needs to happen | |
| 19:52:43 | dansmith | I wish we could get a read on this from melwitt and gibi so I should know if I should spend my evening trying to get this all changed and tested | |
| 19:53:31 | sean-k-mooney[m] | ignoring your base64 nits that looks about right | |
| 19:54:57 | dansmith | it also seems like the tests on this are pretty lacking, no? | |
| 19:55:04 | dansmith | like, there are no tests on the virt driver changes? | |
| 19:55:10 | sean-k-mooney[m] | im currently trying to set up a devstack env i dont currently have one to test it. we do still have 2 days for code frezee | |
| 19:55:42 | dansmith | like, no test that I see that actually checks that the libvirt driver will honor the flag | |
| 19:56:03 | opendevreview | Rico Lin proposed openstack/nova master: Add traits for viommu model https://review.opendev.org/c/openstack/nova/+/844507 | |
| 19:56:19 | gibi | I'm not sure I have enough brain left today for this. But I have at least two comments to the above 1) I don't think this is a rebuild feature it is tight to hard reboot 2) I think we prevented the possible upgrade issue with the capability trait. | |
| 19:56:43 | ricolin | sean-k-mooney[m]: just update the viommu traits patch :) | |
| 19:56:45 | gibi | I cannot argue that this can be done differently | |
| 19:57:08 | gibi | and dansmith you are right that the system metadata dependency makes it at least a grey interface | |
| 19:57:42 | gibi | it is sad that we figured out this issue late in the cycle | |
| 19:58:12 | dansmith | gibi: I don't understand what you mean by #1, but yeah I guess you're right on the trait.. that's preeety thin though :) | |
| 19:59:02 | gibi | #1 is probably just a missunderstanding from | |
| 19:59:03 | gibi | 20:58 < dansmith> rebuild is basically growing a new feature, and we need to pass it a flag, | |
| 20:00:03 | sean-k-mooney[m] | the trait wont be reported on a non upgraded compute yes | |
| 20:00:14 | sean-k-mooney[m] | which might help for the upgrade chase specifically | |
| 20:00:26 | dansmith | gibi: oh yeah I meant reboot there sorry | |
| 20:00:27 | gibi | on the flag itself. The RPC flag vs the persisted field has some semantic difference. If we update the user_data in the DB in the API layer then pass a flag to regenerate the config drive via the RPC then a lost RPC means that the DB data and the config data got out of sync | |
| 20:01:24 | dansmith | gibi: but we can and should revert if we're reporting failure to the user | |
| 20:01:30 | sean-k-mooney[m] | so im not sure if we should revert | |
| 20:01:32 | dansmith | because if this happens because of the lack of a trait or version, | |
| 20:01:34 | gibi | the reboot RPC is a cast | |