| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-02 | |||
| 18:40:29 | dansmith | jroll: not very | |
| 18:40:41 | jroll | whomp whomp | |
| 18:40:44 | tssurya | dansmith: yes, | |
| 18:41:58 | melwitt | jroll: multi cell is not a thing in ocata, I'd say | |
| 18:42:16 | jroll | melwitt: yeah, that was my actual question, thanks | |
| 18:43:03 | melwitt | and, affinity scheduling doesn't work with multi cell yet, there's a bug (I have a patch up to fix it) https://review.openstack.org/540258 | |
| 18:43:35 | melwitt | I know that's something y'all need | |
| 18:45:02 | jroll | tis, thanks for that | |
| 19:02:33 | melwitt | hm, seeing an issue where a BFV instance being deleted fails to detach the volume because attachment_id = None http://logs.openstack.org/01/565601/2/check/tempest-full/f1004d9/controller/logs/screen-n-cpu.txt.gz#_May_02_03_50_00_958050 | |
| 19:03:55 | cfriesen | anyone know offhand what's causing this failure when running tox against newton? http://paste.openstack.org/show/720243/ | |
| 19:04:50 | melwitt | did you do a tox -e<whatevs> -r first? | |
| 19:04:57 | melwitt | usually have to do that when you switch branches | |
| 19:05:39 | cfriesen | will try that, thanks | |
| 19:06:47 | melwitt | usually you'll have different deps versions required per branch and that's where those errors come from. if the env has a module version missing an attribute that another branch needs | |
| 19:11:21 | melwitt | so for the BFV volume detach, it's using the old attach API, that means bdm.attachment_id was not set or None | |
| 19:11:56 | melwitt | and then the old os-detach API is called with attachment_id = null and that yields a 400 from cinder | |
| 19:13:33 | melwitt | now to figure out why attachment_id isn't set for a BFV instance created from a snapshot | |
| 19:22:04 | mriedem | melwitt: i know exactly why | |
| 19:22:08 | arvindn05 | efried: thanks for the review :). For the comment on the commit message, i just squash my individual commits to create the single commit that gerrit requires..hence the Added unit and functional tests | |
| 19:22:20 | mriedem | melwitt: https://review.openstack.org/#/c/541420/ | |
| 19:22:44 | melwitt | yesss, I was hoping there was already a patch for dat | |
| 19:22:49 | mriedem | melwitt: either way, when detaching we should know if we're doing new style detach (attachment_id is not None) or old style detach | |
| 19:23:05 | efried | arvindn05: You mean in your sandbox you had multiple commits, one that did the code and one that did the test, and you squashed them together before proposing them to gerrit? | |
| 19:23:19 | arvindn05 | yup | |
| 19:23:36 | efried | arvindn05: Yeah, that doesn't change my opinion :) There's no need for a commit message on a change that clearly has unit and functional tests to say that it has unit and functional tests. | |
| 19:23:39 | melwitt | mriedem: right. we're doing old style because attachment_id is None but, attachment_id sent over to cinder in the old style os-detach action call is also None which gets a 400 response | |
| 19:23:45 | efried | arvindn05: But you notice I'm not blocking on it either. | |
| 19:24:16 | melwitt | so old style detach fails | |
| 19:24:34 | mriedem | we don't send an attachment_id to os-detach in cinder | |
| 19:24:40 | mriedem | it's not a parameter for that api | |
| 19:24:49 | mriedem | well, i don't think it is | |
| 19:24:52 | melwitt | it's in the log here http://logs.openstack.org/01/565601/2/check/tempest-full/f1004d9/controller/logs/screen-n-cpu.txt.gz#_May_02_03_50_00_958050 | |
| 19:24:54 | mriedem | for new style detach, we'd call attachment_delete | |
| 19:25:03 | mriedem | log schmog, that's fake news | |
| 19:25:05 | melwitt | it's the only thing we're sending | |
| 19:25:06 | arvindn05 | efried: cool....i dont like the gerrit workflow...more used to github which allows individual commits and lets the maintainer decide on squash vs rebase vs commit as is | |
| 19:25:21 | arvindn05 | but going with the flow :) | |
| 19:25:36 | melwitt | WHAT SAY YOU | |
| 19:26:17 | efried | arvindn05: You're welcome (even encouraged) to split up reviews in gerrit, though generally if along test/impl lines the test should come first. | |
| 19:26:44 | efried | arvindn05: Me, I've never used github, so no basis for comparison there. | |
| 19:26:46 | mriedem | oh yeah i guess os-detach does take an attachment_id parameter, which is optional | |
| 19:27:14 | melwitt | it's weird that of all the params, that's the only thing we're sending. seems wrong | |
| 19:27:15 | mriedem | melwitt: we can't detach the volume on a volume-backed instance, is that what you're trying to do? | |
| 19:27:31 | mriedem | *root volume | |
| 19:28:34 | melwitt | mriedem: maybe. this is a delete_on_termination=True instance created from a snapshot (the tempest test) and when we delete the instance, it tries to detach the volume and delete it. and it fails to detach it | |
| 19:28:46 | mriedem | this schema check looks wrong | |
| 19:28:47 | mriedem | InvalidInput: Invalid input received: Invalid input for field/attribute attachment_id. Value: None. None is not of type 'string' | |
| 19:28:57 | melwitt | a BFV instance, that is | |
| 19:29:04 | mriedem | this schema doesn't say attachment_id is required https://github.com/openstack/cinder/blob/master/cinder/api/schemas/volume_actions.py#L73 | |
| 19:29:08 | mriedem | and it allows null | |
| 19:29:16 | mriedem | oh no sorry | |
| 19:29:23 | mriedem | that says you don't have to put a body in it | |
| 19:30:15 | melwitt | so what's the correct flow for deleting a BFV instance with delete_on_termination=True? has to detach and then delete the volume, the volume can't be deleted if it's in-use | |
| 19:31:31 | arvindn05 | efried: in gerrit, subsequent changes are always amends for a single review which means on the git side, you do lose history. If you wanted to say revert changes back to commit #2, we would need to download the code from gerrit for PS#2...anyway just my rant from using both github and gerrit :) | |
| 19:32:23 | efried | arvindn05: You can do commits in series, which means you use a different change-id for the second commit but set the first commit as its parent. | |
| 19:32:41 | mriedem | melwitt: the attachment_id getting passed to cinder isn't from the bdm, it's from the volume['attachments'] information, which is a thing that existed before the new style attach flow https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4026 | |
| 19:33:32 | efried | arvindn05: example: https://review.openstack.org/#/c/565279/2 is first, https://review.openstack.org/#/c/564351/5 is second, https://review.openstack.org/#/c/517757/33 is third in the series. | |
| 19:34:37 | mriedem | this code is likely all old and busted and should be removed https://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L537-L559 | |
| 19:34:44 | mriedem | since we don't support multiattach volumes with old style attachments | |
| 19:34:58 | openstackgerrit | Merged openstack/nova master: Remove ExactCoreFilter ExactDiskFilter ExactRamFilter https://review.openstack.org/508886 | |
| 19:35:16 | mriedem | that was all prep work written in mitaka https://review.openstack.org/#/c/193134/ | |
| 19:35:41 | melwitt | arvindn05: yeah, with gerrit, you can't have multiple commits in one "review". so to do that, you have to make a stack of gerrit reviews (one review per commit) and then upload them together. you just do all of your commits as usual, then 'git review' will upload all of them as separate reviews that depend on each other | |
| 19:36:18 | mriedem | melwitt: so it looks like we have a bug that we still pass an attachment_id to os-detach even if we don't have an attachment_id | |
| 19:36:46 | melwitt | mriedem: okay, so an empty body os-detach will still do the right thing and detach the volume | |
| 19:37:30 | mriedem | maybe, not really sure, we don't have tests that try to detach a root volume | |
| 19:37:37 | melwitt | I'll try depends-on your patch and see if it makes the tempest test change work (I added an assert that the volume was gone and that failed because the volume couldn't be detached and thus deleted) | |
| 19:38:36 | mriedem | melwitt: ok, but you need this to get ceph working on stable branches? | |
| 19:39:11 | melwitt | mriedem: on master. I don't know why it's failing 100% now over this "volume dependent on snapshot" issue when it didn't use to, but that's what's going on | |
| 19:39:31 | mriedem | ok deps-on my patch and then we'll go from there | |
| 19:39:56 | arvindn05 | melwitt: efried: ahh...k. will keep that in mind. Good workflow to use when all of your commits are complete...if the last one is WIP, we will have to mark it in gerrit review i guess | |
| 19:40:02 | melwitt | but because the test does not first remove the volume snapshot, the second instance can't be deleted. I Don't Know How This Ever Worked(tm) for ceph | |
| 19:40:15 | arvindn05 | learn something new about gerrit everyday :) | |
| 19:40:21 | efried | arvindn05: Happens all the time. Even ones that *aren't* the last one get marked WIPs. | |
| 19:40:35 | efried | arvindn05: And the whole series doesn't merge at once; they merge bottom up like you would expect. | |
| 19:40:42 | efried | each is a dependency of the next. | |
| 19:40:46 | melwitt | arvindn05: yeah, the usual way is to just prepend your commit message subject line on the WIP one with "WIP" | |
| 19:41:30 | mriedem | melwitt: the test wasn't skipped for the ceph job before was it? | |
| 19:41:56 | arvindn05 | thanks! | |
| 19:42:00 | melwitt | mriedem: it might have been. I can't think of any other way this makes sense but I didn't find that yet. tbh I don't remember where to find the skip list for ceph | |
| 19:43:03 | mriedem | i don't think it ever landed, i remember cleaning up something about this recently | |
| 19:43:49 | mriedem | https://review.openstack.org/#/c/550497/ | |
| 19:46:40 | melwitt | so both skip list changes are abandoned ... does that mean there's no skip list? I'm confused | |
| 19:47:08 | melwitt | so that implies that test is new? *looks* | |
| 19:47:18 | mriedem | it's not new | |
| 19:47:23 | mriedem | it's only recently been renamed | |
| 19:47:31 | mriedem | but the idempotent id hasn't changed | |
| 19:47:38 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add nova-manage placement heal_allocations CLI https://review.openstack.org/565886 | |
| 19:47:51 | melwitt | hrm | |
| 19:47:54 | mriedem | mgagne: dansmith: ^ here is my start, sans tests and docs and lots of todos/notes | |
| 19:48:16 | mriedem | melwitt: this is the job definition, and no it doesn't have a skip list http://git.openstack.org/cgit/openstack-infra/openstack-zuul-jobs/tree/playbooks/legacy/tempest-dsvm-py35-full-devstack-plugin-ceph/run.yaml#n2 | |
| 19:48:26 | mriedem | jbernard was working on one but as you can see those patches were all abandoned | |
| 19:48:27 | melwitt | doubleyew tee eff | |
| 19:48:31 | mriedem | so for awhile the job was just totally broken | |
| 19:48:52 | melwitt | I don't know how this test could ever pass with ceph | |
| 19:49:45 | melwitt | and the job hasn't been broken for _that_ long, I didn't think | |
| 19:49:52 | melwitt | anywho | |
| 19:50:01 | mriedem | melwitt: could be that it wasn't using DEVSTACK_GATE_TEMPEST_FULL | |
| 19:51:09 | mriedem | tempest tox -e full runs tempest api tests concurrently and then scenario tests serially | |
| 19:51:12 | mriedem | and this is a scenario tes | |
| 19:51:13 | mriedem | *test | |