| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-08-05 | |||
| 13:46:09 | sean-k-mooney | alex_xu: by the way i reworked https://review.opendev.org/#/c/739131/ after your comments in version 5, im hoping stephenfin will get back to it to be the second +2 later today but just an fyi incase you want to look at it before then. | |
| 13:46:49 | sean-k-mooney | efried: this is basicaly why we are providing the provider.yaml i.e. to enable a supported way to do this so ya. | |
| 13:47:19 | sean-k-mooney | efried: hows openshift land going? | |
| 13:49:08 | efried | correct | |
| 13:49:33 | efried | Things are going well. After a couple months of pretty serious culture shock and vertical learning curve, I'm getting my feet under me. | |
| 13:50:44 | efried | I've written an operator (in go) and feel pretty comfortable navigating openshift/kube APIs. | |
| 13:51:44 | sean-k-mooney | cool, i have read some go but havent really written any, never had the need. | |
| 13:52:16 | sean-k-mooney | i am vaguly aware of what operators do but never looked under the cover to see how they are implemented | |
| 13:52:55 | efried | With existing tools (operator-sdk) and any kind of sweng background, they're ridiculously easy to write. | |
| 13:52:57 | sean-k-mooney | i belive there are some frameworks to implement them without go too using declaritive yaml files referencing CRDs too right | |
| 13:53:32 | sean-k-mooney | i know you can go the custom contoler route too in go | |
| 13:53:50 | sean-k-mooney | but again more or less have just read the docs and then done nothing with that info | |
| 13:54:00 | sean-k-mooney | /read/skimmed/ | |
| 13:54:38 | efried | And yes, an operator is effectively a custom controller that follows some rules. | |
| 13:54:38 | efried | The CRDs are how the operators extend the kube API. Instances of those (CRs) are how you trigger the operator to do its thing. I'm not aware of a way to "declare" an operator with just yaml. | |
| 13:56:08 | sean-k-mooney | efried: are you invovled with the current experiments to deploy openstack using operators | |
| 13:56:28 | efried | I don't get anywhere near openstack anymore. | |
| 13:56:55 | sean-k-mooney | there is an experiment to run openstack on openshift the repo is public somehere | |
| 14:00:45 | sean-k-mooney | efried: https://github.com/openstack-k8s-operators | |
| 14:01:14 | efried | cool | |
| 14:01:15 | sean-k-mooney | efried: mdbooth and other have been working on that for a while | |
| 14:03:02 | sean-k-mooney | efried: also this is what i was thining of https://kudo.dev/ | |
| 14:05:46 | sean-k-mooney | efried: i was considring writing an operator for deploying zuul at one point and i was looking at kudo amoung other things as a way to do it | |
| 14:06:48 | sean-k-mooney | well zuul, nodepoll , zookeyper, german and gerrit | |
| 14:07:08 | sean-k-mooney | but i just wrote hte manifest by had instead. | |
| 14:16:06 | openstackgerrit | sean mooney proposed openstack/os-vif master: support pyroute2 0.5.13 https://review.opendev.org/744809 | |
| 14:22:45 | openstackgerrit | Merged openstack/nova master: Document nova in tree virt drivers https://review.opendev.org/740061 | |
| 15:17:04 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP zuul: nova-multinode-evacuate https://review.opendev.org/744883 | |
| 15:53:02 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Move equality check into LibvirtConfigGuestInterface https://review.opendev.org/744524 | |
| 15:53:03 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Remove unused vpn param from allocate_for_instance https://review.opendev.org/744933 | |
| 15:54:15 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Only unplug vif after the device is detached from libvirt https://review.opendev.org/744934 | |
| 15:54:15 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: [WIP] Support SRIOV interface attach and detach https://review.opendev.org/740995 | |
| 15:57:53 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM: add logging for device matching https://review.opendev.org/744936 | |
| 16:04:21 | openstackgerrit | Harshavardhan Metla proposed openstack/nova stable/rocky: [stable-only] Moved the quoted section https://review.opendev.org/744681 | |
| 16:05:50 | melwitt | lyarwood: I just barely started looking at ceph job failures on stable/queens, saw this error message "libvirtError: internal error: unable to execute QEMU command 'device_add': Property 'virtio-blk-device.drive' can't find value 'drive-virtio-disk1'" https://zuul.opendev.org/t/openstack/build/fa85e5eeb1f84389b5dd259c8a829552/log/controller/logs/screen-n-cpu.txt#32824 | |
| 16:06:03 | melwitt | does that ring any bells to you? | |
| 16:09:01 | melwitt | hm nvm maybe https://ask.openstack.org/en/question/95328/ceph-cinder-attach-volume-to-running-instance | |
| 16:21:01 | openstackgerrit | Harshavardhan Metla proposed openstack/nova stable/rocky: "[stable-only]" Removed the quoted section https://review.opendev.org/744681 | |
| 16:26:46 | stephenfin | melwitt, bauzas: not sure if you'll know this off the top of your head, but should we be releasing resources before we confirm a resize? | |
| 16:27:49 | stephenfin | Based on https://review.opendev.org/#/c/641806/21, it seems we do (via the resource tracker's periodic task run) | |
| 16:28:01 | melwitt | stephenfin: releasing as in deleting allocations on the source before confirming? AFAIK no, supposed to hold allocations on the source and dest until confirm/revert | |
| 16:31:43 | melwitt | I think that patch's commit message is saying prior to that patch, we were reporting resources held on the source *after* a confirm resize (until the next periodic run) whereas after a confirm, the resources should be shown as released on the source. but only after the confirm, not before | |
| 16:32:33 | stephenfin | ah, that makes sense | |
| 16:32:41 | stephenfin | so now we're freeing those resources immediately | |
| 16:33:12 | melwitt | immediately upon a confirm, yeah | |
| 16:34:06 | melwitt | or rather reporting them immediately after a confirm. the claim has always been dropped at the time of confirm, but apparently the reporting was delayed potentially by one periodic task run | |
| 16:34:12 | openstack | Launchpad bug 1879878 in OpenStack Compute (nova) "VM become Error after confirming resize with Error info CPUUnpinningInvalid on source node " [Medium,Confirmed] - Assigned to Stephen Finucane (stephenfinucane) | |
| 16:34:12 | stephenfin | okay, cool. There's a bug in it that I'm working on (https://bugs.launchpad.net/nova/+bug/1879878, fwiw); tl;dr: if the periodic runs between the API request and the 'confirm_resize' call in the RT, the ComputeNode.numa_topology gets out of whack | |
| 16:34:43 | melwitt | ah, gotcha | |
| 16:36:21 | melwitt | oh interesting, so if the periodic task fires while confirm resize is still processing, it turns the instance to ERROR | |
| 16:37:00 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Move equality check into LibvirtConfigGuestInterface https://review.opendev.org/744524 | |
| 16:37:11 | stephenfin | yeah, the '_update_available_resource' function of the RT seems to regenerate ComputeNode objects from scratch, including the embedded numa_topology | |
| 16:37:47 | stephenfin | and it doesn't seem to be accounting from the still-present allocations on the host once the instance has been confirmed | |
| 16:38:12 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Only unplug vif after the device is detached from libvirt https://review.opendev.org/744934 | |
| 16:39:25 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: [WIP] Support SRIOV interface attach and detach https://review.opendev.org/740995 | |
| 16:39:26 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM: add logging for device matching https://review.opendev.org/744936 | |
| 16:42:01 | melwitt | stephenfin: note that sean-k-mooney's comments are correct though, that if this is in a version older than claims in placement, races are expected (especially since we don't have numa in placement). in your repro, is that with cpu_dedicated_set and on the master branch? | |
| 16:42:38 | gibi | sean-k-mooney: added a patch top of the sriov_attach series with extra logging that can help you figuring out why the macvtap and direct-physical devices are not detached for you | |
| 16:42:42 | gibi | sean-k-mooney: https://review.opendev.org/744936 | |
| 16:42:59 | gibi | also added a patch to move the vif unplug after libvirt detach | |
| 16:43:02 | openstackgerrit | Stephen Finucane proposed openstack/nova master: TODO https://review.opendev.org/744951 | |
| 16:43:02 | openstack | bug 1879878 in OpenStack Compute (nova) "VM become Error after confirming resize with Error info CPUUnpinningInvalid on source node " [Medium,Confirmed] https://launchpad.net/bugs/1879878 - Assigned to Stephen Finucane (stephenfinucane) | |
| 16:43:02 | openstackgerrit | Stephen Finucane proposed openstack/nova master: tests: Add reproducer for bug #1879878 https://review.opendev.org/744950 | |
| 16:43:05 | stephenfin | melwitt: ^ | |
| 16:43:15 | stephenfin | I can reproduce trivially with new-style configuration | |
| 16:43:35 | melwitt | while using cpu_dedicated_set? /me looks | |
| 16:43:38 | stephenfin | yup | |
| 16:43:52 | melwitt | ok I see | |
| 16:45:34 | stephenfin | I think I'm close to a fix at least. Worst case scenario, we put mriedem's stuff inside a conditional to only run if we still know that it exists | |
| 16:45:52 | stephenfin | i.e. if it's in the list of tracked migrations and instances | |
| 16:46:17 | stephenfin | though I haven't figured out what that will leak yet | |
| 16:46:22 | stephenfin | tbd | |
| 16:47:22 | melwitt | sorry, what's the proposed fix? being that it's intended that resources be held on source and dest until confirm or revert | |
| 16:48:13 | stephenfin | https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L579-L591 | |
| 16:48:29 | stephenfin | we only do that if the instance appears in our list of tracked_migrations or tracked_instances | |
| 16:50:44 | stephenfin | afaict, '_update_usage_from_instances' wipes those and regenerates them. When we confirm the resize, the instance is marked as deleted, which means it's not picked up by '_update_usage_from_instances' | |
| 16:51:22 | stephenfin | so the race is between that (the instance getting marked as deleted and '_update_usage_from_instances' running) and the call to 'drop_move_claim' | |
| 16:51:30 | stephenfin | I _think_ | |
| 16:58:56 | sean-k-mooney | gibi: thanks ill restart with that now and see how it works | |
| 17:00:22 | melwitt | stephenfin: this is interesting bc the commit message of mriedem's change says "This fixes the issue by also updating usage in drop_move_claim when the instance is not in tracked_migrations but is in tracked_instances." (which would do part of what you're suggesting) but it appears not to be what is in the code | |
| 17:00:29 | gibi | sean-k-mooney: thanks. I will end my day about now but I will be back tomorrow morning | |
| 17:00:34 | sean-k-mooney | stephenfin: sound plausible i had not got to the point of diging into how the race happens | |
| 17:00:54 | sean-k-mooney | gibi: no worries | |
| 17:03:52 | melwitt | stephenfin: oh, nvm, I'm misreading this... it's removing usage by removing the instance from tracked_instances and then updating usage | |
| 17:08:48 | sean-k-mooney | melwitt: stephenfin if this race is happening the way you say | |
| 17:09:00 | sean-k-mooney | would this also happen with the old config options? | |
| 17:09:07 | stephenfin | sean-k-mooney: I think so | |
| 17:09:18 | sean-k-mooney | ok its just more obvious now? | |
| 17:09:25 | stephenfin | I think the bug is with that patch from mriedem | |
| 17:09:28 | sean-k-mooney | or they were just unlucky | |
| 17:09:38 | stephenfin | I don't think it's anything to do with the PCPU work | |
| 17:09:43 | sean-k-mooney | ok | |
| 17:10:16 | stephenfin | tbc, it's still a working theory but it would be easy prove out by backporting that fix to e.g. stable/stein where none of that code is present | |
| 17:10:18 | sean-k-mooney | i think the upgrade procedure we proposed for our downstream customer still makes sense | |
| 17:10:33 | sean-k-mooney | but i think they will need the backport of your two fixes | |
| 17:10:36 | sean-k-mooney | the one for this race | |
| 17:10:49 | sean-k-mooney | and the one for the isolate on smt host with new config options | |
| 17:10:51 | sean-k-mooney | right? | |
| 17:11:56 | stephenfin | the isolate on SMT fix is mostly unrelated and should be backported regardless | |
| 17:12:01 | stephenfin | still unsure about this race | |