| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-10 | |||
| 08:09:22 | openstackgerrit | Yongli He proposed openstack/nova master: clean up orphan instances https://review.opendev.org/627765 | |
| 08:09:22 | openstackgerrit | Yongli He proposed openstack/nova master: Clean up orphan instances virt driver https://review.opendev.org/648912 | |
| 08:19:04 | alex_xu | bauzas: do we have a short race window between allocate the gpu mdev and create the domain in the libvirt https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L3235 ? | |
| 08:19:34 | alex_xu | bauzas: I'm reviewing the vpmem patches, and thinking of where we should allocate the actual device | |
| 08:21:09 | bauzas | alex_xu: AFAIK nope, because we already synchronize https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1865 | |
| 08:22:07 | bauzas | it calls https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1873 which eventually calls spawn() https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2251 | |
| 08:22:18 | alex_xu | bauzas: that is for avoid mulitple action for a singple instance, but if there are multiple instances are creating at same time | |
| 08:24:10 | bauzas | mmmm you're right | |
| 08:24:29 | alex_xu | bauzas: it is only protected by https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6415 | |
| 08:24:52 | bauzas | yup indeed | |
| 08:25:24 | alex_xu | bauzas: but there still have a small window between https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L3235 and https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L3241 | |
| 08:26:08 | bauzas | alex_xu: sure but we only assigned the mdev in the synchronized method | |
| 08:26:16 | bauzas | then we only define the XML | |
| 08:26:29 | bauzas | so two instances would have different mdevs | |
| 08:26:41 | bauzas | two concurrent* | |
| 08:28:00 | alex_xu | bauzas: but the available device is calculated by all - free, the assigned device is get by list all the libvirt domain | |
| 08:28:36 | alex_xu | so both two instances get the available devices by listing existed domain in the libvirt, and may get the same dev | |
| 08:29:09 | alex_xu | bauzas: here is the code for populate the mdev devices https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6306 | |
| 08:29:30 | alex_xu | s/all - free/all - assigned/ | |
| 08:29:41 | bauzas | alex_xu: oh shit you're right | |
| 08:30:05 | bauzas | given we're defining the XML out of that semaphore, the assigned mdev couldn't be seen concurrently | |
| 08:30:18 | bauzas | excellent point | |
| 08:30:29 | bauzas | alex_xu: worth shouting a patch ? or want me to do so ? | |
| 08:30:58 | alex_xu | bauzas: you can fix that if you want, actually I'm looking for a solution for pmem | |
| 08:31:14 | alex_xu | bauzas: also do we support resize for the instance with vgpu? | |
| 08:32:01 | bauzas | alex_xu: I'm working on it | |
| 08:32:07 | bauzas | alex_xu: I have a patch up | |
| 08:32:18 | bauzas | but I need to work on revert_resize too | |
| 08:32:20 | alex_xu | bauzas: where are you claim the mdev? | |
| 08:33:08 | bauzas | alex_xu: first, something is missing https://review.opendev.org/#/c/589085/ | |
| 08:34:04 | alex_xu | bauzas: that doesn't include the mdev claim | |
| 08:34:25 | alex_xu | so I'm thinking whether we should manage those device in the resource tracker instead of virt driver | |
| 08:34:39 | alex_xu | s/manage those device/tracking those resource/ | |
| 08:36:52 | bauzas | sorry in meeting | |
| 08:37:18 | alex_xu | bauzas: no worries, catch up you later | |
| 09:07:29 | bauzas | alex_xu: I'm back | |
| 09:08:27 | bauzas | alex_xu: so, we don't technically claim mdevs | |
| 09:08:43 | bauzas | alex_xu: as we do with PCI devices | |
| 09:08:47 | openstackgerrit | huanhongda proposed openstack/nova stable/pike: [Stable Only] libvirt: Handle volume API failure in post_live_migration https://review.opendev.org/670016 | |
| 09:18:49 | openstackgerrit | huanhongda proposed openstack/nova stable/pike: [Stable Only] libvirt: Handle volume API failure in post_live_migration https://review.opendev.org/670016 | |
| 09:35:30 | openstackgerrit | Merged openstack/nova master: doc: Fix a parameter of NotificationPublisher https://review.opendev.org/669993 | |
| 09:44:00 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: Enable driver configures PMEM namespace https://review.opendev.org/634549 | |
| 09:58:03 | alex_xu | bauzas: yes, mdevs needn't claim, you can free to choice any one of free mdevs | |
| 09:59:50 | alex_xu | bauzas: vpmem needs the claim, since we need to copy the vpmem data to the destination vpmem | |
| 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 | 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 | |