| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-08 | |||
| 13:10:26 | bauzas | and when looking at code, we're just getting the dict | |
| 13:10:41 | claudiub | bauzas: ah, cool, ty. It's in merge conflict though. :) https://review.openstack.org/#/c/471790/ | |
| 13:10:52 | bauzas | sfinucan: so I'm pretty sure we're not exactly verifying the order | |
| 13:10:59 | sfinucan | bauzas: Agreed. You can close it as invalid, IMO | |
| 13:11:09 | bauzas | sfinucan: will do | |
| 13:11:21 | bauzas | if the user cares about the order, that would be a feature to me | |
| 13:11:32 | bauzas | and good luck with that. | |
| 13:11:43 | bauzas | claudiub: oh good point, will rebase it | |
| 13:15:40 | mriedem | bauzas: sfinucan: there is some network sorting in the neutronv2.api code | |
| 13:15:59 | mriedem | https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L2522 | |
| 13:16:40 | mriedem | that's for networks, but when given ports we get the networks for those ports too so maybe that's what they are thinking | |
| 13:19:32 | bauzas | humpg | |
| 13:20:30 | bauzas | mriedem: so there are two possible outcomes | |
| 13:20:55 | bauzas | mriedem: either we say "sorry, but we don't sort NICs when you have both network and port requests" | |
| 13:21:07 | bauzas | mriedem: or we say it's a valid bug that requires resolution | |
| 13:22:47 | mriedem | i don't want to say anything | |
| 13:24:40 | bauzas | mriedem: so I'll keep the bug open | |
| 13:24:43 | bauzas | :) | |
| 13:24:52 | bauzas | anyone can just look at it | |
| 13:31:16 | mriedem | i replied with some details too and code links | |
| 13:31:30 | mriedem | looks like we do only preserve the order of requested networks, not the networks associated with a requested port | |
| 13:31:49 | mriedem | adding the port.network_id to the list of ordered_networks would be easy, but would probably break something else, | |
| 13:32:03 | mriedem | like i bet that would end up having it create another port on that other port's network or something | |
| 13:32:07 | mriedem | it just gets very complicated | |
| 13:32:29 | mriedem | we could build a 3rd list of networks :) | |
| 13:41:33 | openstackgerrit | Matt Riedemann proposed openstack/nova master: libvirt: handle missing rbd_secret_uuid from old connection info https://review.openstack.org/472266 | |
| 13:42:32 | openstackgerrit | Matt Riedemann proposed openstack/nova master: libvirt: handle missing rbd_secret_uuid from old connection info https://review.openstack.org/472266 | |
| 13:42:33 | mriedem | sdague: here is that fix for the rbd secret uuid ^ | |
| 13:42:47 | mriedem | sdague: which reminds me of something i've thought about for awhile, | |
| 13:43:02 | mriedem | which is adding an admin api to force a bdm.connection_info refresh | |
| 13:43:26 | openstackgerrit | Alex Szarka proposed openstack/nova master: Add method for verify multiple versioned notifications https://review.openstack.org/465526 | |
| 13:43:36 | mriedem | since if you changed things somewhere on the cinder side, or in your underlying storage, i think you have to detach the volume and re-attach it, or reboot the instance to refresh the connection info | |
| 13:43:48 | mriedem | this has come up when people recycle secrets in ceph | |
| 13:44:00 | mriedem | and the only way to update the existing bdm connection info is via straight db hackery | |
| 13:45:43 | mriedem | so we could do something as simple as an rpc cast to the compute, call os-initialize_connection in cinder to get the connection_info and then update the bdm record | |
| 13:45:59 | mriedem | melwitt: haven't you and i talked about something like this before? ^ or at least a periodic task | |
| 13:46:29 | mriedem | the problem is i bet initialize_connection in cinder isn't the same (idempotent) for all volume backends | |
| 13:49:15 | sdague | mriedem: could that come from cinder in the admin event interface? | |
| 13:49:32 | sdague | it would be best if cinder was the one that said to do that when needed | |
| 13:52:18 | mriedem | it could | |
| 13:52:29 | mriedem | but, | |
| 13:52:48 | mriedem | send what? just telling us to do it? because that api doesn't take something like a big json blob | |
| 13:53:15 | mriedem | we could get a "refresh-connection-info" event from cinder though yeah, like we do for network-changed in neutron | |
| 13:57:02 | sdague | yeh, that's kind of what I was thinking | |
| 13:57:24 | sdague | I guess it should be instance oriented? | |
| 13:58:22 | mriedem | yes it would be | |
| 13:58:26 | mriedem | just sent to the ML | |
| 13:58:33 | mriedem | might be a duplicate idea i've already brought up before | |
| 13:58:35 | mriedem | anyway, bbiab | |
| 14:13:37 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove cpuset_reserved from __init__ of InstanceNUMATopology https://review.openstack.org/466030 | |
| 14:13:37 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove related pinning from __init__ of InstanceNUMATopology https://review.openstack.org/408242 | |
| 14:13:38 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: remove pagesize from __init__ of InstanceNUMATopology https://review.openstack.org/408241 | |
| 14:18:56 | sfinucan | dansmith: I think I finally figured out the remaining kinks with the above ^^^ patches, if you want to take a look at some point | |
| 14:20:06 | openstackgerrit | Hironori Shiina proposed openstack/nova master: Ironic: Check a last error of power action https://review.openstack.org/470754 | |
| 14:20:20 | bauzas | sfinucan: I think the consensus is not to have __eq__ | |
| 14:20:42 | dansmith | sfinucan: nak on the __eq__ yeah :) | |
| 14:20:49 | sfinucan | bauzas: Oh? Any particular reason? | |
| 14:20:59 | bauzas | sfinucan: when you want to verify o.vo objects, you need to verify the fields | |
| 14:21:39 | sfinucan | bauzas: Yeah - we do that in nova/objects/pci_device.py and nova/objects/numa.py | |
| 14:21:51 | sfinucan | ...and we could also do with it in nova/objects/instance_numa_topology.py | |
| 14:22:09 | bauzas | sfinucan: there are multiple reasons, one being that objects can be semantically identical without being the same, like if they're passed over RPC | |
| 14:24:05 | sfinucan | bauzas: You mean there'd be differences other than timestamps and the likes? | |
| 14:25:30 | bauzas | well, take an object, pass it as an argument to a remote method, you won't get the same object at the end | |
| 14:29:52 | sfinucan | hmm, well I'm going to try something to see if it makes sense | |
| 14:30:01 | sfinucan | it might not, but then I'll know :) | |
| 14:31:19 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP/POC Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 14:31:20 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Skip microversion discovery check for update/delete volume attachments https://review.openstack.org/469668 | |
| 14:31:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use 3.27 microversion when creating new style volume attachments https://review.openstack.org/469609 | |
| 14:31:21 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use microversions for new style volume attachments https://review.openstack.org/469579 | |
| 14:31:44 | mriedem | sdague: could you add the bottom 3 changes here to your review queue? ^ | |
| 14:31:50 | mriedem | the new cinder stuff depends on that | |
| 14:33:03 | sdague | yeh, can do | |
| 14:33:21 | sdague | it will be a minute, in the middle of unrelated stuff | |
| 14:34:24 | mriedem | sure, thanks | |
| 14:34:44 | mriedem | dansmith: looks like your devstack fleet week patch needs to depend on the check min service version in all cells patch | |
| 14:34:51 | mriedem | http://logs.openstack.org/94/436094/32/check/gate-tempest-dsvm-neutron-full-ubuntu-xenial/0d3a7e2/console.html#_2017-06-08_13_33_37_461957 | |
| 14:34:54 | mriedem | i think... | |
| 14:38:03 | bauzas | sfinucan: the consensus is basically about providing a general __eq__ rule | |
| 14:38:18 | bauzas | sfinucan: semantically, the equivalence really depends on the object type | |
| 14:38:39 | bauzas | sfinucan: like for example, you could assume some fields are irrelevant for that | |
| 14:38:43 | sfinucan | Yeah, that sounds fair. It does seem pretty contextual | |
| 14:39:21 | bauzas | the same goes with __hash__ | |
| 14:39:45 | bauzas | since objects can be very different, we try hard to not define __hash__ on a global basis | |
| 14:40:50 | bauzas | sfinucan: that said, you know we have an obj_equal_prims() right? | |
| 14:41:22 | sfinucan | bauzas: I've just spotted that, alright | |
| 14:41:41 | bauzas | cool | |
| 14:41:58 | bauzas | that's the only thing we can really do on a general basis | |
| 14:42:05 | bauzas | if primitives are equal, fine | |
| 14:42:29 | bauzas | but standing that obj_1 == obj_2 is a total different deal | |
| 14:42:47 | bauzas | same goes for obj1 in an objectlist | |
| 14:46:34 | dansmith | mriedem: aye, not sure why I wasn't hitting that before | |
| 14:47:14 | mriedem | dansmith: i think the test was recently-ish merged | |
| 14:47:22 | mriedem | well, | |
| 14:47:23 | dansmith | ah | |
| 14:47:36 | mriedem | and it required the ssh validation stuff being turned on, which happened globally about 2 weeks ago | |
| 14:47:50 | dansmith | mriedem: thanks for fixing the bottom patch, I just realized I needed to get back to it | |
| 14:48:10 | dansmith | is the merge conflict because of all the infra drama yesterday? | |
| 14:48:17 | mriedem | yes, i've rechecked | |
| 14:48:25 | mriedem | well, i assume it is, i haven't tried rebasing | |
| 14:48:26 | dansmith | cool | |