Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-06
21:55:11 efried sean-k-mooney: what pissed me off enough to do this was having to add new passthrough utils in fake_libvirt_utils at the same time as adding them to utils - see the predecessor patch.
21:55:22 efried this makes it so you don't have to do that.
21:55:27 efried which is a win.
21:55:42 sean-k-mooney why not mock them in the testfucntion where they are actuly needed
21:55:46 efried i.e. by default anything you write in utils is not mocked - but also won't break the world if you try to use it.
21:55:59 sean-k-mooney e.g. instaad of a blanket mock
21:56:04 efried Yeah, sean-k-mooney agree with that. Not having looked, that sounds like a huge effort.
21:56:10 efried I assume the test surface of libvirt is enormous
21:56:24 efried though from the little I've seen, we're nowhere near majority coverage.
21:56:33 efried I guess we have a 'cover' job to tell us that, huh
21:56:43 sean-k-mooney efried: you should assume that but it is not nessicaialy true.
21:57:11 efried in any case, yes, stage 2-N might be to remove the mocks one at a time, see which tests break, and explicitly mock the removed thingy in those tests.
21:57:13 sean-k-mooney well i guess we could do that first then localise the mocs
21:57:21 efried just so.
21:58:19 mriedem cfriesen: wrote this up based on what i pinged you about last night https://bugs.launchpad.net/nova/+bug/1818914
21:58:20 openstack Launchpad bug 1818914 in OpenStack Compute (nova) "Hypervisor resource usage on source still shows old flavor usage after resize confirm until update_available_resource periodic runs" [Low,Triaged]
21:58:42 melwitt dansmith: oh, gotcha. consider it part of a "not completed" migration and operator must clean up to finish migration. yep, thank you
21:59:21 sean-k-mooney efried: also fun fact https://github.com/openstack/nova/search?q=assert_called_once&unscoped_q=assert_called_once
21:59:28 melwitt dansmith, mriedem: something that sucks that I realize now is that if we don't land the counting part of the series, we need to restore the policy change behavior thing to account for what to do with server creates in the face of down cells...
21:59:33 dansmith melwitt: yeah, you could also make a nova-status check for those I guess, and/or just do a warn-once on the migrate to say "yo dawg, this might never end if you don't do something"
21:59:38 sean-k-mooney i leaned yesterday that assert_called_once was added in py36
21:59:44 cfriesen mriedem: oh yeah, meant to bring that up.
22:00:00 mriedem melwitt: i don't know that we *need* to
22:00:10 mriedem it's a latent issue since multi-cell support in pike right?
22:00:12 dansmith melwitt: we already changed that? but it's no different than if we land none of this right?
22:00:16 mriedem and counting quotas since pike
22:00:18 efried sean-k-mooney: Yes, in base python, BUT we use mock-the-lib, which had it added/backported to much earlier.
22:00:22 melwitt dansmith: ++ appreciate the ideas, as my brain is about fried
22:00:24 dansmith right, nothing has changed since _that_ point right?
22:00:40 melwitt mriedem: ok. I wasn't sure if it's need or not. ok
22:00:41 efried sean-k-mooney: I remember learning that a couple years ago when I freaked out about same.
22:00:42 sean-k-mooney efried: oh ok
22:01:24 sean-k-mooney efried: ya so i freaked out a bit yesterday when i found that neutron have a hacking check that prevent you from using ig
22:01:32 sean-k-mooney *it
22:01:36 melwitt dansmith: yeah... yeah, that's true, current behavior is to ignore cell timeouts and such
22:01:45 melwitt so it's no worse than today
22:01:56 dansmith yeah, I thought you were saying we had landed something very out of order
22:02:00 dansmith from where we are today,
22:02:08 dansmith landing the data bit won't change anything I don't think
22:02:13 melwitt no, just... I don't know, I was thinking did down cell support change something about that. but I don't think it did
22:02:23 dansmith I surely hope not
22:03:01 melwitt haha. just paranoid about things changing things
22:03:27 sean-k-mooney change is scary
22:04:27 melwitt it is :) especially in nova where you can change something and some weird side effect pops up somewhere else because of it
22:05:22 cfriesen mriedem: might want to check whether the last paragraph in https://bugs.launchpad.net/nova/+bug/1498126 is still applicable
22:05:23 openstack Launchpad bug 1498126 in OpenStack Compute (nova) "Inconsistencies with resource tracking in the case of resize operation." [Medium,Confirmed]
22:06:00 cfriesen mriedem: that bug is old enough that the code may be fixed by now
22:06:43 dansmith melwitt: sean-k-mooney: that's whyfor we have careful review and don't just slam everything in, IMHO
22:07:55 sean-k-mooney change is scary? or so that we can all check our understading of the change and its wider effect so that changin a thing does not change other tings unless we ment too
22:10:04 dansmith sean-k-mooney: no, the fear of weird side effects part.. that's why review from a heterogenous pool of experienced reviewers is important
22:10:14 mriedem cfriesen: you mean this? "drop_move_claim looks up the new_flavor to compare it with the flavor that was used to track the migration, but on the source node it's certain to be the old_flavor. Thus as it stands now drop_move_claim (only ran on source nodes) doesn't do anything. Not a big deal, but we should probably fix it."
22:10:25 cfriesen mriedem: yeah
22:10:47 cfriesen I haven't checked whether it still applies
22:10:50 mriedem i don't think that applies anymore, i'm not really sure what it's talking about
22:11:13 mriedem when confirm_resize calls drop_move_claim, it's from the source and dropping usage for the old_flavor
22:11:18 sean-k-mooney dansmith: yes because we all have different experice and approce solutions in different ways so we tend to see different things.
22:11:51 mriedem when revert_resize (on the dest) calls rt.drop_move_claim, it's dropping the new_flavor usage from the dest host
22:13:20 mriedem anyway, the bug i reported is something i hit in my cross-cell resize functional tests for confirm resize and should be really easy to recreate in general
22:13:39 mriedem auto-confirm the resize and set the update_available_resource interval to a high number, like 10 minutes
22:16:31 cfriesen mriedem: I think you're right and it doesn't apply anymore.
22:17:20 sean-k-mooney dansmith: by the way the horrible code in https://review.openstack.org/#/c/624592/9/nova/network/linux_net.py
22:17:36 sean-k-mooney we have the same code more or less in os-vif https://github.com/openstack/os-vif/blob/master/vif_plug_linux_bridge/linux_net.py#L115-L153
22:20:42 sean-k-mooney we also have iptables code we inheritied form nova networks in the linux bridge plugin.
22:23:00 sean-k-mooney i wouldnt be too surpisidn if we could replace alot of this code wide a call to plug on the linux bridge driver
22:23:16 sean-k-mooney *surprised
22:27:16 mikal efried: I effectively did this sort of thing with the nova-net privsep thing. I'd much rather end up with mock decorators on the methods which actually need them. Otherwise its super hard to untangle what calls what when something fails. I can take a look at this later, but only if people say one nice thing about me each day. Today's topic can be how pleasing my hairstyling is. Go.
22:28:18 efried I saw a shampoo commercial the other day and thought of you.
22:29:42 efried mikal: Did you see conversation above where I said: this is stage 1, so we don't have to have "passthrough mocks" in the fake module. Stage 2-N might be to remove the remaining mocks one at a time, see which tests break, and explicitly mock the removed thingy in those tests.
22:29:58 mikal No not really. I was trying to remember how jeans work.
22:30:17 mikal I didn't read all the scrollback, it looked long and boring.
22:31:45 efried it was
22:31:51 sean-k-mooney mikal: fyi dansmith reorderd some of you privsep removal code so the 4 patchs with +w can merge without being blocked by https://review.openstack.org/#/c/624592
22:32:01 efried mikal: that's why I extracted the salient bit of it for you. So you wouldn't have to mess up your hair.
22:32:05 efried or... something.
22:32:30 mikal sean-k-mooney: careful. Some of us said regrettable things when we noticed that but hadn't yet had our valium for the day.
22:38:00 sean-k-mooney mikal: i assumed you would be happy that 4 more of them are currently makeing there way through the gate
22:38:34 sean-k-mooney mikal: but on a releated note we have similar code in os-vif which i kindof want to kill at some point once i figure out why we do it
22:38:35 sean-k-mooney https://github.com/openstack/os-vif/blob/master/vif_plug_linux_bridge/linux_net.py#L115-L153
22:49:55 efried mriedem: Are we waiting for train for https://review.openstack.org/#/c/633855/ series? (SEV stuff)
22:51:55 mriedem efried: yes
22:52:34 efried mriedem: procedural hold so I don't keep looking at it and trying to decide whether to approve it?
22:53:41 mikal Oh wow, this is terrible. Did you know we sometimes mock away the fake for that fake_libvirt_utils thing? Why? Who did this?!?
22:54:28 mriedem efried: i put a -W on the 2nd patch in the series which adds the capability which will now auto-generate the trait on the compute node resource provider,
22:54:41 mriedem i avoided the -2 to not blow up jay's beautiful stats
22:54:51 efried mriedem: ack
22:54:53 sean-k-mooney mikal: do you know that we somtimes import the fake_libvirt direver for the unit tests in too fucntional tests
22:54:59 mriedem efried: as for the one below that, i don't have an opinion
22:55:07 efried mikal: You mean mock.patch(restore original method)?
22:55:20 efried that's pretty fantastic
22:55:32 mikal efried: no, its even more useless. We do a mock.patch to a noop for a function THAT IS ALREADY A NOOP.
22:55:39 efried wow
22:56:17 sean-k-mooney mikal: well actully that is proably new test that were written without knowing that ere was a global mock
22:56:45 sean-k-mooney so those test are actully proably mocking just the things they need to mock
22:56:49 sean-k-mooney which is good
22:57:24 sean-k-mooney the fact it was already mocked is wasteful but it does not hurt anything
22:58:14 sean-k-mooney by the way is there a reason the fake libvirt dirver is not with the other virt dirvers like the the Fake virt diriver is?
22:59:10 mikal sean-k-mooney: please don't ask logical questions, it just makes use feel even more inadequate
23:08:22 mikal Herm. Turns out we apply the fake_libvirt_utils fixture in many places it isn't even used.
23:08:33 mikal This rathole is nice. I like it. It feels like home.
23:23:36 mikal Wait what? nova.virt.libvirt.utils.file_open is definitely my favourite part of all this.

Earlier   Later