| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-06-29 | |||
| 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 | |
| 14:26:56 | stephenfin | dansmith: correct. Validation through association | |
| 14:27:06 | bauzas | okay, I'll provide a comment then | |