Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-04-03
10:30:43 openstackgerrit Stephen Finucane proposed openstack/nova master: Optimize creating security_group https://review.openstack.org/315311
10:34:24 f10d4 hi
11:02:21 openstackgerrit Andrey Volkov proposed openstack/nova master: [placement] Add api-ref for GET RP inventories https://review.openstack.org/449765
11:02:22 openstackgerrit Andrey Volkov proposed openstack/nova master: [placement] Add api-ref for POST RP inventories https://review.openstack.org/451855
11:02:22 openstackgerrit Andrey Volkov proposed openstack/nova master: [placement] Add api-ref for PUT RP inventories https://review.openstack.org/452707
11:02:23 openstackgerrit Andrey Volkov proposed openstack/nova master: [placement] Add api-ref for DELETE RP inventories https://review.openstack.org/452708
11:02:37 openstackgerrit John Garbutt proposed openstack/nova-specs master: Add additional-default-policy-roles spec https://review.openstack.org/427872
11:10:59 openstackgerrit Dinesh Bhor proposed openstack/nova master: Use HostAddressOpt for opts that accept IP and hostnames https://review.openstack.org/445782
11:13:07 johnthetubaguy ildikov: had a deeper look at this check_detach patch, actually I think we loose some super important checks, but I could be missing something big: https://review.openstack.org/#/c/446671
11:27:47 ildikov johnthetubaguy: I added that check as a prep for multi-attach, which Nova still does not support
11:28:19 johnthetubaguy ildikov: right, but I am saying it breaks all our existing users, how do we stop that
11:29:07 johnthetubaguy ildikov: it seems like it would work fine with multi-attach to me, it would be good to describe what checks are bad for multi-attach there, I guess its one of the status checkes
11:29:38 ildikov johnthetubaguy: it's just that it's unnecessary just like the check_attach was
11:29:55 johnthetubaguy ildikov: I described in my comments, I believe its critical
11:30:09 ildikov johnthetubaguy: it's Cinder who should do the status checks
11:30:23 johnthetubaguy ildikov: sure, but we don't have an API in the new flow to do that
11:30:44 ildikov ildikov: the status checks you mean?
11:30:49 johnthetubaguy ildikov: and the old flow doesn't seem to check the instance uuid
11:31:09 ildikov johnthetubaguy: because it didn't need to
11:31:09 johnthetubaguy ildikov: ... so your patch says stop check_detach, because we can just call begin_detaching?
11:31:34 johnthetubaguy ildikov: it really, really does
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 jaypipes dane-fichter: k. any questions for me?
13:11:26 dane-fichter jaypipes: working on another draft currently
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.

Earlier   Later