Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-06-11
15:46:00 mriedem i realize it would be backward incompatible, and upgrade release note, etc
15:46:14 dansmith mriedem: I don't know what that means
15:46:26 mriedem which part?
15:46:33 dansmith oh you mean [0,4] for either return value, sure,
15:46:36 dansmith but it has to change
15:46:40 mriedem right
15:46:56 mriedem if you're just saying != 0 in your tooling, that's kind of on you
15:47:06 mriedem anyway, i don't mean to open a can of worms
15:47:13 dansmith well, you did
15:47:16 dansmith worms. everywhere.
15:47:50 dansmith although efried is like the child that just starts throwing cans on the floor of the supermarket aisle
15:47:59 dansmith denting cans left and right until one opens
15:48:10 efried lol
15:48:20 efried like a child in my ignorance, at least.
15:48:51 efried My vote is that we merge this as is and (maybe) figure out what to do about that other thing later.
15:49:14 mriedem sure, throw a TODO in there about the rc=2 idea i guess
15:50:29 openstackgerrit Dan Smith proposed openstack/nova master: Document the internal online_migrations function behaviors https://review.openstack.org/574268
15:50:34 mriedem efried: can you double check https://review.openstack.org/#/c/486475/36/doc/source/admin/configuration/hypervisor-xen-api.rst and if i'm right, i'll update the patch for my comments and respin so we can flush it
15:50:53 efried ...
15:51:04 mriedem flush from the runways spot i mean
15:53:46 efried mriedem: You're right. History there was that they were going to switch the default, but I said nay. They wound it back, but missed a couple spots. Good eye.
15:54:01 mriedem ok, i figured that was the case
15:54:05 mriedem want to default to the new shiny
15:55:11 efried mriedem: Was I right to disallow that?
15:56:00 mriedem shrug
15:56:05 mriedem we do change defaults from time to time
15:56:21 mriedem if you were explicitly using the old thing, it's no upgrade issue, otherwise it is if you don't want that new default
15:56:30 mriedem but new installs get the new thing out of the box
15:56:37 efried mm
15:56:45 openstackgerrit Zack Cornelius proposed openstack/nova master: Refactor libvirt get_memory_used_mb() https://review.openstack.org/571030
15:56:46 openstackgerrit Zack Cornelius proposed openstack/nova master: Implement file backed memory for instances in libvirt https://review.openstack.org/567876
15:56:49 mriedem if the xenapi team wants the vdi_remote_stream stuff to be the default at some point, their docs should probably say something to that effect, like "this is the recommended image handler"
15:57:01 mriedem maybe direct is just the simplest / easiest to setup?
15:57:30 efried Well, reading back (PS29..34) it looks like it was more of an internal decision to revert the default. But I registered a not-having-liked-it comment too.
15:57:51 efried in any case, we're at not changing the default at this point, so your comment is apt.
16:00:35 openstackgerrit Jan Gutter proposed openstack/os-vif master: [WIP] Add support for generic offload representors https://review.openstack.org/572081
16:00:59 openstackgerrit Matt Riedemann proposed openstack/nova master: XenAPI: define a new image handler to use vdi streaming https://review.openstack.org/486475
16:01:00 openstackgerrit Matt Riedemann proposed openstack/nova master: XenAPI: update the document related to vdi streaming https://review.openstack.org/568444
16:01:01 openstackgerrit Matt Riedemann proposed openstack/nova master: DNM: default [xenserver]/image_handler to vdi_remote_stream https://review.openstack.org/574318
16:01:23 mriedem efried: ok i'll wait to +2 until that top test patch passes the 3rd party ci
16:01:46 efried mriedem: ack, poke me then and I'll +A
16:04:52 mriedem dansmith: on the consumer allocs patch, i think you were asking for this test https://review.openstack.org/#/c/567678/11/nova/tests/functional/api/openstack/placement/gabbits/ensure-consumer.yaml
16:05:03 mriedem creates an allocation at 1.7 so the consumer is auto-generated
16:05:06 mriedem using the conf
16:05:16 dansmith yup
16:05:57 mriedem ok +W
16:14:47 openstackgerrit Curt Moore proposed openstack/nova master: Add ability to download Glance images into the libvirt image cache via RBD https://review.openstack.org/574301
16:15:33 openstackgerrit Curt Moore proposed openstack/nova master: Add ability to download Glance images into the libvirt image cache via RBD https://review.openstack.org/574301
16:17:32 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix nits from change Id609789ef6b4a4c745550cde80dd49cabe03869a https://review.openstack.org/574324
16:20:19 openstackgerrit Chris Dent proposed openstack/nova stable/pike: Ensure resource class cache when listing usages https://review.openstack.org/574327
16:20:37 mriedem dansmith: don't think i didn't see you give me that todo
16:57:34 dansmith mriedem: I was hoping you would notice :)
17:22:33 mdbooth mriedem: Saw your note about access_mode, but note that we're testing attach_mode. Started looking at cinder to see how robustly they're related but didn't finish.
17:26:46 mriedem there are likely glaring inconsistencies; access_mode is what comes back in the connection_info dict,
17:27:00 mriedem attach_mode is something on the attachment record itself, but you can't set it when creating or updating an attachment
17:27:05 mriedem https://developer.openstack.org/api-ref/block-storage/v3/#create-attachment
17:27:08 mriedem not directly anyway
17:27:25 mriedem https://developer.openstack.org/api-ref/block-storage/v3/#update-an-attachment
17:27:43 mriedem today read-only access for attachments is controlled via the readonly status on the volume,
17:27:47 mriedem which is via it's volume admin metadata
17:27:52 mriedem which is what i did in the dependent tempest change
17:28:45 mriedem yeah you can't create or update an attachment record and specify the attach_mode directly https://github.com/openstack/cinder/blob/master/cinder/api/schemas/attachments.py
17:29:04 mriedem however, you can set it's attach_mode via the 'mode' key in the 'connector' dict when creating/updating the attachment
17:29:07 mriedem it's all not very REST-y
17:29:23 mriedem https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L4376
17:29:43 openstackgerrit Artom Lifshitz proposed openstack/nova master: Add InstanceNUMATopology to LibvirtLiveMigrateData https://review.openstack.org/566398
17:29:44 openstackgerrit Artom Lifshitz proposed openstack/nova master: WIP: LM: re-fit topology and claim resources on dest https://review.openstack.org/567242
17:29:45 openstackgerrit Artom Lifshitz proposed openstack/nova master: Service version check for NUMA live migration https://review.openstack.org/566723
17:30:10 mriedem mdbooth: i planned on cleaning up the _translate_attachment_ref stuff in a follow up
17:30:15 mriedem since it's not necessary for the backport
17:31:34 mdbooth mriedem: I was thinking of the issue of whether ro attachments are really ro.
17:31:45 mdbooth Which they might be, but it's not at all obvious.
17:32:10 mdbooth Bizarrely, that code is adding attach_mode to connection_info, but I couldn't find any use of it.
17:32:10 mriedem if they aren't, that's a bug in cinder yes?
17:32:23 mriedem right, attach_mode doesn't need to be in the connection_info
17:32:27 mdbooth But as you pointed out, we *are* using access_mode in nova/os-brick.
17:32:29 mriedem since access_mode is there and that's what the volume driver uses
17:32:33 mriedem yes
17:32:47 mriedem in a follow up, i'll move attach_mode back top-level on the attachment record
17:33:07 mdbooth So, what's the relationship between access_mode and attach_mode, and why do we both putting the latter in connection_info if we don't seem to us it?
17:33:08 mriedem where it belongs given https://developer.openstack.org/api-ref/block-storage/v3/#show-attachment-details
17:33:29 mriedem that is an excellent question for our friends in the -cinder channel
17:33:44 mdbooth And if access_mode and attach_mode don't correlate 100%, is our assumption that attach_mode==ro means a readonly attachment a safe one?
17:33:45 mriedem i assume in part to eventually decouple from needing to stash stuff in connection_info
17:33:51 mriedem that is not host/backend specific
17:34:08 mriedem mdbooth: again, that would be a bug in cinder if they aren't
17:34:29 artom We could just be ruthless about it and block swap if attachments (of any kind) are > 1
17:34:32 mdbooth mriedem: If that's true, that's good enough for me.
17:34:49 mdbooth mriedem: It was just weird enough for me to flag it.
17:34:55 mriedem i agree it's weird
17:35:22 mriedem there are some things missing from the attachments API, like the ability to actually create/update an attachment and set attach_mode on the record directly
17:35:30 mriedem rather than indirectly via connector['mode']
17:36:05 mriedem AND
17:36:28 mriedem the volume admin metadata readonly stuff needs to be decoupled from the actual attachments, so attach mode is per-attachment
17:36:30 mriedem not per-volume
17:36:52 mriedem but i don't think jgriffith et al got that far when plumbing this into cinder, but the TODOs and NOTEs are in the code
17:37:04 mriedem e.g. https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L4384
17:37:29 mdbooth mriedem: Actually I did get far enough to read some of that
17:37:46 mdbooth It looks like attached_mode in admin_metadata is basically a temporary stash
17:38:07 mdbooth So it's admin_metadata on the volume, but it only lives during the creation of a single attachment.

Earlier   Later