Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-06
21:33:14 cfriesen :)
21:34:11 cfriesen I'm easy
21:34:11 mikal cfriesen: https://www.madebymikal.com/adding-oslo-privsep-to-a-new-project-a-worked-example/ is an overly complete answer with way more detail than you care about.
21:34:38 mikal cfriesen: but the short answer is its pretty trivial. Decide where in nova/privsep to put the method. Add the method with the decorator. Call the method from other places. Profit.
21:35:13 mikal cfriesen: the only real warts are around style (don't pass in command lines, pass in args that are used to generate a command line); and unit testing hoops if efried notices that test coverage has dropped.
21:35:30 cfriesen sweet, thanks.
21:35:45 mikal cfriesen: I'd be happy to do a worked example in a blog post, but you're the first person to ever ask because no one loves me and my life is meaningless.
21:36:20 cfriesen (I'm trying to resolve a problem with emulated TPM and cold migration...the software TPM daemon runs as root but nova doesn't, and I need to copy the root-owned file to the dest node.)
21:36:27 mikal cfriesen: although when I think of it, https://review.openstack.org/#/c/624593/ is probably close enough to documentation for now. Copy that.
21:36:54 mikal Oh, so you just want to read a file with escalated permissions?
21:36:58 mikal That's already implemented IIRC.
21:37:36 mikal cfriesen: http://git.openstack.org/cgit/openstack/nova/tree/nova/privsep/path.py#n27 is what you want to read a file as root.
21:37:48 cfriesen I suspect I need to modify SshDriver.copy_file() and RsyncDriver.copy_file() to optionally run with root privileges.
21:38:00 mikal cfriesen: with maybe a http://git.openstack.org/cgit/openstack/nova/tree/nova/privsep/path.py#n73 thrown in for the destination file.
21:39:03 mikal Ummmm. Why not just change the permissions on the file, sync, then change the permissions at the destination?
21:39:13 mikal Instead of running the sync as root?
21:40:17 cfriesen might work. It's tricky because root might not even be set up for passwordless ssh, and the resize code path doesn't do RPCs to set things up the way live migration does.
21:40:35 cfriesen still studying the flow to figure out how to make it work
21:41:47 mikal Ok, well I'm going to put on pants and stuff, but ping me if you need more random pointers.
21:50:21 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Make fake_libvirt_utils DIAF https://review.openstack.org/641489
21:50:31 efried mikal, mriedem: let's see how that goes ^
21:50:45 efried it's stage 1 of ? btw
21:51:10 efried Probably better to explicitly mock only what's necessary to mock in only the places it's necessary.
21:51:17 efried But I probably *am* too lazy for that.
21:53:04 dansmith melwitt: I think we should include it in found, not in done, so it's clear that it's not migrated.. a sentinel doesn't make sense, IMHO, and it really should be cleaned up via archive/purge I think
21:53:20 dansmith melwitt: this is exactly why I wanted a test for it, so we make some decision and commit to it :)
21:53:47 sean-k-mooney so instead of mocking nova.virt.libvirt.driver.libvirt_utils you are now mocking a buch of functions in that module
21:53:54 sean-k-mooney efried: ^
21:54:22 efried sean-k-mooney: right, instead of mocking a whole module with a whole module, I'm mocking the individual methods that are actually mocked.
21:54:31 sean-k-mooney most of which were just pass....
21:54:40 efried which is fine
21:54:53 sean-k-mooney ya
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.

Earlier   Later