| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-06-29 | |||
| 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 | |
| 14:27:06 | dansmith | bauzas: I think we've done worse things than string->enum even :) | |
| 14:27:15 | bauzas | just to make sure people understand why I'm accepting it | |
| 14:27:31 | dansmith | does it have a code comment about the type changin? | |
| 14:27:40 | dansmith | if not, you could/should -1 probably and demand it | |
| 14:27:58 | dansmith | just so people know that older clients could be less strict | |
| 14:28:06 | stephenfin | The type isn't changing | |
| 14:28:19 | dansmith | it's going from string to enum right? | |
| 14:28:32 | stephenfin | Nope. It was an EnumField with 4 allowed values. It's still an EnumField with four values | |
| 14:28:35 | stephenfin | https://review.opendev.org/#/c/706013/6/nova/objects/migration.py | |
| 14:29:00 | stephenfin | I've just making it a custom enum field so I have constants I can reference | |
| 14:29:01 | dansmith | oh, even less of a thing then, nevermind | |
| 14:29:19 | dansmith | yeah that's definitely not visible to RPC, so whatever | |
| 14:30:46 | openstackgerrit | Balazs Gibizer proposed openstack/nova stable/train: Guard against missing image cache directory https://review.opendev.org/738455 | |
| 14:31:15 | bauzas | stephenfin: dansmith: yeah, it's just changing the EnumField to be a specific one | |
| 14:31:40 | bauzas | but, tbc, I don't want to see other changes just changing types without thinking about this | |
| 14:32:30 | bauzas | stephenfin: fwiw, I'd also have loved if you could have cut this change in two and not adding two properties as well in the same | |
| 14:32:42 | bauzas | given this wasn't needed | |
| 14:33:14 | stephenfin | It wouldn't really help though | |
| 14:33:37 | bauzas | anyway, reviewing it | |
| 14:33:56 | stephenfin | You'd save about four lines in the first patch (the properties), and then you'd have to review a second patch that's reworks virtually everything you changed in the first one | |
| 14:54:36 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM: Test the state of VMware NSX 3pp CI https://review.opendev.org/734114 | |
| 15:17:45 | openstackgerrit | Balazs Gibizer proposed openstack/nova stable/train: Guard against missing image cache directory https://review.opendev.org/738455 | |
| 17:21:18 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Warn at controller start if there are older than N-1 computes https://review.opendev.org/738482 | |
| 17:38:08 | openstackgerrit | Merged openstack/nova master: objects: Add MigrationTypeField https://review.opendev.org/706013 | |
| #openstack-nova - 2020-06-30 | |||
| 04:15:12 | openstackgerrit | Merged openstack/nova stable/queens: Reproduce bug 1862633 https://review.opendev.org/729539 | |
| 04:15:12 | openstack | bug 1862633 in OpenStack Compute (nova) "unshelve leak allocation if update port fails" [Medium,Fix released] https://launchpad.net/bugs/1862633 - Assigned to Balazs Gibizer (balazs-gibizer) | |
| 06:04:18 | openstackgerrit | Alexandre Arents proposed openstack/nova master: Limit the number of concurrent snapshots https://review.opendev.org/736169 | |