Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-07-28
16:51:29 sean-k-mooney so regenreating the entire xml form info passed back is the opisce of makeing the xml cannonical
16:51:36 stephenfin sean-k-mooney: oh, gotcha. Yeah, I'm on the "nova is authoritative" side of the fence
16:51:43 sean-k-mooney same
16:52:01 sean-k-mooney which is why i would be totally happy to always regenerate the xml form nova datastuctures
16:52:25 sean-k-mooney kind of like the migrate data but im asserting you already have the info you need with out having to pass it
16:52:33 sean-k-mooney not that the approch is wrong
16:52:35 stephenfin it really sounds like you're advocating for the same position as me
16:52:38 stephenfin ah
16:52:50 sean-k-mooney jsut that you should not have two places to get the info
16:53:03 sean-k-mooney the falvor/image and the migrate data object
16:53:10 stephenfin so I'm for it, artom is against it, and sean-k-mooney is kind of for it but not the way I've done it
16:53:23 stephenfin lovely :)
16:53:44 sean-k-mooney well 3 enginerrs your normally expect at least 4 oppinions
16:53:47 artom stephenfin, if we end up doing a push towards "Nova is authoritative, XML can suck it" I wont' stand in anyone's way
16:53:49 sean-k-mooney 3 is light :P
16:54:06 artom stephenfin, I just think in the current context it's premature
16:54:12 sean-k-mooney artom: that is the status quoe today
16:54:27 sean-k-mooney so the null hypotous is that nova is authritavive and xml is not
16:54:33 artom sean-k-mooney, well, except when it isn't? Like live-migration? :)
16:54:39 artom In practice, I mean
16:54:40 stephenfin Okay, I figured I had the information to hand when generating the migration data object, so it made sense to slot it in there, even if I could technically generate it on the other end
16:54:44 sean-k-mooney nova is still athuritive
16:55:02 sean-k-mooney if you ever modify the xml then the vm is not supproted anymore
16:55:06 artom sean-k-mooney, I guess? Is it really though, if it's using the existing XML to pull info?
16:55:39 sean-k-mooney nova created the exsting xml
16:55:50 artom Heh, true
16:55:53 sean-k-mooney and nothing outside of nova is allowed to modify it
16:55:54 artom Turtles all the way down!
16:55:55 sean-k-mooney well bar libvirt
16:55:57 stephenfin artom: As before. Maybe. Maybe not. It seemed fairly trivial, consistent and would always be correct, even it was slightly overengineered
16:56:23 artom stephenfin, all true. My beef is about the last part.
16:56:32 sean-k-mooney ok so https://review.opendev.org/#/c/743568/1 is artoms backportable change
16:56:33 stephenfin Yup, got that now
16:56:48 sean-k-mooney and https://review.opendev.org/#/c/743588/1 is your cleanup
16:56:51 stephenfin sean-k-mooney: nah, that's me too
16:57:04 stephenfin Add a TODO, resolve TODO in follow-up
16:57:14 sean-k-mooney ok well the first is intended to be backported and then a master only followup
16:57:24 stephenfin yup
16:57:35 stephenfin that's the overengineered idea, anyway
16:57:38 stephenfin :)
16:58:24 sean-k-mooney how did we end up with 2 elements
16:58:35 stephenfin read the bug report
16:58:39 stephenfin libvirt does it
16:59:12 sean-k-mooney really because it looks like we shoudl be updating the xml to only have one right
16:59:17 sean-k-mooney with the orginal code
16:59:35 stephenfin nope, we just update the first one we find
16:59:40 stephenfin not expecting there to be more
16:59:48 sean-k-mooney oh libvirt splits them
17:00:14 sean-k-mooney so <vcpusched vcpus="2-3" scheduler="fifo" priority="1"/>
17:00:23 sean-k-mooney becomes <vcpusched vcpus="2" scheduler="fifo" priority="1"/>
17:00:26 sean-k-mooney <vcpusched vcpus="3" scheduler="fifo" priority="1"/>
17:00:30 sean-k-mooney then we update the first one
17:00:59 stephenfin yup
17:01:43 sean-k-mooney then why can we not delete the vcpushed element and just not add the new one
17:01:57 stephenfin that's precisely what I'm doing
17:02:03 sean-k-mooney oh that is what your doing
17:02:10 sean-k-mooney in https://review.opendev.org/#/c/743568/1/nova/virt/libvirt/migration.py
17:02:17 stephenfin yuuup
17:02:46 stephenfin loads of context in the bug report and commit message itself, and it's pretty trivial to reproduce
17:02:52 stephenfin and with that, I'm out
17:02:54 stephenfin o/
17:03:13 sean-k-mooney so i would just read the policy form the element before we remove it
17:03:31 sean-k-mooney here https://review.opendev.org/#/c/743568/1/nova/virt/libvirt/migration.py@130
17:03:50 sean-k-mooney we can even assert that its the same for all element if we want
17:03:52 stephenfin <stephenfin> we should be trying to avoid introspecting that XML to try guess what was done previously
17:04:23 sean-k-mooney well yes which is why i said we shoulc caluated it form the flaovr/image
17:04:48 sean-k-mooney but if you dont want to do that and dont want to hardcode tehn introscpect is the only other option
17:05:19 sean-k-mooney wait
17:05:46 sean-k-mooney the check is "if 'sched_vcpus' and 'sched_priority' in info:"
17:06:03 sean-k-mooney what is info['sched_priority']
17:06:24 sean-k-mooney its the dest numa toplogy object
17:06:39 sean-k-mooney which is calulated form the flavor and image
17:06:55 sean-k-mooney oh but that does not have the schulder right
17:07:02 sean-k-mooney just the priority
17:09:21 sean-k-mooney actully no info is this https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L113-L130
17:20:29 sean-k-mooney stephenfin: so looking at the code you would have to pass the flavor/image down two function calls form
17:20:32 sean-k-mooney https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L8939
17:20:38 sean-k-mooney or you could pass the instance object
17:20:49 sean-k-mooney i would proably pass the instance
17:21:41 sean-k-mooney stephenfin: im +1 on the first patch and -0.5 on the second
17:22:42 sean-k-mooney i think changing 2 internal function calls and caluating it form the flavor/image is cleaner then changing the objects
18:49:54 lyarwood stephenfin / artom ; https://review.opendev.org/#/q/topic:bug/1889108 updated btw if you have time this evening
18:55:59 artom lyarwood, left a comment on one of them. Not sure it's -1 worthy
18:57:57 lyarwood artom: well the issue itself isn't tied to any microversion but pinning on anything later than stable/queens and 2.60 just introduces pointless churn in the backports
18:58:38 artom lyarwood, so we could just remove the microversion= line altogether?
18:58:57 lyarwood artom: wouldn't it pin to latest then introducing churn in the backports?
18:59:09 artom How so?
18:59:48 artom I honestly don't know, but if we do, 'latest' for queens isn't 'latest' for master...
18:59:57 lyarwood microversion = None
19:00:00 lyarwood fun
19:01:50 artom lyarwood, so I went looking to remember how I did func tests for NUMA live migration
19:01:58 artom Which I've proposed as a U backport
19:02:13 artom https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_numa_live_migration.py#L41 appears to run just fine in U
19:02:45 artom And then https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_numa_live_migration.py#L387 is the "functional" reason I'm talking about it - ie, there's a need for something a specific microversion provides
19:03:22 artom So I would suspect 'latest' will be fine all the way down to queens
19:03:30 lyarwood artom: so microversion = None actually means the oldest?
19:03:38 artom I guess?
19:07:12 lyarwood artom: urgh this sucks
19:07:26 artom How so?
19:07:36 lyarwood artom: leaving it set to None breaks my use of loads of the helper methods from _IntegratedTestBase

Earlier   Later