| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-10-01 | |||
| 15:14:40 | bauzas | any new contributor is appreciated :) | |
| 15:14:51 | bauzas | (even if pierre isn't exactly new to me :D ) | |
| 15:15:27 | priteau | I can try to add unit tests, if I can't figure it out I'll ask for help | |
| 15:15:30 | artom | bauzas, yep, I'm happy either way, just interested in keeping things moving | |
| 15:16:00 | bauzas | priteau: sure, you are 1 hour away from my usual time, so you can ping me for help | |
| 15:16:09 | bauzas | I could redirect | |
| 15:16:14 | priteau | bauzas: Actually I live in France now ;-) | |
| 15:16:31 | bauzas | heh, I see the French mafia growing | |
| 15:16:59 | priteau | I've actually thought about an issue with the whole migration approach (including the existing function) | |
| 15:17:13 | priteau | It updates the object but not the version number field | |
| 15:17:28 | priteau | Is this likely to be an issue? | |
| 15:18:40 | priteau | With the existing code, a pinned instance started on Ussuri would have a 1.4 InstanceNUMACell object | |
| 15:18:49 | artom | priteau, that's an excellent question, actually. I suspect there's magic in the ovo code that does it automatically? | |
| 15:19:50 | bauzas | well, I need more context | |
| 15:20:21 | dansmith | I'm not sure what the question is | |
| 15:20:26 | bauzas | we only update the object is something remotable is added | |
| 15:20:34 | bauzas | the object version* | |
| 15:20:37 | artom | dansmith, the context is https://review.opendev.org/c/openstack/nova/+/810849/2/nova/objects/instance_numa.py, and the question is, won't that leave the InstanceNUMACell as version 1.4 in the DB, even if we added the pcpuset and migrated to 1.5 | |
| 15:21:10 | bauzas | artom: is the pcpuset attribute a ovo field ? | |
| 15:21:35 | artom | It's a SetOfIntegersField | |
| 15:21:56 | bauzas | lemme find this object | |
| 15:22:00 | bauzas | the whole module is too large | |
| 15:22:11 | bauzas | ok, I see it | |
| 15:22:20 | bauzas | so, yeah this field exists | |
| 15:22:28 | bauzas | why should we bump the object version ? | |
| 15:22:32 | dansmith | artom: you're asking if you deserialize, tweak and re-serialize, if you'll serialize the 1.4 version that came from the original thing? | |
| 15:23:03 | artom | dansmith, aye | |
| 15:23:26 | artom | Tweak specifically by adding the field that was added in the 1.5 version | |
| 15:23:53 | bauzas | artom: then the remote object won't get it | |
| 15:24:08 | dansmith | no, when you deserialize an object, it becomes a 1.5, which is why you have to be compatible with your object changes | |
| 15:24:36 | dansmith | the new field will be unset because it came from a 1.4, but when you re-serialize it, you'll use the current version unless you ask for an older one (like conductor does when it backports changes for you) | |
| 15:24:49 | priteau | I've just looked at a production system with pinned-instances launched on Train, the system now being on Victoria. The InstanceNUMACell objects are actually showing "nova_object.version": "1.6" | |
| 15:25:14 | priteau | So it looks OK | |
| 15:25:23 | bauzas | wait | |
| 15:25:25 | bauzas | you lost me | |
| 15:25:38 | artom | priteau, cool, so you confirmed in a real system the mechanism that dansmith explained \o/ | |
| 15:25:43 | bauzas | who's on 1.5 (or later) and who's on 1.4 ? | |
| 15:25:53 | dansmith | bauzas: Who is on first | |
| 15:26:43 | bauzas | dansmith: that's what I'm asking | |
| 15:26:58 | bauzas | but it looks it works, so meh | |
| 15:27:27 | priteau | 1.4 are pre-Victoria objects. 1.5, actually none because 1.6 was also introduced in Victoria. Any instance launched or restarted on Victoria gets 1.6 (but only for cpu_policy=dedicated, which is the problem) | |
| 15:27:46 | dansmith | artom: I might actually be wrong here, I' | |
| 15:27:47 | priteau | Anyway, there is no problem with the version number, I just wasn't aware of the nova magic | |
| 15:27:50 | dansmith | I'm looking hang on | |
| 15:28:17 | bauzas | priteau: so, you're asking about what would happen with an old compute and a Victoria conductor or compute, I guess | |
| 15:28:47 | bauzas | dansmith explained it very pretty well | |
| 15:28:59 | bauzas | when you serialize, you add the object version | |
| 15:29:45 | bauzas | so any reader getting the serialized object would know the version and creates a new object based on the compatibility methods we provide | |
| 15:30:30 | bauzas | keeping in mind that obj 1.X doesn't know what to do with 1.y if y > x | |
| 15:30:32 | dansmith | soooo, | |
| 15:30:52 | dansmith | I'm actually not sure this behaves the way I thought.. I don't actually know that it matters, but I think it might be different | |
| 15:31:21 | dansmith | it looks like we _will_ preserve the inbound version that we got from the serialized form when we re-serialize, | |
| 15:31:45 | dansmith | which I think we do for the purposes of not up-leveling objects if we're newer than others in the cluster | |
| 15:32:05 | dansmith | i.e. to avoid a new conductor up-leveling an old version that an api sent to it, knowing an old compute will need the old version | |
| 15:32:27 | dansmith | and also I guess to avoid upleveling something in the database when we load/save it if we're not ready to roll everyone forward yet | |
| 15:32:52 | dansmith | but, the behavior you notice is not the version, it's whether or not the field sticks, which is what I was focusing on, and I think it will, even though that doesn't match the version | |
| 15:32:54 | dansmith | meaningm | |
| 15:33:11 | artom | dansmith, wouldn't upleveling not be a problem because we have code to make it compatible to old versions? | |
| 15:33:25 | dansmith | if you store the object with the new field, it will serialize the new field even if the version is wrong, and a newer node will still pull it out because it doesn't know which version the field belongs to | |
| 15:33:30 | artom | So if the conductor uplevels something, then an old compute loads, it will automatically downlevel for that compute? | |
| 15:34:04 | dansmith | artom: well, I picked conductor because it was easy, but imagine two computes talking | |
| 15:35:43 | dansmith | so I'm not actually sure I can explain priteau's observation of the new versions | |
| 15:36:02 | dansmith | that's how I recalled it working, but I can't back that up with references to the code purely in the deserialization/serialization stuff | |
| 15:36:12 | priteau | These instances were live migrated after the Victoria upgrade, could it explain it? | |
| 15:36:22 | dansmith | do we recreate the numa objects as part of another operation? yeah, like live migration | |
| 15:38:17 | dansmith | this is the test that proves that we don't automatically uplevel it: https://github.com/openstack/oslo.versionedobjects/blob/e7b6d52aa4b1b40e68a21a122c09b968d5959b0e/oslo_versionedobjects/tests/test_objects.py#L778-L785 | |
| 15:38:31 | dansmith | which isn't how I remembered it, but re-reading that test jogs my memory about why | |
| 15:40:33 | dansmith | so I think technically that operation should do something like object.VERSION = ObjectClass.VERSION when it goes to save it out | |
| 15:46:47 | artom | So the upleveling happens at serialization then? | |
| 15:46:58 | artom | Or some other operation like live migration, as you've said | |
| 15:47:00 | dansmith | it doesn't | |
| 15:47:04 | artom | But not at de-serialization, at any rate | |
| 15:47:20 | dansmith | I'm thinking live migrations create new numa stuff for the new host and that's how those got updated | |
| 15:48:18 | dansmith | in the past, we've done things like "load and save the object to update it" which would run routines to do updates, but they are probably saving the objects back with the old version but including the updates :/ | |
| 15:48:28 | dansmith | I can't find any VERSION updating going on in the tree right now | |
| 15:48:42 | dansmith | like for online data migrations I mean | |
| 15:49:08 | dansmith | technically it probably hasn't mattered, because who (besides artom and priteau) would notice that the version was wrong if the data was right? :) | |
| 15:49:54 | artom | Yeah, maybe it's an academic debate - if it does the right thing... | |
| 15:50:00 | artom | Which it appears to do | |
| 15:50:22 | priteau | I just wanted to check it wouldn't break anything if the version number was not updated | |
| 15:50:23 | dansmith | well, we shouldn't store the version incorrectly just because that's going to be confusing, but yeah the object would still load with the data we wanted so in practice it hasn't mattered | |
| 15:50:41 | dansmith | priteau: right, but we should fix it to update the version though | |
| 15:52:02 | priteau | I will leave that to you if you don't mind :D | |
| 15:52:42 | dansmith | artom: do you want me to do that or will you? | |
| 15:53:28 | artom | dansmith, do what? What priteau said, "fix it to update the version though"? | |
| 15:53:38 | artom | Wouldn't that be within priteau own's patch? | |
| 15:53:53 | artom | Well, maybe not, since the on-load migration is pre-existing... | |
| 15:53:54 | dansmith | artom: I said it, but no, the code that claims to migrate is already in the tree | |
| 15:53:55 | artom | He's just fixin it | |
| 15:53:55 | dansmith | right | |
| 15:53:58 | dansmith | right | |
| 15:54:21 | dansmith | looks like there are several _migrate_* cases in instance_numa.py | |
| 15:54:28 | artom | And probably elsewhere... | |
| 15:54:33 | dansmith | maybe, not sure | |
| 15:54:57 | dansmith | if you do it I can review/+2 but ... :) | |
| 15:55:46 | artom | I mean, the hint of a +2 from dansmith on a PS1 is pretty alluring... | |
| 15:55:58 | dansmith | well, if you get it right in PS1.. :P | |
| 15:56:13 | artom | Crazier things have happened | |
| 15:56:14 | dansmith | let me write you a diff to start, hang on | |
| 15:58:13 | dansmith | I think this should be unified instead of done in multiple places like it is, so something like this: https://termbin.com/iawkf | |