| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 14:57:31 | jaypipes | mdbooth: you previously reviewed https://review.openstack.org/#/c/399679/. would be great to have a once over from you on that. | |
| 14:57:57 | mdbooth | jaypipes: Sure. | |
| 14:58:03 | jaypipes | cheers | |
| 14:58:55 | asettle | johnthetubaguy: further explanation, package managers test often with automatic setup and that might include updating config files | |
| 14:58:57 | mdbooth | jaypipes: Incidentally, I was looking for validation of this approach for handling RequestContexts for cells: https://review.openstack.org/#/c/443627/ | |
| 14:59:14 | mdbooth | Or invalidation, obviously, which is why I haven't tried to do the whole job yet. | |
| 14:59:25 | jaypipes | mdbooth: k, queued. | |
| 14:59:37 | mdbooth | jaypipes: Thanks | |
| 15:01:30 | openstackgerrit | Gábor Antal proposed openstack/nova master: Add json style checking for sample notifications https://review.openstack.org/443677 | |
| 15:03:05 | dansmith | mdbooth: I skimmed it briefly | |
| 15:03:28 | dansmith | mdbooth: I think we can go with that approach for things for object contexts, where we iterate over multiple cells | |
| 15:03:36 | dansmith | mdbooth: I don't like it for clearly single-cell-targeted operations on an instance or a host | |
| 15:04:03 | dansmith | mdbooth: because after getting all of this to work, I feel confident we'll have some leaks where we don't realize that we've now got two copies of the context | |
| 15:04:06 | mdbooth | dansmith: Ok. So it's worth adding context.for_cell() and fixing up say get_all() and its users? | |
| 15:04:12 | dansmith | mdbooth: the context _is_ gobal and by reference everywhere | |
| 15:04:21 | dansmith | mdbooth: we get it by reference from the request | |
| 15:05:31 | dansmith | mdbooth: maybe, I also would want to see that change on top of a fully working tempest run of the current stack to prove it doesn't break anything else | |
| 15:05:59 | jaypipes | sfinucan: so https://review.openstack.org/#/c/399653/is going to require the gate to have libvirt 1.2.9 | |
| 15:06:01 | mdbooth | dansmith: That was another reason I didn't want to go all the way down this rabbit hole. I expect there are a bunch of tricky cases. | |
| 15:06:12 | dansmith | mdbooth: there are a ton | |
| 15:06:31 | mdbooth | dansmith: However, I was hoping that by removing the whole side-effect of changing the global context there might be significantly less | |
| 15:06:41 | dansmith | mdbooth: I think you might be surprised | |
| 15:07:03 | sfinucan | jaypipes: I thought the point of those tests was to fake out libvirt? | |
| 15:07:12 | mdbooth | dansmith: Ok, I'll limit to get_all() initially and see how it looks. | |
| 15:07:14 | mdbooth | Thanks for looking. | |
| 15:07:28 | jaypipes | sfinucan: see the functional test failure :) | |
| 15:07:30 | cdent | coreycb: I think I've got another one. Is it considered a bug for the service.conf files to have uncommented config settings? | |
| 15:08:11 | coreycb | cdent, not necessarily. some of the packages set defaults. | |
| 15:08:24 | sfinucan | jaypipes: Well that doesn't look right :D | |
| 15:08:36 | cdent | coreycb: in this case it is that os_region_name in the [placement] section is being set | |
| 15:08:50 | sfinucan | Guess we can mock out that version check too | |
| 15:08:55 | dansmith | mdbooth: fwiw, we already had a discussion about whether or not to fork the context in the objects that are tied to cells and decided on this approach to start | |
| 15:09:09 | dansmith | mdbooth: one thing to remember is that if you send an object over rpc, it resets the context on all the objects in the stack | |
| 15:09:25 | dansmith | mdbooth: so if you make a call over to conductor, for anything like boot or resize, contexts get reset anyway | |
| 15:09:33 | coreycb | cdent, that may not need to be set. we set defaults so that the service will can start after the package is installed. | |
| 15:10:13 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403 | |
| 15:10:21 | cdent | coreycb: the reason I noticed was because the docs I was testing changed it, i made all those changs at the top of the section, and then realized it was going to conflict with something that's set further below | |
| 15:10:47 | cdent | coreycb: ah, okay, yeah, that makes sense then | |
| 15:10:53 | cdent | it won't start without that | |
| 15:10:57 | cdent | (there's no default) | |
| 15:11:42 | mriedem | dansmith: ftr, i did give you the business, yes | |
| 15:11:44 | mriedem | :) | |
| 15:11:55 | dansmith | mriedem: yeah, I just totally wasn't paying attention when I wrote that :) | |
| 15:11:59 | dansmith | mriedem: see my rev | |
| 15:12:00 | dansmith | ? | |
| 15:13:02 | mriedem | yes | |
| 15:13:08 | mriedem | wait, what? | |
| 15:13:26 | mriedem | oh i see | |
| 15:13:53 | dansmith | actually, I have to fix target_cell too | |
| 15:13:58 | mriedem | dansmith: you missed on | |
| 15:13:59 | mriedem | *one | |
| 15:14:00 | dansmith | oh no | |
| 15:14:01 | dansmith | it works | |
| 15:14:07 | mriedem | you missed one in the exception block | |
| 15:14:09 | dansmith | ah yeah | |
| 15:14:13 | dansmith | just a sec | |
| 15:14:31 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: Temporarily untarget context when deleting from cell0 https://review.openstack.org/443403 | |
| 15:14:48 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Using max api version in notificationt sample test https://review.openstack.org/443686 | |
| 15:15:32 | coreycb | cdent, ok. we're considering loosening the restriction of having to start services by default because it can cause issues particularly with deployment tools. | |
| 15:15:54 | dansmith | mriedem: so we should be able to write a functional test for this that checks the quota in each after delete right? | |
| 15:15:56 | coreycb | cdent, but we haven't done that yet. if we did i think we'd just use the default config files. | |
| 15:16:20 | cdent | coreycb: it's a reasonable thing to do for machines being directly used by humans but not so much in this case :) | |
| 15:16:40 | mriedem | dansmith: yes | |
| 15:16:55 | mriedem | that's the plan | |
| 15:17:00 | mriedem | the tricky thing is going to be, | |
| 15:17:00 | coreycb | cdent, yeah :) | |
| 15:17:16 | mriedem | we only need one of these fixes for the backport to ocata | |
| 15:17:24 | mriedem | so the functional test won't pass until the top patch in pike | |
| 15:17:29 | mriedem | but it will work for the bottom patch in ocata | |
| 15:17:42 | mriedem | but re-ordering the changes in master doesn't make sense to me | |
| 15:17:49 | dansmith | okay | |
| 15:18:02 | mriedem | so i'm thinking maybe the functional test is in a separate patch | |
| 15:18:03 | mriedem | not sure | |
| 15:18:09 | dansmith | yeah | |
| 15:22:06 | mriedem | so i think the easy thing i can do first is create a functional regression patch that goes at the bottom of the series and recreates the failure, and asserts the error, | |
| 15:22:10 | mriedem | then the fix patches on top of that, | |
| 15:22:31 | mriedem | and the top fix patch changes the functional test to show it working | |
| 15:22:35 | mriedem | that was my plan anyway | |
| 15:22:39 | dansmith | I was just looking at the tests | |
| 15:22:44 | dansmith | give me a few minutes | |
| 15:23:00 | dansmith | we don't really have a way to query into the reservation stuff to look, AFAICT, but I think I can hack something in at least to check | |
| 15:23:09 | mriedem | well, | |
| 15:23:16 | mriedem | for a functional test i think this is pretty straightforward, | |
| 15:23:19 | mriedem | we have cell1 and cell0, | |
| 15:23:25 | mdbooth | dansmith: Incidentally, https://review.openstack.org/#/c/443403/ is pretty much what I was talking about with the side-effect thing. | |
| 15:23:29 | mriedem | we disable nova-compute or do something to make scheduling fail, | |
| 15:23:39 | mriedem | create the server and show it goes into ERROR state, | |
| 15:23:47 | dansmith | mdbooth: it's not at all, since this is a problem across multiple requests | |
| 15:23:49 | mriedem | assert we have quota of instances=9 (default-1) | |
| 15:23:55 | mriedem | then delete the instance, | |
| 15:24:03 | mriedem | and assert that quota of instances=10 (but it won't be until the fix) | |
| 15:24:12 | mriedem | it's basically the same recreate steps as in the bug report | |
| 15:24:35 | dansmith | mriedem: I guess I'm lost in the db api on terminology, knowing which things are returning limits and which are calculated by limit and usage | |
| 15:24:44 | dansmith | mriedem: but yeah, that's what I'm doing | |
| 15:24:48 | mriedem | the functional test just hits the rest api | |
| 15:24:54 | mriedem | like novaclient would | |
| 15:25:04 | mriedem | i really don't want to be going into the actual databases | |
| 15:25:32 | mriedem | i was going to do this in nova/tests/functional/regressions | |
| 15:25:54 | dansmith | mriedem: well, you're not really validating that exactly what you think is happening really is, but okay | |
| 15:26:09 | dansmith | I'll leave it to you then | |