Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-25
18:20:26 melwitt I think it makes the fixture much simpler, but definitely want to run it by you in case I missed something
18:22:13 dansmith yeah I'm just afraid of it breaking something subtle which we don't find for a couple months and then think we need to fix it by changing the real code when in fact the fixture is too relaxed or something
18:22:26 dansmith but that's just because of how hard it was to get it right in the first place, of course
18:27:00 melwitt oh, you mean something to do with racing tests appearing like real bugs when there's really just an issue with the fixture? yeah, I can understand that concern. as far as I can tell, my proposed patch removes all changing of global state, so I'd think there won't be an issue. but those are famous last words, I know
18:29:44 melwitt if we're too afraid to change the fixture, then we will hopefully be able to accept the multi-cell nova-manage patch's func test not being full coverage because of the faking that the CellDatabases fixture does. my primary objective is to get the multi-cell nova-manage db archive_deleted_rows done
18:31:33 melwitt and while working on that, its func test was not failing when it should have been (bug in a patchset), and I found it wasn't failing properly because of the "last context manager" faking in the fixture
18:31:33 dansmith okay, I'm not sure why that is harder than other cell iteration things we do in tests, but I'd be much more inclined to accept more mockery (since that's really a trivial operation) vs. blocking that on rearchitecting the fixture. But, I haven't looked enough into why that's a problem to say really
18:31:40 dansmith maybe I can try to do that before tomorrow at least
18:34:30 melwitt dansmith: tl;dr is the func test is written correctly and is good, but it did _not_ catch a bug in the proposed multi-cell archive impl because of the faking in the fixture. the fixture auto-targets untargeted database access to the last targeted database or the default database. the former hid the bug in the impl because the fixture auto-targeted something that was not targeted in real life and needed to be targeted in real life
18:34:57 melwitt I hope that makes sense
18:35:02 mriedem i'd also rather figure out why the func tests on the archive patch don't work rather than block on redoing the fixture, but i don't know what the issues were,
18:35:04 mriedem having said that,
18:35:16 mriedem i have a func test in my cross-cell resize series that does db archive on all 3 cells in the test https://review.opendev.org/#/c/651650/22/nova/tests/functional/test_cross_cell_migrate.py
18:35:20 dansmith but the real code sends untargeted stuff to the default db in the config, which is why the fixture does
18:35:36 melwitt mriedem: I did figure it out, it was because of the auto-targeting by "last targeted database" if untargeted
18:36:04 dansmith mriedem: yeah, even still, I'd be happy with just a unit test to make sure that archive is calling archive on all the cell mappings.. it
18:36:09 dansmith is such a trivial op I don't really know that we need much more than that,
18:36:17 mriedem ok i guess i mean "i'd rather figure out an easier way to make the new tests work with the existing fixture"
18:36:24 dansmith and we run archive in tempest jobs, which should hit cell0 and cell1 if we make it run all cells
18:36:24 melwitt dansmith: but the fixture also sends untargeted stuff to the "last targeted db" first. I think it should only send untargeted stuff to the default db
18:36:25 mriedem also,
18:36:29 melwitt (which is what my patch is doing)
18:36:48 mriedem i was going to say - nova-next runs archive_deleted_rows, we can and should make it run on all cells
18:36:54 mriedem which will hit cell0 and cell1 as dansmith said
18:37:03 mriedem so....then we'd have real integration test coverage
18:37:04 dansmith melwitt: everything in the compute node is untargeted though, which is why I don't really know how you can change that globally and have it match the real world
18:37:09 dansmith but.. I haven't read it so I dunno
18:37:11 mriedem of the cli, which is better than the functional stuff anyway
18:37:18 dansmith mriedem: agreed
18:38:03 melwitt dansmith: I think maybe things are getting confused. in the fixture we have two ways of targeting untargeted stuff. one is sending it to the default db (good) and one is sending it to the last db that was targeted (bad IMHO)
18:38:38 mriedem i have run afoul of that latter behavior
18:38:40 dansmith I understand
18:38:41 mriedem agree it's not fun
18:38:53 mriedem but i think i've found ways around that in my multi-cell func testing
18:39:16 melwitt so y'all actually like the last targeted db thing? I'm open to that, just didn't think anyone would think they wanted to keep it
18:39:25 mriedem this one https://review.opendev.org/#/c/641179/
18:39:36 mriedem i remember i was hitting weirdness because of that 'last targeted context' thing as well
18:39:43 mriedem i'm not saying i like it
18:39:52 mriedem but i also don't like redoing the whole thing per se
18:39:56 mriedem when there are maybe other ways
18:40:05 melwitt I'm +1 on the real integration testing, that's fine by me. but I just thought it would make the fixture a lot simpler and less big hiding to remove that bit about "last targeted db"
18:40:12 mriedem it's like touching the old quotas code - i can, but don't want to if i can help it
18:40:40 mriedem w/o looking deep into your change idk
18:40:45 mriedem i wouldn't abandon it,
18:40:50 mriedem but i wouldn't block the other thing on it either
18:40:57 mriedem i'd get the integration testing in nova-next working
18:41:09 mriedem which should be like, 1 line
18:41:09 melwitt that's fair. it's less like redoing and more like "delete all the self._last_ctxt_mgr" but yeah, when you get around to it, take a look and see if you hate it
18:42:22 melwitt don't get me wrong, I'm totally fine with that. as long as it gets tested, I'm happy. I was honed in on trying to make the func test work 100%
18:43:50 melwitt and thought people might be happy to see all the _last_ctxt_mgr stuff deleted from the fixture, no more global state changing, much simpler
18:45:06 melwitt I'll rebase the multi-cell archive patch on top of a different change to add --all-cells to nova-next
19:04:30 mriedem efried: ha http://lists.openstack.org/pipermail/openstack-discuss/2019-July/008037.html
19:25:29 openstackgerrit Merged openstack/nova master: api-ref: touch up the os-services docs https://review.opendev.org/672571
19:33:45 artom That actually went pretty well.
19:45:30 openstackgerrit Matt Riedemann proposed openstack/nova-specs master: Support delete_on_termination in volume attach api https://review.opendev.org/612949
19:45:39 mriedem i cleaned this up ^ it's pretty straight-forward
20:02:28 efried mriedem: approved that. Seems like an easy win.
20:03:19 mriedem ack, i'm cleaning up https://review.opendev.org/#/c/667894/ now
20:04:55 efried mriedem: make all the names match up too if you please
20:05:40 efried mriedem: the bp is at https://blueprints.launchpad.net/nova/+spec/add-user-id-field-to-the-migrations-table
20:06:48 efried (so the path at the top of the spec is right, but the file path, commit message, and topic are wrong)
20:09:42 openstackgerrit Merged openstack/nova-specs master: Support delete_on_termination in volume attach api https://review.opendev.org/612949
20:10:25 efried sean-k-mooney, stephenfin: Did we ever figure out whether/how you could tell from within a guest which / how many CPUs are pinned? Was that going to be via "metadata"?
20:10:46 sean-k-mooney efried: that is what the latest spec say yes
20:11:24 efried okay, I was about to dig into it. Unfortunately, I don't see me being competent to update it such as to get it approved today.
20:12:02 sean-k-mooney https://review.opendev.org/#/c/668656/5/specs/train/approved/use-pcpu-vcpu-in-one-instance.rst@173
20:12:19 sean-k-mooney The metadata API will be extended to dedicated cpu info with new version.
20:12:21 sean-k-mooney The new field will be added to the `meta_data.json`::
20:12:23 sean-k-mooney dedicated_cpu=<cpuset string>
20:12:25 sean-k-mooney The ``cpuset string`` indicated the instance cpus which are running on
20:12:27 sean-k-mooney dedicated pCPU.
20:12:27 efried ah nice
20:13:32 sean-k-mooney you could alos use that new numa toploty api if we approved that i guess
20:13:45 sean-k-mooney although that is not really from within the guest
20:14:45 efried I would want stephenfin to be a +2 on that spec anyway, so I guess we'll see if alex_xu et al can polish it up and request sfe, since we seem to have general agreement with caveats as noted.
20:15:16 efried I'll sent a note on sfe process (which apparently I'll be making up) either tomorrow or Monday.
20:15:21 sean-k-mooney does the timeline i set out make sesne to you
20:15:28 efried oh, totally.
20:28:47 openstackgerrit Matt Riedemann proposed openstack/nova-specs master: Add user_id to the migrations https://review.opendev.org/667894
20:29:28 openstackgerrit Matt Riedemann proposed openstack/nova-specs master: Add user_id to the migrations https://review.opendev.org/667894
20:29:39 mriedem first is the diff, second is the file rename
20:31:23 mriedem only open question on ^ is if we should add user_id/project_id as request filter params to GET /os-migrations
20:31:33 mriedem i think we might as well
20:35:22 efried sean-k-mooney: please vet that I represented you correctly in the whiteboard https://blueprints.launchpad.net/nova/+spec/use-pcpu-and-vcpu-in-one-instance
20:37:01 sean-k-mooney sure
20:38:03 sean-k-mooney yep
20:38:14 efried thx
20:38:23 sean-k-mooney without all the typos in my original comment :)
20:38:26 gmann efried: melwitt sean-k-mooney ack I will check the spec. it was in my list but did not get time to review.
20:38:43 efried thanks gmann
20:39:11 melwitt cool gmann
20:41:45 efried mriedem: where are request filter params for GET /os-migrations documented in the api-ref?
20:42:26 efried oh, are they the top-level fields hidden, host, instance_uuid etc?
20:42:28 efried uck
20:43:26 efried mriedem: but yeah, I think it makes sense to add this in there.
20:43:46 mriedem yeah
20:44:26 mriedem i frequently filter on migration_type and instance_uuid in functional tests since GET /servers/{server_id}/migrations is hard-coded to only be in-progress live migrations
20:54:56 efried mriedem: chuck that filter field in there and I'm +2
20:55:09 mriedem alright then
20:55:13 efried not sure if you're still comfortable being the other +2

Earlier   Later