| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-21 | |||
| 21:22:23 | dansmith | sean-k-mooney[m]: it's just not a thing I care much about, but if it's a mixin, then that's better than changing the behavior of the base object | |
| 21:22:53 | dansmith | they behave like any other python object right now, which seems more like I would expect | |
| 21:23:01 | dansmith | it just only version-tracks the things in fields | |
| 21:23:37 | dansmith | I agree we should only set things in init if they're in fields, and explode otherwise | |
| 21:24:41 | sean-k-mooney[m] | Ya the current behavior has helped us break some circular depency too so the current behavior makes sense as the default. | |
| 21:25:10 | tssurya | mriedem: the new bug is again the same thing right ? because a) RPs are not deleted and b)probably the compute service was not shut down before deleting ? so the compute_node record keeps getting recreated ? | |
| 21:25:48 | mriedem | mikal: nice topic https://review.openstack.org/#/q/topic:bp/execs-ive-had-a-few+(status:open+OR+status:merged) | |
| 21:25:52 | melwitt | dansmith: so it would be a mixin in ovo and then use the mixin only in the nova base object? just making sure I understand the best way to go about it | |
| 21:25:58 | mriedem | tssurya: yes | |
| 21:26:24 | tssurya | mriedem: just saw you updated the bug, cool | |
| 21:26:40 | dansmith | melwitt: well you couldn't do it in the nova base object because we utilize the fact that we can set non-field properties on some of our objects | |
| 21:26:50 | dansmith | melwitt: but you could mix it into certain objects if you want | |
| 21:27:09 | dansmith | melwitt: the init fix though could go to the base object in o.vo I think | |
| 21:27:11 | melwitt | I mean to restrict only when calling __init__ | |
| 21:27:13 | melwitt | oh, I see | |
| 21:27:22 | dansmith | oh no, that can co straight to the core as far as I'm concerned | |
| 21:27:37 | dansmith | although we might break some things by doing that, but they're broken now silently | |
| 21:27:38 | melwitt | the setting of non-field properties hasn't confused me before, it's only the passing them in __init__ that has | |
| 21:27:47 | melwitt | (for whatever that's worth :P) | |
| 21:27:58 | dansmith | I guess some library types might want a warning instead of a failure in a minor/rev release or something | |
| 21:28:01 | dansmith | melwitt: ah okay | |
| 21:28:09 | sean-k-mooney[m] | Melwitt probably 2 mixins. One for init check that could be in base nova object and one for all assignments that would not be in base nova object | |
| 21:28:09 | mikal | mriedem: you're welcome | |
| 21:28:13 | dansmith | like, mark it as deprecated, warn and then fail later | |
| 21:28:39 | dansmith | sean-k-mooney[m]: I dunno, we don't actually set the values from init if they're not in fields, do we? | |
| 21:29:05 | dansmith | ah I guess we do | |
| 21:29:09 | dansmith | well, I dunno, | |
| 21:29:21 | dansmith | a warning initially would be totally fine on the base object IMHO | |
| 21:29:31 | dansmith | and the make it required in a year on a version bump | |
| 21:29:44 | sean-k-mooney[m] | Dansmith apparently in the tests mel found | |
| 21:29:59 | dansmith | sean-k-mooney[m]: yeah, I just didn't think it was actually _setting_ it, but I see it is | |
| 21:30:18 | sean-k-mooney[m] | Yeah more warning in test output :) | |
| 21:30:28 | dansmith | well, then we fix it :) | |
| 21:30:44 | dansmith | we could do it in our nova object to pre-fix everything | |
| 21:31:21 | melwitt | yeah, I'm not clear on if everyone would want to warn/fail if they call init with non-existent fields. maybe someone likes a shortcut like that | |
| 21:31:52 | dansmith | *shrug* I guess | |
| 21:31:58 | sean-k-mooney[m] | Ya im sure you could phase it in gradually | |
| 21:32:13 | dansmith | it was my intent to allow free-form properties, not to allow anything in init(), that's my bias I guess :) | |
| 21:32:56 | melwitt | yeah, I had the same thinking too as a user of objects | |
| 21:33:20 | sean-k-mooney[m] | Melwitt well if they ever call make compatible or serialise the object those non fields are lost so you still have to be careful with thwm or you will get weird bugs | |
| 21:34:09 | melwitt | yeah. I agree it makes sense to make it more intentional when setting free-form properties, i.e. you have to do that outside of init | |
| 21:34:19 | melwitt | being that those will be lost as you said in those situations | |
| 21:35:38 | sean-k-mooney[m] | Yep. We don't really use the free form attributes that often but they can be useful sometimes | |
| 21:42:37 | openstackgerrit | Merged openstack/nova stable/pike: Handle volume-backed instances in IsolatedHostsFilter https://review.openstack.org/543603 | |
| 21:42:44 | openstackgerrit | Merged openstack/nova stable/pike: Fix docs for IsolatedHostsFilter https://review.openstack.org/543604 | |
| 21:42:51 | mriedem | so uh, | |
| 21:42:52 | mriedem | https://bugs.launchpad.net/nova/+bug/1755392 | |
| 21:42:53 | openstackgerrit | Merged openstack/nova master: Remove useless run_periodic_tasks call in ClientRouter https://review.openstack.org/554381 | |
| 21:42:53 | openstack | Launchpad bug 1755392 in OpenStack Compute (nova) "resize the instance is fail" [Undecided,New] | |
| 21:43:06 | mriedem | i thought there was a thing where you couldn't resize/migrate ephemeral/swap disks? | |
| 21:43:09 | mriedem | am i making that up? | |
| 21:45:41 | openstack | Launchpad bug 1755266 in OpenStack Compute (nova) "Instance resize with swap on cinder volume fails" [Undecided,New] | |
| 21:45:41 | mriedem | similar https://bugs.launchpad.net/nova/+bug/1755266 | |
| 22:02:09 | openstackgerrit | Julia Kreger proposed openstack/nova master: WIP: Add microversion to ironic client wrapper call https://review.openstack.org/554762 | |
| 22:02:34 | sean-k-mooney[m] | mriedem: i believe its undefined behavior if you change the number of ephemeral disk on resize. Migration of swap is fine. Resize is not | |
| 22:03:32 | sean-k-mooney[m] | Actually swap resize should be fine since we basically reboot | |
| 22:04:40 | openstackgerrit | Michael Still proposed openstack/nova master: Use a pythonic delete. https://review.openstack.org/554792 | |
| 22:04:40 | openstackgerrit | Michael Still proposed openstack/nova master: Remove duplicative implementation of temporary directories. https://review.openstack.org/554791 | |
| 22:04:41 | openstackgerrit | Michael Still proposed openstack/nova master: Use a pythonic delete, with a retry. https://review.openstack.org/554793 | |
| 22:05:30 | melwitt | mriedem, dansmith: finally tracked down what's going on in the bug where after upgrading the pike, if there are service records with no uuid (from the N-1 version), they get the "TypeError: Can't upgrade a READER transaction to a WRITER mid-transaction" error | |
| 22:05:45 | melwitt | explained it here https://bugs.launchpad.net/nova/+bug/1746509/comments/9 | |
| 22:05:46 | openstack | Launchpad bug 1746509 in OpenStack Compute (nova) "TypeError: Can't upgrade a READER transaction to a WRITER mid-transaction" [Medium,Confirmed] | |
| 22:06:06 | mriedem | sean-k-mooney[m]: i seem to remember diana clarke trying to fix something wrt swap disks and resize, found https://github.com/dianaclarke/openstack-notes/wiki/resize-disks but not the thing she was trying to fix | |
| 22:06:09 | melwitt | the good news is it's no longer a bug in queens or rocky, but it is a bug in pike | |
| 22:08:01 | melwitt | is it cool if I propose a fix only for pike? how does that usually work? | |
| 22:08:44 | dansmith | melwitt: ah yeah that sort of nesting is exactly what I was saying would have to happen, but was skeptical of it existing | |
| 22:08:54 | dansmith | melwitt: so.. glad you traced it all the way down :) | |
| 22:09:05 | cfriesen | mriedem: what about this? https://bugs.launchpad.net/nova/+bug/1552777 | |
| 22:09:07 | dansmith | melwitt: mriedem would know better than me, but we've had to do that before, IIRC | |
| 22:09:07 | openstack | Launchpad bug 1552777 in OpenStack Compute (nova) "resizing from flavor with swap to one without swap puts instance into Error status" [Medium,In progress] - Assigned to Kam Nasim (knasim-wrs) | |
| 22:09:20 | mriedem | reminds me of https://review.openstack.org/#/c/507854/ | |
| 22:10:07 | cfriesen | mriedem: though it looks like that one went away in pike | |
| 22:10:43 | sean-k-mooney[m] | Cfriesen resize with swap is picky. I think it works more or less now | |
| 22:10:50 | melwitt | dansmith: yeah, initially I couldn't repro it (as expected) with only a service query. but yeah, got to the bottom of it :) I think it could be easily fixed by just splitting the _make_instance_list call out from under the _get_by_filters_impl, that is, move it to get_by_filters | |
| 22:11:30 | dansmith | melwitt: okay, I just read your comment but I didn't go look at the (old) code to see, but.. sounds good? :) | |
| 22:11:52 | melwitt | dansmith: yeah, just chattering aloud. I'm excited that this makes sense now | |
| 22:12:28 | dansmith | melwitt: I will put a dan dummy in my chair who will continue to listen to your chattering. he doesn't type though, so just assume he's saying "uh huh, yeah, oh. sounds good. uh huh, yeah..." | |
| 22:12:47 | melwitt | haha | |
| 22:13:03 | dansmith | mriedem: and no snide comments from you mister. | |
| 22:13:42 | openstackgerrit | Jay Pipes proposed openstack/nova-specs master: Standardize CPU resource tracking https://review.openstack.org/555081 | |
| 22:15:51 | mikal | pick me pick me! | |
| 22:15:59 | mriedem | i'd have to see the proposed fix | |
| 22:16:01 | mikal | Basically snide comments is all I do now. | |
| 22:17:36 | melwitt | Snide Comment Czar | |
| 22:17:38 | mriedem | melwitt: so you want to move this call to _make_instance_list from here https://github.com/openstack/nova/blob/9465d1c/nova/objects/instance.py#L1235 to right after _get_by_filters_impl is called here https://github.com/openstack/nova/blob/9465d1c/nova/objects/instance.py#L1243 ? | |
| 22:17:51 | mikal | I would accept that job | |
| 22:18:01 | mikal | Or anything entitled "Old man shakes fist at clouds" | |
| 22:18:04 | melwitt | mriedem: yeah, that's what I'm trying right now. already have the func test written. let's see if it works | |
| 22:19:56 | melwitt | yay | |
| 22:23:50 | mriedem | so why couldn't we just also make this change on master and backport it? | |
| 22:24:01 | mriedem | even if it's not a problem on master, | |
| 22:24:06 | mriedem | would changing the same code cause any problems? | |
| 22:24:50 | melwitt | oh yeah, that's a better idea actually | |
| 22:25:08 | melwitt | when I first asked, I wasn't thinking the code was going to be the same on master but it is | |
| 22:26:15 | mriedem | dansmith: it would be best if your dan dummy looked like https://www.youtube.com/watch?v=_WQfZYacEAw | |
| 22:26:20 | melwitt | I'll have to rewrite this func test a bit to be a regression func test that will still fail on master. not a big deal | |
| 22:26:44 | dansmith | mriedem: what else would it look like? | |
| 22:26:44 | mriedem | melwitt: writing a test on master to fail for a thing that doesn't fail on master...breaks my brain | |
| 22:27:10 | mriedem | unless you're not going to reproduce it through the API | |
| 22:27:22 | mriedem | but just through the object methods directly | |