| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-30 | |||
| 14:43:12 | cdent | or rather the fact that we hide the error response in the expection message made it hard to debug | |
| 14:43:16 | jaypipes | cdent: well, the whole "the error is saying I used a bad UUID, when I actually used a bad name" is problematic. | |
| 14:43:26 | cdent | that's a different bug: | |
| 14:43:26 | jaypipes | cdent: since they both have the same error message | |
| 14:43:30 | jaypipes | cdent: ack | |
| 14:43:42 | cdent | https://bugs.launchpad.net/nova/+bug/1735405 | |
| 14:43:44 | efried | So let's say we do grep the error string - how would report client react that would be appropriate here? | |
| 14:43:44 | openstack | Launchpad bug 1735405 in OpenStack Compute (nova) "Error message from placement when creating resource provider uses ambiguous identifier" [Undecided,New] | |
| 14:43:53 | jaypipes | ok, agreed, diff bug | |
| 14:43:55 | cdent | the db duplicate response message does include which columns were duplicated | |
| 14:44:03 | cdent | I changed that some months ago | |
| 14:44:10 | cdent | but we hide that in the report client, and shouldn't | |
| 14:44:15 | jaypipes | efried: raise an exception back to the caller. | |
| 14:44:29 | jaypipes | cdent: gotcha | |
| 14:44:39 | cdent | why do we need to grep the error response? | |
| 14:44:48 | cdent | if uniqueness fails, uniqueness fails | |
| 14:44:58 | cdent | you can only get a 409 from post to /resource_providers for uniqueness fails | |
| 14:45:00 | jaypipes | cdent: in order to log an appropriate message, that's all | |
| 14:45:17 | cdent | pass on the response's error message, no need to parse it, just use it | |
| 14:45:17 | jaypipes | cdent: it's just misleading to write an error saying bad UUID, when it was the name that was problematic | |
| 14:45:40 | jaypipes | cdent: sure, can do that | |
| 14:46:09 | efried | Hold on | |
| 14:46:13 | efried | I don't agree | |
| 14:46:45 | efried | Right now we assume it's UUID conflict, so we GET the provider by UUID and return it (or None) | |
| 14:47:03 | cdent | the code is incomplete | |
| 14:47:16 | efried | Right | |
| 14:47:28 | cdent | if we want to assume that a 409 _could_ be "another thread created" | |
| 14:47:33 | cdent | then yes, parsing it required | |
| 14:47:52 | cdent | (or adding more detailed error codes, eventually) | |
| 14:48:36 | gibi | gmann_afk: +W | |
| 14:49:47 | cdent | but, then we have 409 meaning potentially 3 things: uuid uniqueness violation (good luck!), name uniqueness violation, concurrent violation | |
| 14:50:09 | cdent | and while telling some of those apart is gravy, the uuid ones, unclear | |
| 14:50:12 | jaypipes | cdent: right. thus the need for more specific error codification. | |
| 14:50:35 | cdent | right, but even with better error codification how do you distinguish conficts 1 and 3? | |
| 14:50:43 | bauzas | oh shit, DST shift impact :( | |
| 14:50:46 | bauzas | mriedem: ^ | |
| 14:50:53 | efried | cdent We have to assume #1 doesn't happen. | |
| 14:51:02 | efried | Or rather, assume the risk | |
| 14:51:51 | efried | Realistically, we don't get to that point if there's a UUID "conflict" - we would have just returned that provider from the first GET in _ensure_resource_provider. | |
| 14:51:57 | mriedem | bauzas: ? | |
| 14:52:38 | cdent | brb | |
| 14:52:49 | mriedem | sdague: i think you will enjoy this https://review.openstack.org/#/c/507693/ | |
| 14:53:20 | efried | cdent jaypipes IMO the first thing we should do is the simplest: raise exceptions instead of returning None in those various paths. | |
| 14:54:03 | efried | Today that 404 will be indicating name conflict 99% of the time. | |
| 14:55:07 | efried | The only excepton would be concurrent create-followed-by-delete. So I'd even go so far as to say 99.99999% | |
| 14:55:09 | cdent | efried: yeah, I agree None is not right | |
| 14:55:13 | jaypipes | efried: go for it. | |
| 14:55:27 | cdent | you'll have to have the fun of figuring out what's up with the api_samples test :( | |
| 14:55:47 | efried | Exactly. jaypipes it looks like this: http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz | |
| 14:56:24 | efried | It happens because those test cases are starting the fake compute host twice. Each one seems to be getting its own database. But still just one placement service. | |
| 14:56:57 | sdague | mriedem: nice, those test cleanups are especially nice | |
| 14:57:01 | efried | We create ComputeNode with a name and let the DB assign its UUID. Then we push that guy to placement. So the second time around, it has a different UUID. | |
| 14:57:13 | cdent | efried: i don't think "own database" is the right analysis. something else is going on, like the second node (not host), clobbering the first | |
| 14:57:51 | efried | cdent Okay. FWIW, I pdb'd it, and the compute host definitely ain't there the second time around. | |
| 14:58:42 | efried | I don't really have context on how these fixtures do their business. Who would be an expert there? | |
| 14:59:22 | cdent | sdague might have some insight into efried's problem? | |
| 15:01:55 | mriedem | efried: same cell? | |
| 15:02:00 | mriedem | i.e. it's a single-cell test right? | |
| 15:02:10 | takashin | nova-specs cores, would you take a look at a patch to create specs directory for Rocky https://review.openstack.org/#/c/514101/ ? | |
| 15:02:11 | mriedem | could be an issue with not using fake.set_nodes() and fake.restore_nodes | |
| 15:02:12 | efried | mriedem I don't know that answer. | |
| 15:02:28 | efried | mriedem http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz | |
| 15:05:37 | openstackgerrit | Jackie Truong proposed openstack/python-novaclient master: Microversion 2.56 - Add trusted_image_certificates https://review.openstack.org/500396 | |
| 15:06:28 | takashin | a patch to create specs directory for Rocky https://review.openstack.org/#/c/514101/ | |
| 15:09:13 | openstackgerrit | Matthew Booth proposed openstack/nova master: Simplify BDM boot index checking https://review.openstack.org/524208 | |
| 15:10:17 | sdague | what's up? | |
| 15:10:32 | sdague | efried: can you summarize the issue, maybe I can help | |
| 15:11:25 | efried | sdague If report client tries to ensure a RP exists, and gets any error, it returns None (rather than raising an exception) but nobody is checking for that. | |
| 15:11:45 | efried | sdague In this test: http://logs.openstack.org/32/523932/2/check/openstack-tox-functional/05f8149/testr_results.html.gz we're hitting that code path | |
| 15:11:45 | mriedem | efried: these are a single cell test, using the SingleCellSimple | |
| 15:12:04 | mriedem | efried: looking at HypervisorsSampleJson233Tests, it's creating a 2nd service | |
| 15:12:10 | mriedem | but not setting unique nodes | |
| 15:12:42 | efried | sdague As long as we keep the `return None` in there, the test works; but if I raise an exception, the test fails because, as mriedem is saying, it's creating a second service, which creates a second ComputeNode with the same name but different UUID. | |
| 15:12:51 | mriedem | _IntegratedTestBase, the base class, creates a compute service, and the HypervisorsSampleJson test creates a compute service, but i think they have the same node name | |
| 15:13:01 | efried | sdague When we try to create that in placement, we get that 409 because the *name* conflicts. | |
| 15:13:10 | sdague | mriedem: yeh, that's probably something we should prevent on the test side | |
| 15:13:21 | efried | mriedem Yes, they're both called fake-mini. | |
| 15:13:22 | mriedem | so you need to put this in HypervisorsSampleJson when creating the 2nd service: | |
| 15:13:30 | mriedem | fake.set_nodes(['host1']) | |
| 15:13:35 | mriedem | self.addCleanup(fake.restore_nodes) | |
| 15:13:36 | efried | I'm confused as to why it's not `host1` | |
| 15:13:40 | mdbooth | stephenfin: How sure are you of assert(), btw? | |
| 15:13:53 | mriedem | efried: because the fake driver is pullin the node name from CONF.host | |
| 15:13:58 | mriedem | if you don't use fake.set_nodes | |
| 15:14:09 | stephenfin | mdbooth: Not hugely sure. I just recall being told not to use it at some point in the past | |
| 15:14:11 | efried | But as far as I can see, CONF.host *should* be `host1` and `host2` | |
| 15:14:55 | mdbooth | stephenfin: I think I'm using it correctly in that patch, but there may be practical considerations. e.g. if we disable it in CI. | |
| 15:14:57 | mriedem | efried: ala https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L1283 | |
| 15:15:18 | mdbooth | If it's disabled in production, we don't really care. | |
| 15:15:32 | efried | mriedem Okay, thanks, trying... | |
| 15:15:59 | cdent | efried: I can't remember, is the rp ending up named fake-mini or 'host?' ? | |
| 15:16:07 | efried | cdent fake-mini | |
| 15:16:17 | mdbooth | mriedem: Do you happen to remember, btw: all(el + 1 == l[i + 1] for i, el in enumerate(l[:-1])) ? | |
| 15:16:25 | mriedem | melwitt: so i've got this code now http://paste.openstack.org/show/627877/ | |
| 15:16:37 | mdbooth | You chose to add a comment to it for some reason :) | |
| 15:16:44 | mriedem | melwitt: which fails with this http://paste.openstack.org/show/627878/ | |
| 15:16:54 | mriedem | so i'm creating host2 in cell2 but when i try to pull host2 out of cell2, it's not found | |
| 15:17:06 | mriedem | i'm assuming that's because of the CellDatabase fixture defaulting to cell1 | |
| 15:17:15 | mriedem | which is weird since i'm targeting the context specifically to cell2 | |
| 15:17:26 | mriedem | but i can replay this on top of your fixture cleanup patch and see if that fixes it | |