Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-11-06
15:06:19 bauzas if they are optional
15:06:29 bauzas (and only for a specific driver)
15:06:39 lyarwood bauzas: we lazy loaded before silently and it was a PITA to debug issues when they failed to import
15:06:46 lyarwood bauzas: assuming that's what you mean
15:07:07 lyarwood bauzas: could we not just add them as test deps?
15:07:16 bauzas lyarwood: then, lazy load them by creating the libvirt object
15:07:26 bauzas when initting
15:07:43 bauzas gibi: ^ to be honestly, I'm against https://review.opendev.org/#/c/746873/4/nova/storage/rbd_utils.py
15:08:07 bauzas because we create more problems for folks that don't use ceph than we fix the needed ones
15:09:07 openstackgerrit Merged openstack/nova master: zuul: Merge nova-evacuate into nova-multinode-live-migration https://review.opendev.org/755309
15:09:18 dansmith bauzas: me too, that seems crazy
15:10:36 dansmith surely we could check images_type and decide whether or not to explode there?
15:10:37 gibi bauzas, lyarwood, dansmith: can we move these import into one of the classes in the rbd_utils?
15:11:03 gibi dansmith: it is not exploding the driver, it is logging the import error
15:11:10 dansmith gibi: I think the point here was to make it explode before those get loaded, but that's not really okay, IMHO
15:11:13 bauzas gibi: the problem is that when using import directly, you *need* rdb
15:11:22 bauzas by any module
15:11:34 bauzas hence either lazy-loading it
15:11:55 bauzas or having other ways to support it like we did by telling none
15:12:06 dansmith yeah, go back to the old way and make something else check to see if the rbd import is None during early init, if we're in a dependent code path
15:12:16 bauzas by providing an exception you say "sorry, but you need it"
15:12:17 dansmith check config, or put it in a class we only load if we're using ceph
15:13:10 bauzas that too
15:13:19 bauzas there are multiple ways to make sure it's optional
15:16:29 openstack Launchpad bug 1903316 in OpenStack Compute (nova) "excessive logging of rbd and rados import errors" [Undecided,New]
15:16:29 gibi filed a bug https://bugs.launchpad.net/nova/+bug/1903316
15:19:43 gibi feel free to push a fix against it ^^
15:20:00 gibi if nobody then I might get to it at some point next week
15:22:04 bauzas gibi: I can try to fix it
15:22:37 bauzas but ideally, I'd love to understand the dynamics behind us logging this as an exception and what we wanted to fix previously
15:22:38 gibi ack, thanks
15:23:00 gibi I hope lyarwood can provide such context ^^
15:23:04 bauzas because I don't see a problem with lazy-loading at init time
15:23:38 bauzas we could even make nova-compute refuse to start if config option says "you should have it"
15:24:20 dansmith right
15:24:25 lyarwood bauzas: the context there is that we previously silently failed without logging any context about why the import failed.
15:24:30 dansmith or .. log the warning if configured and missing
15:24:54 lyarwood bauzas: we had a few downstream reports where this failed even with the modules in the container image etc.
15:25:25 lyarwood hmm I was sure there was a reason we couldn't check the config before doing this import
15:25:36 dansmith lyarwood: well before it was above the CONF,
15:25:55 dansmith lyarwood: but also, where we actually need it, check to see if the import is None, if so, try to import it again and log the exception there just to get the evidence
15:26:12 dansmith i.e. if rbd_utils.rbd is None: import rbd
15:27:04 lyarwood dansmith: yup understood
15:32:21 bauzas dansmith: lyarwood: honestly, I'm on the side we should fix the problem by making rdb and rados mandatory for the use of the right option, and make nova-compute hardstop if not
15:32:35 lyarwood bauzas: it already does
15:32:52 bauzas not in this module then, right?
15:33:06 lyarwood no, we just raise RuntimeError
15:33:17 bauzas when starting the compute service ?
15:33:41 lyarwood from this module that iirc stops the service eventually yes
15:33:44 lyarwood if rbd is used
15:34:26 bauzas the above try/catch is made at import time, not runtime
15:34:57 bauzas so unless we fail ungracefully there, we do check it at run time
15:35:06 lyarwood https://github.com/openstack/nova/blob/c0fe95fcc5aec99a83dd57093dc230ef67b36b39/nova/storage/rbd_utils.py#L139-L140
15:35:32 bauzas ack, run time
15:35:43 bauzas and when we create the instancez
15:35:56 bauzas which, I suspect, is made at init
15:36:34 bauzas lyarwood: okay, so indeed, we're all good, but why couldn't we just make the imports being made at run time just before this check ?
15:37:05 lyarwood I'm reworking this now
15:39:13 lyarwood the only issue now is that we need to ignore F401 as the imports aren't directly used in __init__
15:44:00 bauzas lyarwood: why ?
15:44:07 bauzas I'm confused
15:44:54 bauzas hah, so you have a rados object which isn't used
15:45:10 lyarwood no idea, must be the way the pep8 rule is written, rados is used within the class but it's also saying that the import isn't used
15:45:27 lyarwood both imports are listed as not used
15:45:40 lyarwood I've #noqa'd the lines anyway
15:45:50 bauzas lyarwood: use the importlib module
15:46:11 sean-k-mooney stephenfin: test
15:46:26 stephenfin HexChat didn't crash \o/
15:46:37 openstackgerrit Lee Yarwood proposed openstack/nova master: rbd: Only log import failures when the RbdDriver is used https://review.opendev.org/761762
15:46:39 sean-k-mooney you should still lookinto weechat
15:46:49 sean-k-mooney but glad its working
15:46:58 stephenfin meh, I live the GUI in a separate window
15:47:27 dansmith lyarwood: that's much better, IMHO
15:47:46 lyarwood yup not sure what I was thinking the first time round
15:47:53 sean-k-mooney stephenfin: by the way https://review.opendev.org/#/q/topic:bug/1895220+(status:open+OR+status:merged) are ready for you to review again
15:48:04 sean-k-mooney they are still running in ci however
15:48:10 dansmith lyarwood: now I have to figure out what drug I can take to let me approve that without a test
15:48:36 lyarwood dansmith: hmm I can work something out now that it's in the class
15:48:38 dansmith lyarwood: I'm thinking it'll be hard to tickle the error case with a mock, so I should probably just let it go...
15:48:53 bauzas wait, you were having global objects
15:48:57 lyarwood true it's awkward
15:49:04 dansmith we used to use importutils more for stuff like that so we can mock that instead of the import keyword
15:49:08 bauzas here, you are making them class attributes
15:49:14 dansmith bauzas: no
15:49:16 bauzas dansmith: right, i just said it
15:49:40 lyarwood wait, the imports aren't global?
15:49:50 dansmith they are
15:49:55 openstackgerrit Artom Lifshitz proposed openstack/nova stable/train: Test for disabling greendns https://review.opendev.org/761763
15:49:59 bauzas lyarwood: importlib does the magic for you
15:50:17 bauzas or importutils, either way
15:50:52 dansmith the imports are global because if they really import, we'll have gotten them up top,
15:51:01 dansmith the import in the class is purely to generate the exception for logging
15:51:39 dansmith using an import helper would be good for the mocking case, but otherwise the noqa seems fine to me, as I'm sure it's just complaining about the unused variable
15:51:53 dansmith you might be able to avoid it with "import rbd as _rbd" but it doesn't matter
15:52:30 sean-k-mooney i think its an unused import rather then unsued varibale so not suer the as will fix it but noqa i think is still fine
15:52:45 sean-k-mooney preferably with a comment as to why its there
15:54:46 sean-k-mooney for what its worth i prefer not using importlib/utiles simply due to the grep factor e.g. if im grepin for import rbd i wont see it but it is cleaner to use there helpers
15:55:08 bauzas hmmmm
15:55:15 sean-k-mooney that not really important but its why i personlly dont tend to use it
15:55:19 bauzas my own test shows me that the import isn't global

Earlier   Later