Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-06-11
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.
17:38:08 openstackgerrit Matt Riedemann proposed openstack/nova master: placement: always create consumer records https://review.openstack.org/567678
17:38:09 openstackgerrit Matt Riedemann proposed openstack/nova master: add consumers generation field https://review.openstack.org/557958
17:38:10 openstackgerrit Matt Riedemann proposed openstack/nova master: placement: Allocation.consumer field https://review.openstack.org/565405
17:38:11 openstackgerrit Matt Riedemann proposed openstack/nova master: rework allocation handler _allocations_dict() https://review.openstack.org/565407
17:38:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Add a microversion for consumer generation support https://review.openstack.org/565604
17:38:13 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix nits from change Id609789ef6b4a4c745550cde80dd49cabe03869a https://review.openstack.org/574324
17:38:23 mriedem efried: dansmith: ok rebased ^
17:38:40 mriedem my find/replace 1.27 -> 1.28 fu in the actual microversion change might have missed something
17:38:44 mriedem but tests are happy
18:10:10 openstackgerrit Dan Smith proposed openstack/nova stable/pike: Change consecutive build failure limit to a weigher https://review.openstack.org/573248
18:12:18 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
18:14:05 openstackgerrit Brianna Poulos proposed openstack/nova master: Implement certificate_utils https://review.openstack.org/479949
18:14:06 openstackgerrit Brianna Poulos proposed openstack/nova master: Plumb trusted_certs through libvirt driver image paths https://review.openstack.org/561262
18:14:07 openstackgerrit Brianna Poulos proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204
18:14:08 openstackgerrit Brianna Poulos proposed openstack/nova master: Add notification support for trusted_certs https://review.openstack.org/563269
18:14:09 openstackgerrit Brianna Poulos proposed openstack/nova master: Add certificate validation docs https://review.openstack.org/560158
18:17:49 artom In situations like this I really wish Python had static typing
18:18:28 artom ... or that OpenStack was written in Java ;)
18:18:29 mriedem melwitt: reminder, you can release r-2 now https://launchpad.net/nova/+milestone/rocky-2
18:18:40 mriedem based on https://review.openstack.org/#/c/573495/ it looks like it was june 8
18:30:28 openstackgerrit Matt Riedemann proposed openstack/nova master: xenapi: drop deprecated vif_driver config option https://review.openstack.org/567877
18:35:31 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
18:49:07 openstackgerrit Merged openstack/nova stable/queens: Fix interpretation of max_attempts for scheduling alternates https://review.openstack.org/573254
18:55:21 mriedem dansmith: i have beat your heart down on that patch as requested
18:56:36 dansmith aye
18:57:28 melwitt mriedem: what does it mean to release r-2 wrt to launchpad? oh, I see a "create release link"
18:58:15 mriedem just paperwork
18:58:34 mriedem but see the storyboard thread in the ML about why people care
18:59:21 melwitt okay, I just didn't know to/how to do it. my bad
19:01:43 mriedem artom: re https://review.openstack.org/#/c/572790/ i don't think we need your tempest patch
19:01:47 mriedem i'm not sure why you created it
19:01:57 mriedem because did you see that ^ depends on https://review.openstack.org/#/c/573025/ ?
19:02:12 artom mriedem, we don't? I mean, to actually test the "positive" case where we refuse the swap
19:02:29 mriedem you mean negative?
19:03:07 artom mriedem, I genuinely have no idea :/
19:03:17 artom mriedem, the case where we assertRaises(400)
19:03:18 artom That one.
19:03:18 mriedem testing error conditions in tempest are called negative tests
19:03:26 artom OK, negative then :)
19:05:17 melwitt mriedem: okay, done. thank you for letting me know about it https://launchpad.net/nova/+milestone/rocky-2
19:05:45 melwitt I removed the rocky-2 milestone target from https://blueprints.launchpad.net/nova/+spec/preemptible-instances also
19:08:02 mriedem artom: ok here is your 400 http://logs.openstack.org/09/573809/2/check/nova-multiattach/ebd4e2a/job-output.txt.gz#_2018-06-09_14_49_19_637410
19:08:18 mriedem PUT http://104.130.136.141/compute/v2.1/servers/57ba4f52-8076-45e2-b6ac-14cf789b6926/os-volume_attachments/8361a693-e7c6-4a9b-8303-c303d5fbdad5
19:08:23 mriedem Body: {"volumeAttachment": {"volumeId": "d9636fa2-78a3-468a-b56a-9e6e2d6f14f6"}}
19:08:48 artom mriedem, yep, which is why I posted that patch
19:08:56 artom mriedem, you don't have to merge it if you don't have to :)
19:09:07 artom Although the cleanup failure is interesting
19:09:11 mriedem i'm not QA core so i can't merge it..
19:09:28 artom Either I'm really thick and doing something wrong, or refusing to swap a volume legit makes it undetachable
19:10:32 mriedem where is the cleanup code that is waiting for the volume to be available after the swap failure,
19:10:41 mriedem the code is doing the right thing, it leaves the old volume attached (in-use)
19:11:04 mriedem the trace in the tempest failure doesn't make sense to me
19:11:14 artom Same here, I can't understand it
19:11:31 artom I tried printing the full stacktrace manually, but got just a bunch of test runner stuff
19:11:46 artom So I can't even pinpoint where it's from - I *think* it's the cleanup phase

Earlier   Later