Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-04-03
11:32:17 ildikov johnthetubaguy: I think we went through the detach cases when we removed check_attach and I think we got a good coverage, but I need to look into the code
11:32:31 johnthetubaguy ildikov: so lets think about the new flow quickly
11:32:38 johnthetubaguy ildikov: what call does the checks?
11:33:10 ildikov johnthetubaguy: you need to find the attachment_id in the bdm to detach
11:33:38 johnthetubaguy ildikov: that doesn't tell us what the state of that attachment is, we could be out of sync
11:34:02 ildikov johnthetubaguy: the attachment state is Cinder's responsibility
11:34:07 johnthetubaguy ildikov: we still need to check that the user has asked us to detach a volume that is currently attached to the instance
11:34:21 johnthetubaguy ildikov: I agree in principal, but we don't have the APIs for it in the new flow
11:34:48 ildikov johnthetubaguy: Cinder should do that either as part of attachment_delete or we can look into other options
11:35:10 ildikov johnthetubaguy: for attach we can create an empty attachment to lock the volume and not for the status checks either
11:35:16 johnthetubaguy ildikov: so attachment_delete is after the API has returned, and after we have attempted to remove the volume
11:36:24 johnthetubaguy ildikov: so we need a spec in Nova to describe all the multi_attach loose ends
11:37:19 ildikov johnthetubaguy: we need that for other reasons too
11:37:27 johnthetubaguy ildikov: this might be one of those, be we need to be clear what checks are replacing our existing guard rails, and describe how you don't break the current API behaviour for Nova
11:37:53 ildikov johnthetubaguy: I would like to just clean up the code and solve the problems that we really have at the right place
11:38:11 johnthetubaguy ildikov: we have the same goal, the problem is your patch breaks our API
11:38:33 ildikov johnthetubaguy: even for the non-dteach cases?
11:38:41 ildikov *non-detach
11:38:52 johnthetubaguy ildikov: its breaks the detach flow only, AFAIK
11:39:26 johnthetubaguy when you call detach with a volume attach on a different instance, we end up doing some very strange things to the volume the user passed into our API
11:39:27 ildikov johnthetubaguy: bah, sorry I meant non-multi-attach
11:39:37 johnthetubaguy yes, it breaks all regular detaches
11:40:05 johnthetubaguy we now report errors in different places, for some cases, other cases I think just break horribly and cause possible data corrupution
11:40:10 ildikov I wonder whether that check will be the first place to recognize that
11:40:18 johnthetubaguy I attempted to describe that in the review comments
11:40:39 johnthetubaguy ildikov: don't know
11:40:47 ildikov I saw that, but if I don't mess with the code purely because of multi-attach that check wouldn't be there
11:41:20 ildikov it's not part of the regular detach flow by design
11:41:58 johnthetubaguy ildikov: so I think it would be much easier for me to review this if we can write up your thinking around the change, and how all these checks should be done
11:42:32 johnthetubaguy ildikov: a few notes in the git commit message might be the best way to start it, but honestly, it feels like it needs a spec, because its API sensitive
11:43:08 ildikov and even for multi-attach case as we have the attachment_id stored in the bdm that approach should help
11:43:24 johnthetubaguy ildikov: I am not sure how that helps the old code flow
11:43:31 johnthetubaguy that would still be broken
11:43:43 ildikov johnthetubaguy: I will look into that later, on a conference this week
11:44:19 ildikov johnthetubaguy: I will test the old flow case and if I hit that error message I will look into how to fix that
11:44:32 ildikov johnthetubaguy: but the status checks should go regardless
11:44:44 ildikov johnthetubaguy: that shouldn't be in Nova
11:45:23 johnthetubaguy ildikov: its the error handling where we hit problems, bad volume-uuids, volume-uuids that are not attached, volume-uuids attached to other instances, volume-uuids for volumes that are already part way through the detach process, we want to give the user an API error, rather than a 200
11:45:59 johnthetubaguy ildikov: as I said, agreed with the goal, just the new flow doesn't have an API for that right now, as I see it, and it seems the old flow still needs these checks
11:47:43 ildikov johnthetubaguy: I will look into that, I cannot tell you more now from te top of my head about this
11:48:06 johnthetubaguy ildikov: that no problem, I just think we need to get this all written down / agreed first
11:49:01 ildikov johnthetubaguy: I just really hope we can clean up the code more, if we can't I'm not going make anything more complicated just to delete 10-20 lines
11:50:08 johnthetubaguy ildikov: me too, I am just trying to understand how this doesn't break the existing flow
11:50:16 johnthetubaguy the error handling in it
11:50:31 ildikov sure, let me look into that later
11:50:57 ildikov thanks for raising your concerns, the goal isn't to break the API
11:51:05 johnthetubaguy ildikov: I think this might actually just be when the Cinder and Nova state drift appart, but thats possible with force-detach
11:51:34 johnthetubaguy ildikov: we do check with the BDM further up (that logic really shouldn't be in there, but it is): https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/volumes.py#L431
11:53:44 ildikov johnthetubaguy: so you check the volume_id in the BDM
11:54:52 ildikov johnthetubaguy: if it does not match you don't do anything
11:56:54 johnthetubaguy ildikov: yeah, I missed all that logic, it certainly protects against the worse bits of what I was worried about
11:57:07 johnthetubaguy ildikov: hence the fresh link above
11:57:48 ildikov johnthetubaguy: it would've been weird to be honest if that checks points that out first as it wasn't part of a bugfix or anything like that
11:58:10 ildikov s/checks/check/
12:19:40 markus_z Is anyone aware of issues with centos nodes and oslo.serialization? Our s390x CI is centos based and goes mad since a few day, other upstream test jobs fail too: http://logstash.openstack.org/#dashboard/file/logstash.json?from=7d&query=message%3A%5C%22Malformed%20request%20body%5C%22
12:20:11 markus_z oslo.serialization 2.18 merged to u-c a a few days ago: https://github.com/openstack/requirements/commit/f0952e715c247ac570ed86341580bbe82fd85760
12:20:51 markus_z And the requirements project doesn't have a centos job: https://review.openstack.org/#/c/450508/
12:22:22 markus_z I checked all test jobs of that logstash query, all with the same issue, at the same "server tagging" tests.
12:22:43 markus_z Might be the PUT with an empty body.
12:25:25 openstackgerrit Pushkar Umaranikar proposed openstack/nova-specs master: Stop using Neutron admin token https://review.openstack.org/439890
12:26:09 markus_z And, IIUC, json decode issue won't get logged because that message https://github.com/openstack/nova/blob/94671e279fddc0d8eba29375750dd85410a0b9b9/nova/api/openstack/wsgi.py#L266-L271 gets overwritten by https://github.com/openstack/nova/blob/94671e279fddc0d8eba29375750dd85410a0b9b9/nova/api/openstack/wsgi.py#L635-L637
13:01:58 cdent Is tomorrow the planned spec sprint day?
13:03:26 dane-fichter cdent: yes
13:03:36 cdent thanks dane-fichter
13:03:44 dane-fichter no problemo
13:11:05 jaypipes dane-fichter: you see my review on the trusted flavors spec?
13:11:16 dane-fichter jaypipes: yes.
13:11:26 dane-fichter jaypipes: working on another draft currently
13:11:26 jaypipes dane-fichter: k. any questions for me?
13:11:30 jaypipes dane-fichter: gotcha
13:12:13 dane-fichter jaypipes: we considered doing this in the way you suggested a couple cycles ago
13:12:28 dane-fichter jaypipes: but I think another core shot it down.
13:12:29 jaypipes dane-fichter: oh? who blocked that idea?
13:12:55 dane-fichter jaypipes: I don't recall, I'll try to find a comment or something on it
13:13:00 jaypipes dane-fichter: I wasn't a specs core until a few weeks ago, so def wasn't me ;)
13:13:19 dane-fichter jaypipes: lol
13:13:39 jaypipes dane-fichter: now whenever I do a specs review I shout "By the power of Grayskull" in my home office.
13:14:03 dane-fichter jaypipes: w great spec power comes great spec responsibility
13:14:10 jaypipes dane-fichter: indeed.
13:19:29 mordred jaypipes: I imagine the pugs REALLY enjoy the shouting
13:19:48 jaypipes mordred: my Pug secretary is deaf and mostly blind, so no, it's cool.
13:20:50 jaypipes mordred: the black one is my secretary: https://pbs.twimg.com/media/C2ylYN-WgAES_p8.jpg:large
13:21:13 jaypipes mordred: the white one, as you can probably tell, is retarded.
13:21:50 jaypipes mordred: sorry, I meant "learning challenged".
13:21:50 cdent I think your secretary may be ready for a relaxed retirement
13:21:58 mordred jaypipes: :)
13:22:11 mordred jaypipes: they both look _very_ excited
13:22:24 jaypipes mordred: oh, that's "active mode". trust me.
13:23:40 mordred it feels active. like they're about to spring forth and maybe change their head position slightly
13:25:26 mordred stillness, after all, does not equal stasis. in every instant of stillness exists the potential for movement, so proper stillness is extremely active
13:36:48 jaypipes how very metaphysical of you, mordred
13:38:36 openstackgerrit Samantha Blanco proposed openstack/nova master: Make flavor-rxtx policy more granular https://review.openstack.org/444036
13:42:28 openstackgerrit Scott Moser proposed openstack/nova master: Limit exposure of network device types to the guest. https://review.openstack.org/400883
13:43:03 sdague alex_xu: you able to revise https://review.openstack.org/#/c/450834 ?
13:44:05 johnthetubaguy sdague: do you remember when we said nova-cert would die?
13:44:26 johnthetubaguy sdague: did we say wait until we arise the API min_version? or kill it sooner?
13:44:35 sdague I thought it was going to die sooner
13:45:00 sdague when did it deprecate?
13:49:01 johnthetubaguy sdague: https://github.com/openstack/nova/commit/789edad0e811d866551bec18dc7729541105f59d
13:49:42 johnthetubaguy sdague: it might be time already, macsz was putting a spec together to delete it, not strictly required, but there is API impact

Earlier   Later