Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-11-29
16:56:52 cdent sigh, at microversin wars with myself
17:01:24 openstackgerrit Eric Fried proposed openstack/nova master: Make _Provider really private https://review.openstack.org/523932
17:01:29 efried jaypipes ^
17:01:52 efried jaypipes I didn't try to anticipate getters of _Provider attributes; we can add those as demand arises.
17:21:29 openstackgerrit Eric Fried proposed openstack/nova master: Make _Provider really private https://review.openstack.org/523932
17:21:29 openstackgerrit Eric Fried proposed openstack/nova master: ProviderTree.uuid_set() https://review.openstack.org/520243
17:21:55 efried jaypipes And there's uuid_set peeled out and modified as you suggested ^
17:22:02 efried (plus a silly UT fix on the base)
17:22:18 artom_ mriedem, is the topic set right for this series? https://review.openstack.org/#/c/521200/
17:22:46 artom_ I feel like bug/1732947 is the most obvious one
17:23:09 artom_ But I'd like to have related/1664931 somewhere, to keep track of all the changes that are happening because of that initial CVE fix
17:23:27 artom_ Hell, even create a (really weird) blueprint
17:23:47 mriedem artom_: the topic is set to the change on top
17:23:53 mriedem which is a different bug
17:24:21 mriedem track it in bug 1664931?
17:24:22 openstack bug 1664931 in nova (Ubuntu) "[OSSA-2017-005] nova rebuild ignores all image properties and scheduler filters (CVE-2017-16239)" [Undecided,New] https://launchpad.net/bugs/1664931
17:24:37 artom_ mriedem, I thought they were related?
17:24:59 mriedem bug 1732947 was a regression introduced by the fix for bug 1664931
17:25:01 openstack bug 1732947 in OpenStack Compute (nova) "volume-backed instance rebuild with no image change is still going through scheduler" [High,In progress] https://launchpad.net/bugs/1732947 - Assigned to Matt Riedemann (mriedem)
17:25:39 artom_ mriedem, right, that's what I meant
17:25:56 artom_ Ah, so only the first 2 changes in that series are fixes for 1664931 regressions?
17:26:55 mriedem melwitt: dansmith: are you aware of any functional test examples where we have multiple cells and a compute in each cell, and where fake.set_nodes works? i'm hitting something weird when trying to do a cold migration across 2 cells to assert it fails and hitting some weird stuff when i use fake.set_nodes
17:27:07 mriedem artom_: yes, the last change is a different bug
17:27:38 dansmith mriedem: no, but let me say it'd be nice if we could fix up that weird fake node behavior
17:27:38 artom_ mriedem, aha, thanks :)
17:27:38 mriedem could be related to the cells db fixture stuff being wonky
17:27:44 dansmith we basically work around it in all the tests that need that
17:27:56 mriedem artom: create an etherpad?
17:28:24 artom mriedem, yeah, not a bad idea
17:28:44 artom It's still a lot of clicking and checking branches, change IDs and topics
17:28:59 artom Since some (all?) of those got backported
17:29:35 melwitt mriedem: it might be related to the wonkiness I intended to fix with https://review.openstack.org/#/c/508432
17:29:53 melwitt because without that fix, every compute will write to the same cell db
17:30:14 mriedem i was going to see if putting your patch under this change fixes it
17:31:12 melwitt yeah, I think it would be worth trying but I'm not 100% it will solve your problem. what I'm thinking of is where compute services need to write records and they don't do it in a cell-aware way
17:31:53 dansmith yeah, any periodic will do the wrong thing I think
17:31:59 melwitt right
17:32:07 melwitt and during compute start
17:32:18 dansmith I think we need a more fundamental change before we can really be clean there
17:32:27 melwitt but I dunno what problem mriedem is hitting
17:32:39 mriedem i'll push it up in a sec
17:33:33 openstackgerrit Matt Riedemann proposed openstack/nova master: Enable cold migration with target host(2/2) https://review.openstack.org/408964
17:33:57 mriedem melwitt: this https://review.openstack.org/#/c/408964/111/nova/tests/functional/test_servers.py@3029
17:34:09 mriedem so takashi has this test_migrate_server_to_host_in_different_cell test,
17:34:16 mriedem which creates 1 host in different cells,
17:34:34 mriedem creates a server on one of the hosts and tries to force the migration to the other host in the other cell, which should fail
17:34:38 mriedem b/c no cross-cell migration
17:35:02 melwitt yeah, the compute node records won't go in the right db without my patch I think
17:35:04 mriedem it does fail with NoValidHost, but i wanted to test how good the test was, so i pulled it down and changed is so that both hosts would be in the same cell, and the test passed
17:35:12 melwitt they'll all go in the same cell, the default cell1
17:35:18 mriedem ok
17:35:40 mriedem so i found that if there is more than one host in a cell, like ServerTestV256RescheduleTestCase, then fake.set_nodes has to be used
17:35:55 mriedem but when i tried to use fake.set_nodes on hosts in different cells, things would fail
17:36:06 dansmith if the periodic runs to update the nodes, it'll put them back in the wrong cell though right?
17:36:18 dansmith er, create duplicates in the wrong cell I mean
17:36:26 melwitt I have seen that before doing single cell functional testing with multiple computes, that set_nodes is needed to have the scheduler consider both computes
17:36:55 mriedem all of our server moving tests in nova.tests.functional.test_servers create 2 computes in the same cell and have to use fake.set_nodes
17:37:03 mriedem but we don't have any multi-cell functional tests like that
17:37:25 mriedem this is really just a negative test, but i wanted to make sure it was actually failing for the right reasons
17:37:29 melwitt mriedem: it's not the set_nodes thing but the ComputeNode record itself that you'd want to target to separate cells and set_nodes won't do that. set_nodes doesn't write the compute node record
17:37:38 mriedem since you can't actually tell what the novalidhost was for on the api caller side
17:37:51 melwitt the compute node record is written by the fake compute service starting up
17:37:57 mriedem i'll pull your change down and put this on top and use fake.set_nodes and see what happens
17:38:03 melwitt okay
17:39:50 dansmith melwitt: won't your patch result in compute node records written into cell0 (which should never happen) if the periodic runs?
17:40:32 dansmith the resource tracker update periodic that creates and destroys compute nodes on the fly for things like ironic
17:40:49 melwitt dansmith: I think I make the db association when the fake service is created and wrapped all the service calls so that the periodics would run in the right cell. but let me look again, it's been a long time
17:40:49 dansmith it'll find that virt reports a compute node that it doesn't find in the database and create it
17:41:19 dansmith hmm, not sure how you could do that for periodics that run later
17:42:27 mriedem still fails
17:42:46 melwitt dansmith: I think periodics are run by the service, so I did this https://review.openstack.org/#/c/508432/9/nova/test.py to target them to the associated cell
17:42:54 dansmith oh you patch periodic_tasks
17:43:13 dansmith not sure that's enough necessarily,
17:43:27 dansmith because anything that stores a context somewhere might not actually make that call in that path
17:43:49 melwitt it's better than what we have :P but yeah, I have been wanting people to review it to see what gaps could be addressed
17:44:11 dansmith well, it's not if we end up with spurious failures when we get unlucky and run a periodic in the middle of a slow test run
17:44:46 mriedem i left comments on the fixture patch
17:44:49 melwitt dansmith: what do you mean by "anything that stores a context somewhere"? the periodics in the compute service all use an anonymous admin context
17:45:16 dansmith melwitt: well, if something like RT has a context stored somewhere
17:46:07 melwitt dansmith: sure. I couldn't think of if/how it could be bullet-proofed
17:46:29 dansmith I think the thing we really should be doing is just make the compute manager grab a context on service start,
17:46:48 dansmith and wipe through compute/* and make sure nothing ever calls get_admin_context() or context.RequestContext() in there
17:46:53 dansmith sort of how we banned db access
17:47:18 melwitt yeah, that would be nicer
17:47:24 dansmith I think that also,
17:47:36 melwitt still, that doesn't stop anyone from doing something else, which is what you were pointing out
17:47:44 dansmith someone with "security" in their title would like to see that we can't just call foo.get_admin_credentials() anywhere
17:48:02 mriedem i think mikal is now head of security at aptira
17:48:13 mriedem securinaut or something
17:48:13 dansmith melwitt: well, it does if we actually clean that up to the point of making sure it won't work, like the db stuff
17:48:56 melwitt one thing I realized just now is, a context is handed off when the periodic tasks are kicked off once, and it reuses that. it's just that in the service code it uses get_admin_context for that
17:50:00 melwitt periodic_tasks is called once during service.start
17:50:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix ValueError when loading old pci device record https://review.openstack.org/523914
17:50:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove vestigial extra_info update in PciDevice.save() https://review.openstack.org/523919
17:50:37 dansmith yeah, but your wrapping of it ensures that anyway
17:51:07 melwitt but I thought you were saying "what if compute manager uses a different context" and it can't
17:51:08 openstackgerrit Chris Dent proposed openstack/nova master: [placement] Enable limiting GET /allocation_candidates https://review.openstack.org/513526
17:51:10 melwitt https://github.com/openstack/nova/blob/master/nova/service.py#L294-L295
17:51:35 dansmith melwitt: I'm saying if we have another context we got from not the periodic one we were handed, not in the scope of the periodic itself
17:51:44 dansmith we have contexts everywhere, on every object, etc

Earlier   Later