| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-10 | |||
| 10:08:55 | openstackgerrit | Qiu Fossen proposed openstack/nova master: Correct project/user id descriptions with nova-server-actions https://review.opendev.org/670027 | |
| 11:36:55 | openstackgerrit | Chris Dent proposed openstack/nova master: WIP: Add conf to not update to placement during instance_claim https://review.opendev.org/670041 | |
| 11:59:57 | openstackgerrit | huanhongda proposed openstack/nova stable/pike: [Stable Only] libvirt: Handle volume API failure in post_live_migration https://review.opendev.org/670016 | |
| 12:48:57 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Defaults missing group_policy to 'none' https://review.opendev.org/657796 | |
| 13:26:34 | openstackgerrit | Eric Fried proposed openstack/nova master: Make sure update resource usage when revert_resize https://review.opendev.org/665138 | |
| 13:49:45 | sean-k-mooney | mriedem: mind if i push a slightly modified version of your coverage patch | |
| 13:49:55 | sean-k-mooney | i have a version that mostly works | |
| 13:50:40 | openstackgerrit | Takashi NATSUME proposed openstack/nova stable/stein: doc: Fix a parameter of NotificationPublisher https://review.opendev.org/670064 | |
| 13:50:51 | sean-k-mooney | we only get an extra 2% form the functional test by the way | |
| 13:50:54 | sean-k-mooney | TOTAL 82128 5924 24452 2240 91% | |
| 13:51:31 | yonglihe | sean-k-mooney, thanks for the review. For a instance is deleted, checking the states is not reliable at least for test case it's not correct. The instance.Destroy action call DB delete, which then make the instance.id attr gone. Instance.id not exist means the instance is 'deleted'. | |
| 13:51:54 | sean-k-mooney | yonglihe: your current code can delete active instnces | |
| 13:52:11 | yonglihe | that what i'm talking about. | |
| 13:52:29 | yonglihe | https://www.irccloud.com/pastebin/BSYfEoER/ | |
| 13:52:53 | yonglihe | "not instance.obj_attr_is_set('id')" means instance is deleted. | |
| 13:53:05 | mriedem | sean-k-mooney: sure | |
| 13:53:20 | sean-k-mooney | yonglihe: im not sure that is correct | |
| 13:53:23 | mriedem | sean-k-mooney: btw i meant to split that out to do some of the cleanup stuff separately and only try to merge the functional tests in at the end | |
| 13:53:29 | mriedem | sean-k-mooney: i can do that split after you update it | |
| 13:53:39 | mriedem | i basically wanted to fire and forget yesterday | |
| 13:54:28 | sean-k-mooney | well this is actully the diff i was going to push http://paste.openstack.org/show/754262/ | |
| 13:55:06 | sean-k-mooney | we cant remove the python hack entirely as running stestr on its own does not record any coverage info | |
| 13:55:35 | sean-k-mooney | the -p groups parrallel test by class | |
| 13:55:50 | sean-k-mooney | this gets rid of almost all the mocking issues | |
| 13:56:05 | sean-k-mooney | i have only 4 failing tests with that | |
| 13:56:10 | mriedem | go ahead and push it | |
| 13:56:17 | cdent | shouldn't we not mask the mocking problems, and fix them instead? | |
| 13:56:17 | sean-k-mooney | sure will do | |
| 13:56:22 | mriedem | i'm doing stable release reviews while tony is on vacation.... | |
| 13:56:28 | mriedem | so i'm going to be stuck for awhile | |
| 13:56:31 | yonglihe | sean-k-mooney, check this line: https://github.com/openstack/nova/blob/de31466fdb09dd367b23660f71b4ca06f83271a2/nova/objects/instance.py#L611 | |
| 13:56:44 | sean-k-mooney | cdent: we should but it proably better to do that after getting the test running | |
| 13:56:45 | yonglihe | I'm not sure either. | |
| 13:56:53 | yonglihe | seems correct. | |
| 13:57:13 | cdent | sean-k-mooney: i was assuming that the test failing was the only way of identifying the failures. tdd and all that | |
| 13:57:58 | sean-k-mooney | cdent we can remove the -p to get them to fail again in the patch that fixes them | |
| 13:58:21 | cdent | or we will just ignore it for a few more years :) | |
| 13:58:34 | sean-k-mooney | well if its not broke... | |
| 13:58:36 | cdent | would love to continue talking about this, but unfortunately must head ot appt | |
| 13:59:03 | aarents | Hi there, it will be great if someone can have a look to this reproducible bug and related fix: https://review.opendev.org/#/c/670000 | |
| 13:59:10 | yonglihe | fake-db does had this behavior for deleted instance, and i also saw while deleted, we save instance.id to instance.delete filed. | |
| 13:59:18 | sean-k-mooney | there is a test isolation analasys flag in stestr to figure out what test cause issues | |
| 13:59:35 | sean-k-mooney | so im hoping i can just let that run and get a list of the broken tests | |
| 14:01:55 | yonglihe | sean-k-mooney: I did not got time to test that on machine. I gonna do smoke test tomorrow to make sure it won't delete active instance. Other comments seems reasonable and good, thanks. | |
| 14:03:06 | sean-k-mooney | ok i didnt run it locally to test. but that was the main reason for the -1 | |
| 14:03:36 | yonglihe | and here is another wired thing i running into. for the unit test | |
| 14:03:43 | yonglihe | https://www.irccloud.com/pastebin/Fep61BPV/ | |
| 14:04:12 | yonglihe | the assert_calls say 'Calls not found.' | |
| 14:04:49 | sean-k-mooney | by the way an instance not having id set does not mean its deleted it means it has not be created in the db | |
| 14:05:05 | yonglihe | but the instance in expect and actual actually just same (but might had wrong order). | |
| 14:05:07 | sean-k-mooney | or nolonger exists in the db | |
| 14:06:26 | yonglihe | sean-k-mooney: Is it possible only database know what is deleted? and we can not get that information from the instance ? | |
| 14:06:34 | sean-k-mooney | anyway that sinip is a little hard to read but you can proably do an order independ check if that is not important | |
| 14:07:09 | yonglihe | sean-k-mooney : i don't find one to ignore the order. | |
| 14:07:56 | sean-k-mooney | you would have to write it. just asser the call object is in the mock.calls array | |
| 14:08:08 | sean-k-mooney | but you would have to construct a mock call object to do that | |
| 14:08:10 | yonglihe | replace wiht assert_called_with also does not work, assert_called_with still checking the order, that's so wired for me. | |
| 14:08:32 | sean-k-mooney | normally we want the order to be checked | |
| 14:09:00 | sean-k-mooney | we have some examples of doing order independ dict comparisons i think | |
| 14:09:05 | yonglihe | seems i had to assert the call is in that array. | |
| 14:10:43 | sean-k-mooney | https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_has_calls | |
| 14:10:50 | yonglihe | I would like to find the example, that's should work. but checking if instance is deleted seems hard? any other condition i can check? | |
| 14:11:04 | sean-k-mooney | you can do mock.assert_has_calls(calls, any_order=True) | |
| 14:11:11 | yonglihe | sean-k-mooney: thanks, got it. | |
| 14:12:35 | sean-k-mooney | if you use assert_has_calls you have to create teh mock calls manually like this https://github.com/openstack/nova/blob/de31466fdb09dd367b23660f71b4ca06f83271a2/nova/tests/unit/utils.py#L323-L336 | |
| 14:14:42 | sean-k-mooney | here are some example you can follow https://github.com/openstack/nova/search?q=assert_has_calls+any_order%3DTrue&unscoped_q=assert_has_calls+any_order%3DTrue | |
| 14:17:26 | yonglihe | thanks. for the 'delete' states, i think the 'deleted_at' filed been set seems could filter out the instance is creating at that moment. | |
| 14:18:20 | yonglihe | 'no id attr' and 'deleted_at' is set before now. | |
| 14:18:39 | sean-k-mooney | perhap but again i would use the instance status personally unless you have an example where that does not work | |
| 14:19:48 | yonglihe | not work in the unite test, that's why i checking other stuff. but the unit test just call the destroy, not set the states right. | |
| 14:20:24 | sean-k-mooney | yonglihe: then why not fix the unit tests | |
| 14:20:51 | sean-k-mooney | anyway ill leave it up to you | |
| 14:21:14 | yonglihe | Sure. do we need checking other stuff if we depend on he vm_state filed? | |
| 14:21:19 | sean-k-mooney | leave a comment in the next version based on your testing | |
| 14:21:33 | yonglihe | Sure. | |
| 14:21:44 | sean-k-mooney | e.g. why you are doing whatever test you come up with | |
| 14:23:11 | sean-k-mooney | im not sure but i need to go work on somehting before a meeting soon | |
| 14:23:29 | yonglihe | I gonna to verify if that code working on real stack. thanks. I got to run also. | |
| 14:23:30 | sean-k-mooney | i think the task state + checking for soft delete would be enough | |
| 14:23:56 | yonglihe | Ok , i gonna checking that, | |
| 14:24:14 | yonglihe | Have a good day. | |
| 14:32:19 | sean-k-mooney | johnthetubaguy: by the way i -1 the unifed limits spec but if just declare the discoverablity as out of scope to be resolved in a futrue version of the sdk or in a follow up spec then im fine to swap to +1 on that spec | |
| 14:32:48 | sean-k-mooney | once the nits inline are adressed that is | |
| 14:33:09 | johnthetubaguy | sean-k-mooney: cool, I will respin | |
| 14:34:34 | sean-k-mooney | is it safe to assume that the keysone limits api wont have limits for compute resouces if we have not enabled it for nova | |
| 14:35:23 | sean-k-mooney | if so the sdk could check that once an cache it or something like that to determin if it should use keystone limits or nova | |
| 14:36:54 | sean-k-mooney | so the actully check would be. 1.) is keystone version new enough to support limtis 2.) are instance limits reported in keysonte limits api, if yes to both use keystone if the either are false use nova | |
| 14:37:56 | sean-k-mooney | johnthetubaguy: would somthing like ^ work | |
| 14:40:00 | johnthetubaguy | sean-k-mooney: Its not a smooth transition that way... nova might still be ignoring keystone, but it might be good enough (assuming default keystone policy means you can list the limits of your project) | |
| 14:40:20 | johnthetubaguy | it gets worse with hierarchy mind, as everything seems to | |
| 14:41:18 | sean-k-mooney | well we dont need to encode that in the spec i was just wondering if there was a huristic the sdk could resonably use | |
| 14:42:27 | sean-k-mooney | it could be as simple as stick it in a config option for now | |
| 14:43:45 | sean-k-mooney | the clouds.yaml has profiles for clouds that encode there knonw behavior for non discoverable things | |
| 14:43:56 | sean-k-mooney | so that is always an option for limits | |
| 14:44:41 | sean-k-mooney | at least i think nodepool gets that stuff from clouds.yaml for the sdk | |
| 14:52:06 | openstackgerrit | Dakshina Ilangovan proposed openstack/nova-specs master: Spec: Provider config YAML file https://review.opendev.org/612497 | |
| 14:59:56 | spatel | sean-k-mooney: did you ever see this error http://paste.openstack.org/show/754267/ | |
| 15:07:05 | stephenfin | johnthetubaguy: Seeing as you've been reviewing docs, could you have a look at https://review.opendev.org/#/c/667164/ ? | |
| 15:07:29 | johnthetubaguy | stephenfin: sure | |
| 15:15:37 | openstackgerrit | John Garbutt proposed openstack/nova-specs master: Add Unified Limits Spec https://review.opendev.org/602201 | |