Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-24
14:56:23 bauwser johnthetubaguy: https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L311-L315
14:56:23 johnthetubaguy bauwser: thats just refreshing the list of instances in the group, at least I thought that was the intent there
14:56:41 bauwser johnthetubaguy: nope, not the instances
14:56:51 johnthetubaguy oh, I see
14:56:53 johnthetubaguy thats odd
14:57:05 bauwser johnthetubaguy: the InstanceGroup.hosts is set by the nodes having instances in there
14:57:11 johnthetubaguy do we fail to boot with the same things I guess?
14:57:16 bauwser johnthetubaguy: but it's a lazy-loadable field
14:57:22 mriedem bauwser: looks like you added that https://github.com/openstack/nova/commit/111a852e79f0d9e54228d8e2724dc4183f737397
14:57:54 bauwser mriedem: we did that in the past
14:58:00 bauwser mriedem: I just modified the callers
14:58:08 johnthetubaguy bauwser: that does do what I said right?
14:58:20 johnthetubaguy bauwser: the bit we need in there is re-populating the up to date group https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L350
14:58:33 johnthetubaguy bauwser: the other bits seem like nonsense for anything other than boot
14:58:48 bauwser johnthetubaguy: yup, we need to create those for boot
14:59:03 bauwser johnthetubaguy: but when we move that instance, then we get the existing field from the Spec object
14:59:11 johnthetubaguy bauwser: I would split out the boot checks, and call them separately in the boot flow right?
14:59:30 bauwser johnthetubaguy: given group.hosts is the only mutable field, it's fine given it's a lazy-loadable
14:59:43 johnthetubaguy I have no idea what you mean
14:59:51 johnthetubaguy sorry, I got lost
14:59:54 bauwser johnthetubaguy: my question is more about having permission to remove that call https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L162-L163
15:00:11 johnthetubaguy bauwser: I just said, thats really bad
15:00:23 bauwser johnthetubaguy: that's bad to what ? :)
15:00:29 johnthetubaguy bauwser: we need to update the group with the latest, but we don't want to do the boot time only checks
15:00:46 bauwser johnthetubaguy: we don't need to update the group info
15:00:56 johnthetubaguy bauwser: because of lazy loading?
15:00:58 bauwser johnthetubaguy: because the group info is made of 3 things
15:01:10 bauwser 2 can't be modified and the 3rd is lazy-loadable
15:01:11 bauwser right
15:01:22 johnthetubaguy but its saved into the json blob in the DB?
15:01:45 bauwser johnthetubaguy: if you see the code https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L162-L163 you will see that I don't update the Spec with that updated group already
15:01:54 johnthetubaguy I thought we had a bug where that list needed to be updated, because we persisted and out of date list
15:02:23 johnthetubaguy s/and/an/
15:03:00 bauwser johnthetubaguy: that was fixed by https://github.com/openstack/nova/blob/master/nova/objects/instance_group.py#L282-L289
15:03:03 johnthetubaguy I don't think we need the boot checks, I just through the filter params would need to be populated correctly, I guess I would have to go read more
15:03:33 bauwser I'm going to cut that call, and you could be reviewing it
15:03:36 johnthetubaguy bauwser: I am not convinced, but I guess you want some functional tests for that stuff really, then we can answer whats correct
15:04:59 bauwser johnthetubaguy: https://github.com/openstack/nova/commit/c48c1098cbcd7a9c7980d5fbe4668c38b16767f5 was missing a functional test but fixed the problem you mentioned
15:05:29 mriedem yeah seriously i'd write functional tests for anything we're removing
15:05:32 mriedem before removing it
15:05:40 mriedem because the unit tests just don't cut it for conductor <> scheduler interactions
15:05:41 johnthetubaguy what mriedem said, its safest
15:05:49 mriedem as we've been seeing lately with a lot of regressions
15:06:23 mriedem i'm still working on one for another regression
15:07:18 mriedem ooo and i think my regression test stumbled on another bug
15:08:42 mriedem ha, the fake virt driver doesn't set the image size on snapshot
15:08:54 mriedem something like that
15:09:23 bauwser mriedem: okay fair enough
15:10:04 mriedem oh it's the api code, not the fake virt driver
15:10:05 mriedem wtf
15:10:48 mriedem https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3355
15:11:02 mriedem we don't set the size attribute when creating the image for a shelving operation?
15:11:06 bauwser mriedem: johnthetubaguy: which specific functional tests would you like to see covered ?
15:11:23 bauwser mriedem: johnthetubaguy: the fact that we get a fresh list of hosts every time?
15:11:24 mriedem bauwser: anything related to whatever you're removing,
15:11:32 mriedem which i guess is a resize/migrate with an instance in a server group?
15:11:42 johnthetubaguy yeah, that ^
15:11:53 cdent leakypipes: I responded on your os-traits stuff. I hope that new comment makes what I mean a bit clear. Your response suggests that my initial comment missed you by a mile.
15:11:54 bauwser mriedem: that test is already in tree
15:11:58 mriedem bauwser: where?
15:12:09 bauwser functional/test_server_group out of mind
15:12:50 bauwser mriedem: but it's not covering the situation that I want to remove, ie. the fact that live-migration can fail if the filter is now disabled
15:13:22 bauwser mriedem: johnthetubaguy: https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py#L367
15:14:34 bauwser mriedem: johnthetubaguy: I could be writing a test basically identical to https://github.com/openstack/nova/blob/master/nova/tests/functional/test_server_group.py#L367 that would disable the anti-affinity filter and show that the the move would fail ugly
15:14:54 bauwser mriedem: johnthetubaguy: so that I could remove that specific test in the cleanup patch I'm doing
15:14:56 bauwser wfy ?
15:15:53 johnthetubaguy just want something to prove the migrate works, and picks the correct host, or fails correctly, with and without your patch
15:16:15 johnthetubaguy maybe we have it already, thats cool
15:16:37 johnthetubaguy I guess we should have a test that fails with your bug, and we see it being fixed, ideally
15:16:42 bauwser johnthetubaguy: that's covering the migrate action, not the live-migrate action
15:16:55 johnthetubaguy bauwser: so we should have coverage for both right?
15:17:06 bauwser johnthetubaguy: yeah
15:17:13 bauwser okay, lemme see what I can do
15:25:50 mriedem hrm, what sets image size when we create a snapshot? does glance do it?
15:26:00 mriedem i can't see during a snapshot flow where we actually pass the size value to glacne
15:26:01 mriedem *glance
15:27:43 mriedem ah it is glance https://developer.openstack.org/api-ref/image/v2/index.html?expanded=create-an-image-detail#images
15:28:01 andreykurilin mriedem: sorry, it looks like my IRC bouncer had crashed and stopped sending me any messages, so I missed your messages yesterday.
15:28:27 mriedem andreykurilin: that's ok, we just disabled the trigger crash dump functional tests in novaclient,
15:28:33 mriedem because they were timing out and causing the jobs to hang
15:28:50 mriedem andreykurilin: i feel like you had found or fixed something in those tests like that not too long ago, but can't recall
15:29:56 andreykurilin mriedem: I remember that we had similar problems before, but I can recall what was done to fix it
15:33:45 openstackgerrit Anthony PERARD proposed openstack/nova master: Add exclusion list for tempest for a libvirt+xen job https://review.openstack.org/449687
15:38:19 mriedem anthonyper: some of that is old news
15:38:22 mriedem anthonyper: see d571e03b5b67a401e7774988a84df3bef16273ad
15:38:28 mriedem https://github.com/openstack/tempest/commit/d571e03b5b67a401e7774988a84df3bef16273ad
15:38:38 mriedem tempest doesn't hardcode vda anymore in test_volume_boot_pattern
15:39:31 anthonyper mriedem: Thanks, I'll tests that and change the patch.
15:40:31 openstackgerrit Sylvain Bauza proposed openstack/nova master: Fix live-migrate usage of RequestSpec https://review.openstack.org/449690
15:41:35 openstackgerrit Anthony PERARD proposed openstack/nova master: Add exclusion list for tempest for a libvirt+xen job https://review.openstack.org/449687
15:43:18 mriedem johnthetubaguy: just fyi i added L137 in here https://etherpad.openstack.org/p/BOS-TC-vm-baremetal-platform
15:43:38 johnthetubaguy mriedem: ah, cool
15:52:50 cdent alex_xu: I assume you're not around anymore today, but wanted to check before I do some rebasing of our combined stacks of code
15:54:57 mriedem johnthetubaguy: leakypipes: http://forumtopics.openstack.org/cfp/details/28
15:55:04 mriedem that might need to be two separate sessions
15:55:10 mriedem jgriffith: ildikov: Kevin_Zheng: ^
15:56:26 sdague johnthetubaguy: ok, I looked again at the new spec, I still thing you want to take it back even one step further. Let me throw a patch on top of it with an idea of tone.
15:56:55 ildikov mriedem: thanks
15:57:14 mriedem johnthetubaguy: sdague: since you guys are going to be decided forum sessions, let me know if i should split http://forumtopics.openstack.org/cfp/details/28 into 2 sessions
15:57:19 mriedem *deciding

Earlier   Later