Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-14
20:45:28 dansmith the story behind why we have enums?
20:45:39 mgoddard melwitt: I'll push a patch for armv6l, but maybe an item for the nova team TODO list to make it a bit more resilient to the unknown?
20:46:04 melwitt dansmith: yeah, why it's a limited set that has to be added to when new arch arrive
20:46:15 dansmith melwitt: kindof the definition of an enum right?
20:46:32 melwitt dansmith: well, ok, then yes, why we have enum. you know what I mean
20:46:38 dansmith we have enums for known image properties so one we don't support doesn't sneak into place, and we have enums for virt drivers for ...
20:47:18 dansmith I mean, everything on the wire could just be strings and dicts, but, the whole point of this stuff was to nail it down so we could tell when something changed
20:47:37 melwitt mgoddard: in what way, a better error maybe? like ArchitectureUnknown?
20:47:57 mgoddard the problem here is that a new arch in libvirt capabilities causes nova-compute to fail
20:48:22 mgoddard it doesn't have to support the arch, but it shouldn't break everything else
20:49:15 dansmith sure, so the driver maybe should catch the failure to set the enum, log an error and exit on startup
20:50:21 mgoddard exit?
20:50:30 melwitt nova meeting in 10 min
20:51:05 mgoddard I don't think that libvirt adding an obscure new arch should break nova
20:51:24 dansmith mgoddard: I haven't clicked on your link so I don't know what's happening specifically
20:51:46 dansmith mgoddard: obviously if it's just a new supported emulated arch, then the driver just needs to not fail when it can't set that on an object, for sure
20:51:54 mgoddard dansmith: +1
20:51:58 melwitt ok, looks like update_available_resource is raising because of the unknown arch
20:52:08 dansmith mgoddard: if it's something more fundamental like it's the native system arch or something, it would make sense for it to not start if it isn't going to function
20:52:56 melwitt I know we've talked before about whether we should except-continue for things that raise during update_available_resource. maybe this would be an acceptable one. for anything else, we let it halt
20:53:19 openstackgerrit Eric Fried proposed openstack/nova master: WIP/PoC: Bypass ironicclient for node.get https://review.openstack.org/642899
20:53:28 dansmith it of course depends on the context of the issue
20:53:38 dansmith if it's unable to do something fundamental, it should abort
20:54:05 dansmith if it just can't expose support for some thing because of a new libvirt (which is sounds like in this case) then of course it should just not break on forward compat
20:54:07 melwitt yeah, I mean in update_available_resource, if InvalidArchitectureName is raised
20:55:02 dansmith it's some virt specific place it's crashing I imagine, so it would be in there, not directly in update_available_resource()
20:55:17 dansmith you surely don't want the resource tracker ignoring something like ArchNotSupported
20:55:32 dansmith but the libvirt driver can know if it's okay to skip that
20:55:45 melwitt oh
20:56:00 dansmith _get_instance_capabilities()
20:56:11 dansmith just needs to catch and ignore that
20:56:13 openstackgerrit Mark Goddard proposed openstack/nova master: Add armv6l to Architecture enum https://review.openstack.org/643455
20:56:15 mriedem this is why it breaks https://review.openstack.org/#/c/356067/
20:56:45 mriedem i mean, there is no major reason for that change except "use the enums we have"
20:56:50 mriedem w/o considering side effects
20:57:39 mgoddard I'll leave this one in your capable hands - it's long past home time here. Thanks for your input
20:57:45 dansmith mriedem: one reason (even if presumptive) so that each driver doesn't call the arches something slightly different causing the api to return inconsistent results
20:57:46 mriedem it would blow up before that as well https://review.openstack.org/#/c/356067/12/nova/compute/arch.py
20:57:57 mriedem so this is super latent behavior
20:58:26 mriedem dansmith: like the 50 different ways to describe power
20:58:32 dansmith point being, the bug (aside from the missing arch) is just in the libvirt driver being stupid
20:59:39 dansmith we need a catch and ignore right here, is all: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L5942-L5945
20:59:42 melwitt mgoddard: ok, thanks for letting us know. I can open a bug for that and propose a patch, if no one else wants to
21:00:23 dansmith melwitt: I'm already rage typing it
21:00:34 mriedem i would not be surprised if that value is used elsewhere though
21:00:39 melwitt dansmith: cool
21:00:39 mriedem and could blow up elsewhere
21:01:08 dansmith mriedem: could be, but it's clearly wrong for that loop to not continue
21:01:32 mriedem yeah sure
21:01:39 mriedem just exclude that one with a warning i guess
21:02:12 dansmith I don't thin it should even be that, otherwise we'll log that over and over for every instance and every run of the periodic
21:02:28 mriedem oh right this is the periodic
21:02:44 mriedem well you could log once and set a flag in the driver instance, or are you just going to exit?
21:03:16 dansmith no, not exit that makes no sense, just continue the loop
21:03:26 dansmith nova should expose whatever arches libvirt reports that it supports, that's really all
21:03:35 dansmith this code is intending to do that, assuming it knows about everything
21:11:38 openstackgerrit Dan Smith proposed openstack/nova master: Avoid crashing while getting libvirt capabilities with unknown arch names https://review.openstack.org/643458
21:14:42 openstackgerrit Dan Smith proposed openstack/nova master: Avoid crashing while getting libvirt capabilities with unknown arch names https://review.openstack.org/643458
21:26:53 mriedem jroll: TheJulia: fyi https://bugs.launchpad.net/nova/+bug/1815799
21:26:53 openstack Launchpad bug 1815799 in OpenStack Compute (nova) "Attempting to delete a baremetal server places server into ERROR state" [Low,Confirmed]
21:27:03 mriedem maybe a simple retry loop in the ironic driver on node destroy would solve that
21:28:03 TheJulia mriedem_away: we should be able to just pass on it if if we've already started the process of deleting the node in ironic
21:36:45 openstackgerrit Takashi NATSUME proposed openstack/nova master: Remove mox in virt/test_block_device.py https://review.openstack.org/566153
21:37:29 openstackgerrit Takashi NATSUME proposed openstack/nova master: Fix cleaning up console tokens https://review.openstack.org/637716
21:44:22 openstackgerrit Marcin Juszkiewicz proposed openstack/nova master: virt/arch: use 'armv6l' for ARMV6 architecture https://review.openstack.org/643467
21:46:33 hrw that ^^ change may fix Kolla CI error
21:57:17 melwitt dansmith ^
22:02:54 hrw and sorry for writing 3rd patch to what looks the same
22:07:12 hrw dansmith: after reading https://review.openstack.org/#/c/643458 and https://review.openstack.org/#/c/643455 I think that 643355 is wrong, your patch is good (left comment) and mine https://review.openstack.org/643467 just fixes to comply with capabilities contents
22:07:30 hrw but that's me - random guy from the internet D:
22:13:24 openstackgerrit Merged openstack/nova master: Customize irrelevant-files for nova-live-migration job https://review.openstack.org/643346
22:13:43 openstackgerrit Merged openstack/nova master: Re-enable Ceph in live migration testing https://review.openstack.org/643150
22:21:35 openstackgerrit Eric Fried proposed openstack/nova master: WIP/PoC: Bypass ironicclient for node.get https://review.openstack.org/642899
22:26:57 dansmith melwitt: what are you pointing me to?
22:27:25 melwitt dansmith: hrw's patch https://review.openstack.org/643467
22:28:04 melwitt it's related to the unknown arch blowup, sort of
22:28:28 dansmith yeah, I'm not sure which is the official arch name and/or if they're both legit as I'm not an arm specialist
22:28:52 dansmith but my patch is still something we should do as (as hrw said)
22:29:12 hrw dansmith: armv6 armv6l depends on kernel.
22:29:13 melwitt hrw: if you think https://review.openstack.org/643455 is wrong, it would be helpful to put your comment on it about why. I'm curious anyway
22:29:23 dansmith hrw: my concern would be it might also depend on libvirt behavior
22:29:40 melwitt dansmith: yeah, not saying your patch isn't something we should do. just that along that line, if you knew anything about changing that enum, hrw's patch was there
22:29:52 dansmith hrw: because if the l just means long or something, then I would expect both or either could be emulatable
22:30:51 dansmith melwitt: hopefully it's obvious that hrw's patch to change the value of the enum is not okay :)
22:31:29 melwitt dansmith: gah, I thought maybe you could comment on the review
22:31:40 melwitt nevermind me
22:31:55 hrw dansmith: libvirt mentions only armv6l and armv7l.
22:31:59 dansmith melwitt: I just did
22:32:05 melwitt thanks
22:32:14 dansmith hrw: on your system you mean right?
22:32:17 hrw https://gitlab.com/search?utf8=%E2%9C%93&snippets=&scope=&search=VIR_ARCH_ARMV6L&project_id=192693#L30
22:32:23 hrw in libvirt code
22:33:29 dansmith hrw: okay, like I said, I'm not really able to say whether armv6 or armv6l (or both) are right
22:33:49 dansmith but we can't just make the change you've proposed because it has effects to versioned objects and will break communication between services
22:33:55 hrw ok.
22:34:03 hrw perfectly fine for me.
22:34:30 dansmith hrw: I will say, if you're right that it's always armv6l, then we definitely need a variant of your patch over mgoddard's
22:34:55 hrw dansmith: let me discuss it tomorrow with libvirt guys, ok?
22:34:58 hrw 23:34 here
22:35:13 dansmith sure

Earlier   Later