Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-10
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 sean-k-mooney sure will do
13:56:17 cdent shouldn't we not mask the mocking problems, and fix them instead?
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
15:15:48 openstackgerrit Stephen Finucane proposed openstack/nova master: ec2: Remove unused functions from 'ec2utils' https://review.opendev.org/662501
15:15:49 openstackgerrit Stephen Finucane proposed openstack/nova master: ec2: Pre-move cleanup of utils https://review.opendev.org/662504
15:15:49 openstackgerrit Stephen Finucane proposed openstack/nova master: ec2: Remove ec2.CloudController https://review.opendev.org/662503
15:15:49 openstackgerrit Stephen Finucane proposed openstack/nova master: objects: Remove unused ec2 objects https://review.opendev.org/662502
15:15:50 openstackgerrit Stephen Finucane proposed openstack/nova master: ec2: Move ec2utils functions to their callers https://review.opendev.org/662505
15:15:51 openstackgerrit Stephen Finucane proposed openstack/nova master: api: Remove 'Debug' middleware https://review.opendev.org/662506
15:19:07 openstackgerrit Stephen Finucane proposed openstack/nova master: vif: Resolve a TODO and update another https://review.opendev.org/666631
15:19:07 openstackgerrit Stephen Finucane proposed openstack/nova master: vif: Stop using getattr for VIF lookups https://review.opendev.org/666630
15:19:08 openstackgerrit Stephen Finucane proposed openstack/nova master: vif: Remove dead minimum libvirt checks https://review.opendev.org/666814
15:20:35 Nick_A We are trying to offload the _base nova image cache using cephfs. our idea was to do a read-only mirror of glance's image pool and mount that to _base on compute. Would nova have problems with that setup?
15:21:04 openstackgerrit Merged openstack/nova master: Perf: Use dicts for ProviderTree roots https://review.opendev.org/637225

Earlier   Later