Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-07-03
16:31:47 sfinucan mdbooth: IMA I'm pretty confused by https://review.openstack.org/#/c/479802/
16:32:48 mdbooth sfinucan: What specifically?
16:33:10 sfinucan I left comments in the review, but tl;dr why's the scope change necessary?
16:33:36 sfinucan The only functional change I can see is that you removed the status check
16:34:05 mdbooth sfinucan: Actually in that one specifically the benefit is marginal
16:35:02 mdbooth sfinucan: The change is that everything after the instance is 'done' is outside the scope of the context manager
16:35:14 mdbooth Although you could argue that quotas should be in...
16:35:25 mdbooth Yeah, that one could probably reasonably go
16:36:00 mriedem mgagne: let me know if you have any questions about the instance action thing i'm talking about in https://review.openstack.org/#/c/454322/
16:36:02 mdbooth However, resize_instance is where it's at
16:36:39 mdbooth sfinucan: The issue in resize_instance is that you've cast the control flow to another host, but after that's happened you've still got this rogue context manager which might yet futz with your state
16:36:55 mdbooth There are no circumstances where you want that to happen
16:37:25 sfinucan mdbooth: wait, are you still referring to that patch or the next one in the series?
16:37:34 mdbooth sfinucan: The only you just posted
16:37:37 mdbooth errors_out_migration
16:38:04 mdbooth And that's directly relevant to the following patch, and why it's a dep
16:38:21 sfinucan ah, gotcha
16:38:26 sfinucan so 'finish_revert_resize' is the marginal change
16:38:37 sfinucan but 'resize_instance' is not
16:38:45 mdbooth Yeah, I could potentially ditch that bit of the diff
16:39:03 sfinucan yeah, you might drop that or at least split it. Just lets me reason about it a little easier
16:39:50 mdbooth The important one is resize_instance, because it does a cast
16:40:04 mdbooth So allowing the context to continue is a race
16:40:51 sfinucan mdbooth: You're referring to the call to finish_resize, yes?
16:40:58 mdbooth sfinucan: Yes
16:41:31 sfinucan OK, that clarifies things. Thanks for the explanation
16:41:52 sfinucan If you can split those out/drop the less necessary diffs, I'm good to sign off on that
16:43:43 mdbooth sfinucan: Thanks for looking at that.
16:43:49 sfinucan no problem at al
16:43:53 sfinucan *all
16:43:56 sfinucan *atal atal
16:44:17 mdbooth sfinucan: I've just quickly replied to 1 comment, btw. The scope change is actually related, even if it's not immediately obvious why.
16:44:55 openstackgerrit Merged openstack/nova master: pci: add uuid field to PciDevice object https://review.openstack.org/469147
17:10:02 claudiub mriedem: hello. i have a question. so there is a patch that basically sanitizes the instance object in the InstanceMetadata ( https://review.openstack.org/#/c/478991/3/nova/api/metadata/base.py ), which will get rid of the instance's context, which will make unset attributes no longer loadable. Do we have an alternative solution to this, rather than touching the lazy-loading attributes?
17:10:19 claudiub IMO, that can be very error-prone.
17:15:43 mriedem claudiub: i wasn't aware of that change, but i remember that we hit some issues with metadata api with multiple cells so fail to lazy-load correctly anyway; the commit message says this isn't the long-term strategic solution,
17:16:01 mriedem so i think long-term we want to whitelist what we store and cache
17:16:32 mriedem because we've had other bugs where we can't cache the instance object using things like memcache because it uses pickle and things in there don't pickle correctly, like the sqlalchemy db connection stored in the context, as noted in the commit message
17:16:50 openstack Launchpad bug 1694666 in OpenStack Compute (nova) "metadata service PicklingError: Can't pickle <class 'sqlalchemy.orm.session.Session'> when using memcached" [Medium,Fix released] - Assigned to Dan Smith (danms)
17:16:50 mriedem yeah that's what the bug was, i remember this bug https://bugs.launchpad.net/nova/+bug/1694666
17:17:15 claudiub well, can't the db connection be recreated after the pickle?
17:18:26 openstackgerrit Claudiu Belu proposed openstack/nova master: Hyper-V: fix live migration with CSVs https://review.openstack.org/380448
17:18:31 mriedem claudiub: maybe, probably depends on what that's needed for
17:18:52 mriedem claudiub: i'm assuming you guys have extra fields on the instance object for hyperv and you need to lazy-load stuff for the metadata api?
17:19:45 claudiub yeah, we need the device_metadata field when we create the instance config drive, but that's not really a niche field. it's a basic field needed for the config drive
17:20:49 mriedem hmm, have you talked to artom?
17:20:56 mriedem because i'd think libvirt needs the same thing
17:21:58 claudiub mriedem: so, the reason why it worked for libvirt is because libvirt always touches that instance.device_metadata field, while we only set it whenever we have device tags to set in it.
17:22:35 mriedem ah
17:22:40 mriedem claudiub: have you reported a bug?
17:22:41 claudiub while we can also always touch it, this isn't a driver specific issue, we shouldn't have to touch it
17:22:51 mriedem sure,
17:22:56 mriedem i think it's an oversight in dan's patch
17:22:58 mriedem i left a comment in there
17:23:21 claudiub well, i think we can simply reopen the same bug, and post a comment on launchpad about the new issue.
17:23:49 mriedem claudiub: it should be a new bug
17:23:56 mriedem otherwise LP tracking gets confusing
17:24:05 claudiub i see. ok then. i'll open up a new bug
17:24:57 mriedem it's also an unfortunate change in another way, in that obj_load_attr for these will pull the entire instance for each entry and join on another table, it would be nicer to do those all at once with expected_attrs,
17:25:02 mriedem but maybe don't mix those issues together
17:25:15 mriedem it could be optimized for sure though
17:25:50 claudiub yeah, that would be nicer.
17:27:29 claudiub mriedem: also, regarding hyper-v generation 2 vms CI testing, it is in the plan, but the main issue is that we have to test both generation 1 and generation 2 VMs, as they are both currently supported. While more and more OSes are supported on generation 2, it's still not all.
17:30:47 claudiub and the way the VMs are built is quite different too, especially when it comes to attaching volumes / disks, setting boot order, nic hot plugging, and other new features.
17:31:46 claudiub anyways. planning to have some coverage in the ci for gen 2 by the end of pike.
17:36:02 mriedem claudiub: if the majority of users are gen2, then i'd just run the ci on gen2
17:36:34 mriedem we could always put something in the docs that since pike the hyperv CI is running against gen2 and that's the recommended configuration
18:03:52 openstackgerrit Merged openstack/nova master: placement: adds ProviderTree for nested resources https://review.openstack.org/415920
18:15:16 openstackgerrit Merged openstack/nova master: placement: support GET /allocation_candidates https://review.openstack.org/475448
18:26:17 openstackgerrit Andrey Kurilin proposed openstack/python-novaclient master: DO NOT MERGE: debug failed test https://review.openstack.org/479818
18:45:31 openstackgerrit Merged openstack/nova master: libvirt: update logic to configure device for scsi controller https://review.openstack.org/459740
18:46:06 openstackgerrit Merged openstack/nova master: libvirt: update the logic to configure volume with scsi controller https://review.openstack.org/459741
18:47:34 mriedem jaypipes: i went through https://review.openstack.org/#/c/476631/
18:47:52 mriedem jaypipes: i think we're going to have to adjust that a bit to not call placement if using the caching scheduler
18:48:44 jaypipes mriedem: at some point we need to bite that bullet.
18:53:25 mriedem jaypipes: sure, but it's easy to not do it in this change
18:53:32 mriedem put a flag on the scheduler driver that the host manager can check
18:53:46 mriedem *scheduler manager
18:54:17 jaypipes mriedem: back shortly. need to go pick the pugs up from the groomers.
18:54:28 mriedem oolala
19:32:27 jaypipes mriedem: nothin' better than the smell of a sweet-smellin' grumble of freshly-groomed pugs.
19:33:35 mriedem dry or wet?
19:34:57 jaypipes dry of course. :)
19:35:06 mriedem good
19:46:34 jangutter Due to some scary flu, external deadlines and various conjunctions I was out of OpenStack action for about 2 months...
19:46:53 jangutter And boy, did the nova and os-vif bits that I'm interested change!
19:47:54 jaypipes jangutter: glad you are feeling better!
19:47:57 jangutter My question is to do with the new OVS_ACCELERATION vif that moshele contributed... I'm not seeing where the PCI devices get allocated.
19:48:30 jangutter Lemme grab the patch set.
19:48:53 jaypipes jangutter: PCI devices do not get allocated by os-vif. They are allocated by the resource tracker when a launch instance request is received that includes a PCI device request.
19:49:09 jangutter Yep, that I understand.
19:49:19 jaypipes jangutter: the resource tracker has a PCI device manager inside it, and it's that that does the actual "allocation" of a PCI device to a guest
19:49:40 jangutter But does it just pick _any_ available PCI address?
19:50:01 jangutter ( https://review.openstack.org/#/q/topic:ovs_acc )
19:50:17 jaypipes jangutter: it picks any available PCI address that matches the PCI device request that is received from the conductor.
19:50:47 jaypipes jangutter: are you wondering who creates that PCI device request? :)
19:51:32 jangutter Yep, remember, I had the brain-dead idea of putting it into neutron-api bits.
19:51:57 jaypipes jangutter: well, there's still quite a bit of Neutron API that is used to construct the PCI device request.
19:52:08 jaypipes jangutter: and that existed before your idea ;)
19:53:53 jaypipes jangutter: what moshele's patch did on the os-vif side was simply create a specialized VIF type class for the "new" OVS_ACCELERATED vif type. That vif type is something that still needs to be created in Neutron and supplied to the nova boot request by passing in a port_id value that corresponds to the pre-created Neutron port that has the port binding information indicating it's an "OVS_ACCELERATED" port.

Earlier   Later