| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-30 | |||
| 17:18:16 | openstackgerrit | Merged openstack/nova master: check query param for server groups function https://review.openstack.org/500347 | |
| 17:29:09 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Implement new attach Cinder flow https://review.openstack.org/330285 | |
| 17:29:09 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: libvirt: Allow multiple volume attachments https://review.openstack.org/267587 | |
| 17:31:40 | cfriesen | melwitt: (or anyone else) has bug 1469179 been fixed with placement/resource_providers? | |
| 17:31:41 | openstack | bug 1469179 in OpenStack Compute (nova) "instance.root_gb should be 0 for volume-backed instances" [Medium,In progress] https://launchpad.net/bugs/1469179 - Assigned to melanie witt (melwitt) | |
| 17:34:10 | melwitt | cfriesen: not yet. at the ptg when we discussed placement priorities, other items were more pressing, like alternate hosts and nested resource providers. so shared storage got bumped to rocky | |
| 17:39:00 | mriedem | melwitt: i've got a workaround for the compute node create / cell issue | |
| 17:39:03 | mriedem | pushing up shortly | |
| 17:39:07 | mriedem | it's uber hacky, but... | |
| 17:39:12 | mriedem | it's also pretty simple | |
| 17:40:24 | melwitt | okay, thanks. that'll help me determine what's wrong unless it's the periodic task thing from the fixture review | |
| 17:41:30 | openstackgerrit | Andreas Karis proposed openstack/nova master: Fixes 'Not enough available memory' log message https://review.openstack.org/524038 | |
| 17:42:50 | openstackgerrit | Eric Fried proposed openstack/nova master: Proper error handling by _ensure_resource_provider https://review.openstack.org/524263 | |
| 17:42:51 | mriedem | this isn't a periodic task issue | |
| 17:43:03 | efried | cdent jaypipes --^ | |
| 17:43:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add multi-cell negative test for cold migration with target host https://review.openstack.org/524027 | |
| 17:43:10 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Workaround CellDatabase fixture limitation when creating computes https://review.openstack.org/524264 | |
| 17:43:13 | mriedem | melwitt: dansmith: ^ | |
| 17:43:28 | cdent | thanks efried | |
| 17:44:24 | openstackgerrit | Dan Smith proposed openstack/nova master: Genericify the instance_list stuff https://review.openstack.org/524253 | |
| 17:44:25 | dansmith | mriedem: melwitt: ^ | |
| 17:44:25 | cfriesen | melwitt: okay, thanks. For the two patches that are up for review, are there problems with them or are they basically functional? | |
| 17:46:34 | melwitt | cfriesen: some people prefer not to take the cruft that will have to be removed later, when resource providers work is going on and the bug/limitation has always been there | |
| 17:47:15 | efried | jaypipes Want me to address https://review.openstack.org/#/c/377215/ ? | |
| 17:47:19 | melwitt | other than that, the patches are basically functional (though in merge conflict right now) and don't pollute any database records with root_gb=0 | |
| 17:47:22 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add multi-cell negative test for cold migration with target host https://review.openstack.org/524027 | |
| 17:47:23 | cfriesen | melwitt: I'm just wondering about taking it as a local patch...we previously had fixed it a different way in Newton | |
| 17:48:11 | cfriesen | melwitt: I expect we'll pull them in and try them out. | |
| 17:49:01 | mriedem | dansmith: ack, will hit that after lunch | |
| 17:49:04 | melwitt | mriedem: it pains me how similar that is to what I'm trying to do with my patch. I just don't get what I missed yet | |
| 17:50:06 | mriedem | ease that pain with pills | |
| 17:50:16 | dansmith | mriedem: I'm thinking I need to follow up with a split of that file into the generic listing infrastructure, and the instance part of that so that the migration dudes can do the same | |
| 17:50:57 | mriedem | yeah, so instance_list is the instance-specific impl, and we'd have migration_list with that specific impl | |
| 17:51:02 | mriedem | and some generic module in between | |
| 17:51:18 | mriedem | not sure what that would be called? multi_cell_list? | |
| 17:51:25 | dansmith | yep something liket hat | |
| 17:51:30 | dansmith | sound good? | |
| 17:51:33 | mriedem | yeah | |
| 17:52:16 | openstackgerrit | Merged openstack/nova master: Remove setting of version/release from releasenotes https://review.openstack.org/520373 | |
| 17:52:27 | jaypipes | efried: I can do that. are there any of your changes you haven't pushed? | |
| 17:53:10 | jaypipes | efried: in particular, I'll rebase through 520246, so make sure I don't overwrite any of your work... | |
| 17:53:28 | efried | jaypipes No, but note that I want to break away the stuff above | |
| 17:53:41 | efried | jaypipes Sorry, redo: No, don't rebase that far up. | |
| 17:53:52 | efried | I'm breaking off the subseries starting at uuid_set() | |
| 17:53:55 | jaypipes | efried: where should I go to. | |
| 17:53:57 | jaypipes | ok. | |
| 17:54:02 | jaypipes | no prob. gimme a few minutes | |
| 17:54:15 | efried | So just rebase from https://review.openstack.org/#/c/385693/ (which needs a rebase onto its predecessor too) | |
| 17:54:57 | efried | jaypipes ^ | |
| 17:55:09 | jaypipes | ya | |
| 18:01:33 | cdent | efried: left some questions on that bug fix, looks mostly good, but I’m curious about some bits | |
| 18:01:48 | efried | cdent Ack, thanks for the review. Looking... | |
| 18:04:10 | openstackgerrit | Dan Smith proposed openstack/nova master: Split instance_list into instance and multi_cell https://review.openstack.org/524302 | |
| 18:07:44 | mgagne | mriedem: so I reread the placement documentation and it's kind of clear already what you need to do. There are some details which need to be updated such as mention of cellsv1. I will pass through the doc again and see if it's clearer for me now. | |
| 18:10:51 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: allow filter providers in tree https://review.openstack.org/377215 | |
| 18:10:52 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: adds REST API for nested providers https://review.openstack.org/384807 | |
| 18:10:52 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: update client to set parent provider https://review.openstack.org/385693 | |
| 18:10:55 | jaypipes | efried: done | |
| 18:11:05 | efried | jaypipes Cool mon. | |
| 18:13:01 | openstackgerrit | melanie witt proposed openstack/nova master: Improve the CellDatabases test fixture and usage https://review.openstack.org/508432 | |
| 18:13:02 | openstackgerrit | melanie witt proposed openstack/nova master: WIP Store a RequestContext on service to reuse for periodic tasks https://review.openstack.org/524306 | |
| 18:14:42 | melwitt | mriedem: the test in https://review.openstack.org/#/c/524027 passes with the above ^ | |
| 18:16:38 | openstackgerrit | Eric Fried proposed openstack/nova master: Proper error handling by _ensure_resource_provider https://review.openstack.org/524263 | |
| 18:16:50 | efried | cdent Not sure if I addressed your concerns here ^ | |
| 18:17:23 | efried | cdent I wouldn't think it necessary to say, "Exceptions raised from here may be caught, logged, or ignored by callers as appropriate" | |
| 18:19:11 | efried | cdent Really just brings things more in line with other methods in SRC, like _update_inventory_attempt which may raise InventoryInUse | |
| 18:19:40 | melwitt | mriedem: *reading the commit message* so your test doesn't always fail though? trying to confirm if the test passing means the issue in my patch was addressed by the latest changes I tried | |
| 18:20:04 | cdent | efried: that wasn’t quite what I meant. You’re obliged, as someone who has introduced a new situation, where exceptions are now raising that weren’t before, to make sure they don’t break other stuff. That can be as simple as identifying the loop where all Exceptions are handled in the scheduler or the resource trackers servers. | |
| 18:20:47 | efried | cdent OIC. Not enough to pass the CI? | |
| 18:21:13 | cdent | We’ve established over and over that CI isn’t covering lots of stuff, especially in the resource tracker | |
| 18:21:54 | cdent | It’s very likely, however, that this _is_ covered, by the kinds of loops that I mention above, but if you don’t say that in your commit message, then reviewers have doubt and we want to minimize that doubt | |
| 18:22:08 | efried | cdent I was being mostly ironical. I'll dig into it. Thanks. | |
| 18:26:31 | efried | cdent AFAICT, it all percolates up to this catch-all, which... ignores it https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L7105 | |
| 18:27:43 | cdent | i assumed it would be something like that, and logging and ignoring is what I would expect: it’s a long running process that’s not allowed to die | |
| 18:28:13 | cdent | but if for some reason that was some subclass of Exception, then there would be a chance your new exception would fall through and boom | |
| 18:30:53 | openstackgerrit | Eric Fried proposed openstack/nova master: Proper error handling by _ensure_resource_provider https://review.openstack.org/524263 | |
| 18:30:59 | efried | cdent Added more words ^ | |
| 18:39:09 | mriedem | melwitt: it was failing | |
| 18:45:44 | openstackgerrit | Eric Fried proposed openstack/nova master: Remove workaround for bug #1709118 https://review.openstack.org/524316 | |
| 18:45:45 | openstack | bug 1709118 in OpenStack Compute (nova) "_ContextAuthPlugin needs a refresh" [Low,Confirmed] https://launchpad.net/bugs/1709118 | |
| 18:54:35 | mriedem | dansmith: comments in https://review.openstack.org/#/c/524253/2 | |
| 18:54:43 | mriedem | just nits, tests are passing, looks good otherwise | |
| 18:57:18 | melwitt | mriedem: hm, so that points to it being related to the periodic task bug dansmith found in my patch. but the initial write of the compute node record doesn't happen via periodic task, it happens through the pre_start_hook() call in Service. so I'm not sure why fixing the periodic task helps | |
| 18:58:30 | dansmith | mriedem: see my reply real quick | |
| 18:58:53 | dansmith | melwitt: a periodic would create another record in cell0 if it runs | |
| 18:59:04 | dansmith | that was my point | |
| 18:59:22 | melwitt | dansmith: right ... trying to think how that could mess up mriedem's test though, where it got ComputeHostNotFound | |
| 18:59:41 | melwitt | like wouldn't it find the original record that was written via pre_start_hook() | |
| 19:00:15 | dansmith | melwitt: ack, okay, I haven't looked at his thing so I dunno | |
| 19:00:20 | dansmith | just commenting from the sidelines | |
| 19:00:56 | melwitt | ack :) | |
| 19:01:29 | mriedem | dansmith: done | |
| 19:02:08 | melwitt | fixing the periodic task thing (by storing it on Service) made the test pass but why O.o | |
| 19:02:30 | dansmith | mriedem: cool, see the split above it? | |
| 19:03:58 | mriedem | yeah, just haven't reviewed it yet | |
| 19:05:24 | dansmith | gahd dammit | |
| 19:05:37 | dansmith | making changes to a file underneath a rename is annoying :) | |
| 19:08:04 | mriedem | cdent: easy cleanup if you want to respin this https://review.openstack.org/#/c/484667/ | |
| 19:08:06 | mriedem | then i'd fast approve | |
| 19:08:52 | cdent | cool, thanks, will get it in a couple minutes, rado’s usually gone by now | |