| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-29 | |||
| 20:43:09 | melwitt | we still have customers that need a fix for it | |
| 20:43:13 | ameeda | mriedem: yes, but my bug scenario doesn't hit _local_delete, so we have to cover all the cases, right ? | |
| 20:44:04 | mriedem | melwitt's patch is fixing a local delete case | |
| 20:44:08 | mriedem | where the instance isn't on a compute host, | |
| 20:44:19 | mriedem | so it is either shelved offloaded or failed during scheduling | |
| 20:44:24 | melwitt | it is on a compute host, it's just in ERROR state | |
| 20:44:29 | mriedem | 'local delete' == delete from the API, not compute | |
| 20:45:07 | melwitt | there's code in compute manager that sets host = None while setting to ERROR state if something fails. IIRC | |
| 20:46:53 | melwitt | like this https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1941-L1943 | |
| 20:48:54 | ameeda | I am not sure about the case , but what about this fix https://review.openstack.org/#/c/528385/ | |
| 20:49:18 | ameeda | this detach volume when instance creation fails | |
| 20:51:22 | mriedem | ameeda: -1 on that one too | |
| 20:51:32 | mriedem | ameeda: we already attempt to detach volumes when deleting a server | |
| 20:51:41 | mriedem | so if that's failing, i don't think your patch is going to fix it | |
| 20:53:41 | ameeda | what is the case !, I just follow the bugs to fix them , after that I found that the bug is invalid ? | |
| 20:54:00 | mriedem | ameeda: well, you could be trying to fix a very old bug that's already been fixed | |
| 20:54:10 | mriedem | or the recreate scenario from the bug is not clear | |
| 20:55:44 | mriedem | so the thing to do here, is probably go through the recreate steps in the bug (which are pretty clear) and see if you can reproduce it | |
| 20:55:47 | mriedem | and if so, see where things are failing | |
| 20:56:24 | mriedem | the bug report was written over a year ago and doesn't say which version of nova they were using when they hit this, | |
| 20:56:26 | mriedem | so it might be fixed | |
| 20:56:37 | mriedem | i'm sorry that you invested time in a patch, but we need to make sure it's still a valid bug first | |
| 20:56:43 | ameeda | mriedem: but I reproduce the bug and fix it , then test it again to make sure that I fix the issue, and there are reviewers ask me to make changes on it :( | |
| 20:57:21 | ameeda | I am sure that the bug is still valid , I reproduce it | |
| 20:58:16 | mriedem | ok, so then why is the original detach failing in _shutdown_instance? | |
| 20:58:22 | mriedem | because that's what should be detaching the volume | |
| 21:02:15 | melwitt | mriedem: I think it's the same bug I'm fixing. I think he means volume quota was exceeded (it's checked in compute). compute catches the exceptions, nulls out the instance.host, then sets it to ERROR state | |
| 21:03:00 | melwitt | then when you go to delete the instance, because compute nulled out the host, the compute api thinks there's no resources to free up because it assumes instance.host = None means failed to schedule | |
| 21:03:44 | melwitt | so I think my patch will take care of it. I'm rebasing it now and rewriting the commit message to be clearer | |
| 21:03:46 | ameeda | my case when vm creations fails when there are no enough quota for volumes. as this https://review.openstack.org/#/c/528069/ scenario. | |
| 21:03:53 | melwitt | I know, that's what I just said | |
| 21:05:12 | ameeda | melwitt: you fix solve the issue when there is no hosts, my fix solve the issue in another case ! | |
| 21:05:29 | ameeda | mriedem: what do you think ? | |
| 21:05:56 | melwitt | it's not another case. compute will set instance.host = None because of the volume quota exceeded | |
| 21:07:28 | ameeda | ok great !! | |
| 21:08:59 | ameeda | duplicated bugs , and my work and time have gone in vain | |
| 21:29:39 | efried | mriedem: Any tips for how I can check in nova.tests.functional.test_servers that a particular exception got raised up from _update() ? | |
| 21:30:34 | efried | So far my mechanism for getting _update() to run is self._run_periodics() | |
| 21:37:09 | melwitt | efried: I think that's the usual way to run it | |
| 21:38:00 | efried | melwitt: I think I'm going to have to deconstruct it a bit and run rt.update_available_resource directly. | |
| 21:38:52 | melwitt | efried: any reason you're trying to verify that in a func test instead of a unit test? | |
| 21:39:17 | efried | melwitt: A reasonable question. | |
| 21:39:26 | melwitt | if there's no way to detect what you expect from the instance or other things you can inspect in the func test | |
| 21:39:29 | efried | melwitt: I was hoping to avoid unit test altogether. | |
| 21:39:56 | melwitt | so there's an existing func test and you want to add an assertion to it? | |
| 21:40:36 | efried | melwitt: I'm writing a new func suite. Stand by, I'll put up what I've got. | |
| 21:43:04 | openstackgerrit | Eric Fried proposed openstack/nova master: New-style _set_inventory_for_provider https://review.openstack.org/537648 | |
| 21:43:04 | openstackgerrit | Eric Fried proposed openstack/nova master: SchedulerReportClient.update_from_provider_tree https://review.openstack.org/533821 | |
| 21:43:04 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Use update_provider_tree from resource tracker https://review.openstack.org/520246 | |
| 21:43:05 | openstackgerrit | Eric Fried proposed openstack/nova master: Fix nits in update_provider_tree series https://review.openstack.org/531260 | |
| 21:43:06 | openstackgerrit | Eric Fried proposed openstack/nova master: Move refresh time from report client to prov tree https://review.openstack.org/535517 | |
| 21:44:06 | efried | melwitt: Right here is where I want to make sure that exception happened: https://review.openstack.org/#/c/520246/41/nova/tests/functional/test_servers.py@1615 | |
| 21:44:20 | efried | melwitt: And similar for other kinds of error conditions. | |
| 21:44:45 | efried | melwitt: I mean, I'm at least proving the whole thing doesn't blow up when I inject error conditions, which is good. | |
| 21:49:08 | melwitt | I see. I suppose you could monkey patch _update (or whatever) to call the real _update inside self.assertRaises. I wonder if that would work | |
| 21:51:33 | efried | melwitt: Well, I don't see _run_periodicals really doing anything that adds value to my narrow little test case. | |
| 21:52:31 | melwitt | so you don't want to assert a particular exception is raised? I'm confused :) | |
| 21:53:15 | efried | I do, and the only reason I can't do assertRaises(self._run_periodicals) is because _run_periodicals invokes x, which invokes y, which invokes z, which swallows exceptions. | |
| 21:53:24 | efried | So I'm going to dig in and invoke z directly, so I can assertRaises(z) | |
| 21:53:44 | efried | ...which I just did, and it seems to work a treat! melwitt thanks for sounding boarding :) | |
| 21:54:06 | efried | Now everyone will want to do what I just did | |
| 21:54:09 | melwitt | yeah, that's what I was trying to say, monkey patch the method that will raise it and just call the real method inside self.assertRaises | |
| 21:56:31 | efried | The monkey patching isn't needed | |
| 21:56:52 | melwitt | oh, so run that instead of periodics | |
| 21:56:55 | melwitt | I see | |
| 21:56:58 | efried | just so. | |
| 21:57:17 | efried | Just needed a few extra LOC to dig into the call stack a couple more levels. | |
| 22:09:25 | openstackgerrit | Jay Pipes proposed openstack/nova master: tests for alloc candidates with nested and traits https://review.openstack.org/531899 | |
| 22:09:26 | openstackgerrit | Jay Pipes proposed openstack/nova master: add test for scenario with sum of child resources https://review.openstack.org/534339 | |
| 22:09:26 | openstackgerrit | Jay Pipes proposed openstack/nova master: _get_trees_matching_all_resources() sum children https://review.openstack.org/534866 | |
| 22:09:27 | openstackgerrit | Jay Pipes proposed openstack/nova master: add _get_usages_from_trees() function https://review.openstack.org/534967 | |
| 22:09:27 | openstackgerrit | Jay Pipes proposed openstack/nova master: validate provider summary structure https://review.openstack.org/536516 | |
| 22:09:28 | openstackgerrit | Jay Pipes proposed openstack/nova master: WIP - populate alloc cands for trees https://review.openstack.org/534968 | |
| 22:26:42 | openstackgerrit | Matt Riedemann proposed openstack/nova master: reno for notification-transformation-queens https://review.openstack.org/518018 | |
| 22:30:11 | mriedem | dansmith: want to hit this pike backport? https://review.openstack.org/#/c/536981/ | |
| 22:30:26 | mgagne | what's the use case for aggregates in Placement API? | |
| 22:32:12 | mriedem | mgagne: the primary one is modeling things where one provider shares a resource with another, like a shared storage pool | |
| 22:33:12 | mgagne | mriedem: thanks for the clarification, I tried to read/understand https://developer.openstack.org/api-ref/placement/#resource-provider-aggregates | |
| 22:34:31 | mgagne | and I'm not sure the doc fully explain its purpose, I would have never guess what you said from the doc, it mainly explains how it's not nova aggregates | |
| 22:34:37 | mriedem | mgagne: is the spec that talks about aggregates the most i think https://specs.openstack.org/openstack/nova-specs/specs/newton/implemented/generic-resource-pools.html | |
| 22:34:46 | mriedem | mgagne: heh yeah that's what i was thinking | |
| 22:34:59 | mgagne | how is Generic Resource Pools == aggregates? =) | |
| 22:35:59 | mgagne | will read and see if I can get more answers | |
| 22:37:11 | mgagne | thanks for the link btw ;) | |
| 22:41:09 | openstackgerrit | Matt Riedemann proposed openstack/nova master: api-ref: provide more detail on what a provider aggregate is https://review.openstack.org/539033 | |
| 22:41:10 | mriedem | mgagne: see if ^ helps wording-wise | |
| 22:41:42 | mriedem | sorry about ending on a preposition | |
| 22:42:23 | mriedem | maybe it's not, me fail english | |
| 22:42:37 | mgagne | mriedem: I think it's enough to give a hint that aggregates do have a purpose other than not being nova aggregates =) | |
| 22:43:10 | mgagne | I'm still trying to digest the spec you linked above. | |
| 22:45:48 | mgagne | would it be ok to update past spec to reflect current implement? Ex.: openstack resource-provider is now openstack resource provider | |
| 22:52:21 | mriedem | mgagne: if it's not too invasive, then that's probably fine | |
| 22:52:25 | mriedem | especially for CLI examples | |
| 22:55:14 | openstackgerrit | Mathieu Gagné proposed openstack/nova-specs master: Update references to OSC resource provider command https://review.openstack.org/539038 | |
| 23:00:11 | efried | mgagne mriedem Sharing providers aren't the only use case FWIW. | |
| 23:00:33 | mriedem | efried: i'm not up to date on the new aggregate hotness | |
| 23:00:37 | mriedem | but yes i know sean-k-mooney has a thing | |
| 23:00:39 | mgagne | efried: please let me know if there is a doc I can read =) | |
| 23:00:43 | mriedem | mgagne: left some comments in there | |
| 23:01:03 | mriedem | mgagne: that spec was written in i think mitaka or newton, so the cli stuff was all future ideas | |