Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-06-29
12:37:02 openstackgerrit renminmin proposed openstack/nova master: SRIOV: Fix instance with macvtap port live migration failed https://review.opendev.org/738432
13:06:34 openstackgerrit Stephen Finucane proposed openstack/nova master: Update keypairs in saving an instance object https://review.opendev.org/683043
13:13:58 stephenfin gibi: I've reproposed the use-pcpu-and-vcpu-in-one-instance BP work for a runway. What was approved has merged (thanks for the reviews!), and now there are four patches at the top good to go and huaquiang is reworking the remainder at the moment
13:14:15 stephenfin Will ping alex_xu when he's around and ask for reviews
13:14:19 gibi stephenfin: cool
13:14:27 gibi stephenfin: I will move it to the last slot then
13:14:36 stephenfin \o/
13:15:54 stephenfin Also, I took a look through Gerrit on Friday trying to find more stuff to add. No luck. melwitt and johnthetubaguy both have stuff that could conceivably go in (nova-audit and oslo.limit integration, respectively) but they both have open comments :(
13:16:37 johnthetubaguy snowed under at the moment sadly
13:16:42 stephenfin johnthetubaguy: I'll review if/when you do
13:16:50 stephenfin The provider.yaml file work looks stalled as well :(
13:17:06 gibi stephenfin: thanks for looking.
13:17:13 johnthetubaguy stephenfin: thanks, I will try get to that ASAP
13:17:16 gibi yeah I noticed that provider.yaml is stopped
13:17:26 gibi stephenfin: what is the resolution of https://review.opendev.org/#/c/737026/2/nova/virt/hardware.py@1708 ?
13:17:35 johnthetubaguy I wanted to reboot that, but right now don't have bandwidth
13:17:52 johnthetubaguy (on the OpenDev call into)
13:19:13 stephenfin I mean, I could take it over but I don't know who'll be available to review. I've already got the vTPM stuff and mixed CPU types work against me too
13:19:36 stephenfin gibi: Oh, I've a patch for that somewhere. Let me try find it
13:19:59 stephenfin gibi: nvm, pushed it. https://review.opendev.org/#/c/737026/3/nova/virt/libvirt/driver.py
13:20:48 gibi stephenfin: thanks, will look
13:23:04 stephenfin gibi: Sweet. Last thing, it's not ready but I've started work on adding type hints for objects. It's kind of ugly and will probably need a mypy plugin to make it not ugly, but it should fix the gap you were seeing with types not being checked for stuff from nova.objects https://review.opendev.org/#/c/738020/2
13:23:25 stephenfin To be clear, obviously not ready for review yet
13:24:42 gibi stephenfin: interesting. so basically that mypy plugin could look at the ovo field declaration directly and generate the type hints on the fly for ovos
13:25:52 stephenfin gibi: Yup. It seems that's what they do for Django, which has similar magic behavior for things like models https://github.com/typeddjango/django-stubs/tree/master/mypy_django_plugin
13:26:04 stephenfin I just need to figure out how mypy works internally first :(
13:27:21 gibi just opened the code of that plugin and got scared by the internals :)
13:27:51 gibi this feel heavy lifting for me right now
13:28:29 stephenfin Yeah, it's pretty low priority. If I can get the ugly but effective thing working for now, I'll do that
13:29:01 stephenfin and if I did a plugin working, it would live in o.vo itself so nothing you'd have to review
13:31:02 gibi don't misunderstand me I'm happy to indulge into mypy I just need some pre-learning first. so it is scary now as it is unknown
13:31:10 openstackgerrit Balazs Gibizer proposed openstack/nova master: Extend is_ipv6_supported() to cover more error cases https://review.opendev.org/736167
13:31:17 gibi stephenfin: btw, fixed up ^^
13:37:55 gibi stephenfin: thanks
13:40:08 sean-k-mooney gibi: when you say the provider.yaml stuff has stalled to you mean the code or the reivew
13:40:16 stephenfin sean-k-mooney: the code
13:40:40 stephenfin sean-k-mooney: https://review.opendev.org/#/c/673341/
13:40:45 sean-k-mooney ah ok well i guess i could looks tat that again or we can figure something out
13:41:33 sean-k-mooney were there pending change sstill need after v47
13:42:07 stephenfin I don't see anything, but I haven't been involved until now
13:43:01 sean-k-mooney if more changes can be done via followups i would personally prefer to start merging the code and adress it that way gibi johnthetubaguy how would you feel about that
13:43:50 sean-k-mooney i think the code was perfectly resonable to merge even at the end of last cycle but im sure we can still tighten the schema definitions and testing
13:44:14 sean-k-mooney i just dont want perfect to be the enemy of good enough and delay this again
13:44:55 openstackgerrit Stephen Finucane proposed openstack/nova master: Update keypairs in saving an instance object https://review.opendev.org/683043
13:49:36 stephenfin dansmith: Can you look at https://review.opendev.org/#/c/683043/ again today?
13:50:18 gibi sean-k-mooney: I have to re-review the patch to see if everything is resolved or not
13:51:38 gibi sean-k-mooney: restricting the schema later is a backward incompatible change which would be expensive so I'm on the side to do something that is pretty solid
13:51:43 gibi it is like an API
13:52:06 sean-k-mooney sure but until we have a release with this im not sure we need to be as strict
13:52:32 sean-k-mooney e.g. we coudl treat the scema as a 0.X and then bump to 1.0 when we are happy and release with that
13:53:12 gibi sean-k-mooney: 0.x could work, indicating that it is beta and can change in a backward incompatible way
13:53:43 dansmith stephenfin: I just glanced at it and he didn't do what I prescribed (you said he did), but I haven't looked closely at why he thinks that will work
13:54:07 dansmith just FYI in case you didn't examine it closely
13:55:34 stephenfin You'd suggested calling '_save_extra_generic' and 'self.obj_reset_changes' from '_save_keypairs', and he's just put 'self.obj_reset_changes' into '_save_extra_generic'. That looked functionally equivalent to me
13:57:07 dansmith but keypairs isn't an extra field
13:57:37 dansmith I said
13:58:07 dansmith "the generic save handler" but "save_extra_generic" is specifically for fields in instance_extra, which I think keypairs is not in, no?
13:58:46 dansmith or maybe I'm confusing keypairS with keypair
13:59:09 stephenfin I think it is
13:59:21 stephenfin Yeah, it's part of _INSTANCE_EXTRA_FIELDS
14:01:12 dansmith right, okay.. Instance has its own (key_name, key_data) from when we could only have one
14:02:31 stephenfin TIL (that those fields existed)
14:02:58 stephenfin anyway, since it's in _INSTANCE_EXTRA_FIELDS we'll trigger the correct code path https://github.com/openstack/nova/blob/master/nova/objects/instance.py#L788-L790
14:03:24 dansmith aye
14:03:43 stephenfin and calling 'self.obj_reset_changes' on a nested object field would presumably always be the correct thing to do in that path
14:04:53 dansmith I'm not sure about that, I need to check something.. because there are cases where we do and don't delegate that to sub-objects
14:05:01 dansmith since originally all sub-objects would have their own save handler,
14:05:13 dansmith we initially (at least) didn't reset through like that
14:05:57 dansmith I'mma pull it down and look
14:06:10 dansmith https://github.com/openstack/oslo.versionedobjects/blob/master/oslo_versionedobjects/base.py#L629
14:06:18 dansmith that's why we have recursive=
14:07:24 dansmith heh, he makes fake_instance() do recursive=True
14:15:21 stephenfin so instead of https://review.opendev.org/#/c/683043/15/nova/tests/unit/fake_instance.py@143 we could have kept that as-is, and added 'inst.keypairs.obj_reset_changes()'
14:15:47 stephenfin any reason that would be preferable, given "there are cases where we do and don't delegate that to sub-objects"
14:15:49 stephenfin ?
14:16:34 dansmith I dunno, yet, I'm poking..
14:16:42 stephenfin ack
14:16:43 dansmith aren't you a typing pedant such that assertFalse(len(of thing)) feels wrong to you?
14:17:44 stephenfin Yeah /o\ I considered changing it when rebasing and decided not to for some reason. Happy to change if it's not just me
14:18:50 dansmith I stared at "false is not 4" for a few moments when I broke the test on purpose... :)
14:19:12 stephenfin bauzas: Mentioned this Friday but it was a bit late. Care to take a look at https://review.opendev.org/#/c/706013/ when you've time?
14:20:54 bauzas stephenfin: sure I can try
14:22:13 bauzas stephenfin: humpf, I think you can't do this https://review.opendev.org/#/c/706013/6/nova/objects/migration.py
14:22:20 bauzas dansmith: ^
14:22:44 bauzas stephenfin: if you want to change an object field, you can't just change its type directly
14:23:05 bauzas you need to provide another field and,
14:23:06 stephenfin bauzas: dansmith looked at it in the past. It's kosher. The serialized objects look identical, and the validation works as it did
14:23:18 bauzas you need to depracate the other
14:23:23 bauzas deprecate*
14:23:35 bauzas hmmm, ok
14:24:01 stephenfin There would be an issue if I was changing from e.g. StringField to ObjectField or IntegerField, but MigrationTypeField is an EnumField
14:25:03 dansmith bauzas: I haven't looked at what he's offering, but in the past, if we've converted the field type from string to enum and the enum has every possible historical value in it, we've allowed it
14:25:36 dansmith i.e. as long as it won't break existing clients.. the field type doesn't go over the wire, just the assumption that it's de-serializable by the type on the remote side
14:25:37 bauzas well, now I understand
14:25:46 bauzas yeah, stephenfin explained it
14:26:03 dansmith bauzas: I know, but if you're me you wouldn't take stephenfin's word for it, so.. :D
14:26:20 dansmith hence, I assume, the name drop above
14:26:29 bauzas because when deserializing the new object, then the old compute could still be able to create its object
14:26:48 bauzas dansmith: haha
14:26:51 dansmith bauzas: yeah

Earlier   Later