Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-11-06
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
15:55:33 sean-k-mooney bauzas: the import in the fuction is not the top levle one is
15:55:34 dansmith bauzas: the import is on L32
15:55:36 dansmith it's global
15:55:37 bauzas http://paste.openstack.org/show/799791/
15:55:37 lyarwood bauzas: the imports at the top of the file
15:55:57 lyarwood bauzas: we only retry these later in the class if the originals failed
15:56:23 dansmith this ^
15:56:29 sean-k-mooney and that is to get the reason it failed
15:56:29 bauzas lyarwood: right, so my purpose is to say that if you weren't able to import them at import time, you will get them at run time but only within the class namespace
15:56:47 dansmith bauzas: no, we won't import them at runtime either
15:57:06 bauzas I maybe missed the point then
15:57:08 lyarwood so we raise
15:57:18 lyarwood L139
15:57:21 bauzas oh, that's just for logging purpose ?
15:57:24 dansmith bauzas: the point is not to "try again", the point is to generate the import exception just for logging
15:57:24 lyarwood right
15:57:35 bauzas hah, ok
15:57:46 bauzas hence the F401
15:57:55 bauzas because the local import wasn't used
15:58:00 sean-k-mooney its something danpb suggested wehn we were trying to figure out why imports didnt work
15:58:23 bauzas lyarwood: but fwiw, imports aren't global
15:58:25 sean-k-mooney we think it was due to memory issues in the custoemr env since the had a bunc of OOM issue at the time
15:58:32 sean-k-mooney but we dont have any logs to say
15:58:46 sean-k-mooney bauzas: import happen in your current scope
15:58:50 openstackgerrit Lee Yarwood proposed openstack/nova master: rbd: Only log import failures when the RbdDriver is used https://review.opendev.org/761762
15:59:00 bauzas sean-k-mooney: right, that's what I said
15:59:09 sean-k-mooney so if you are at global scope they are if you are in function scope tehy are just in that scope
15:59:26 sean-k-mooney although that is only half though
15:59:29 sean-k-mooney *true
15:59:40 bauzas either way
15:59:56 sean-k-mooney the modul is actully imported gloablly including inialising all its global state it just made available in your local scope
16:00:14 sean-k-mooney if you import it somewere else that modules globalse state is still there
16:00:27 bauzas ok, the docstring helps
16:01:08 bauzas dansmith: honestly, I won't bother for tests here
16:02:00 dansmith lyarwood: couple comments on your comments
16:02:42 dansmith bauzas: well, I understand but a typo in a log message in an exception handler really defeats the point of the helper.. but I'd rather get it fixed than complicate it for testability
16:03:28 bauzas dansmith: fwiw I agree with your comments
16:04:03 bauzas I guess we need both rbd and rados modules to be present in order to work
16:04:18 bauzas so only one exception catch should be enough
16:04:51 dansmith ...like in the actual import.. I'm not sure why it's split in the original change or this one, but maybe lyarwood has a reason
16:07:09 lyarwood yeah I don't recall why, likely just leaving specific breadcrumbs but that's part of the issue with the first patch so I'll merge them back togther now
16:07:22 bauzas I guess because the log is different
16:07:32 dansmith sure, but the exception trace will tell you which one
16:07:40 bauzas yup, I was able to tell it
16:13:16 openstackgerrit Lee Yarwood proposed openstack/nova master: rbd: Only log import failures when the RbdDriver is used https://review.opendev.org/761762
16:14:21 dansmith gibi: ^
16:14:29 gibi looking..
16:14:41 dansmith lyarwood: wait, one more thing
16:15:00 lyarwood ^_^
16:16:23 bauzas dansmith: oh shit, I +Wd
16:16:34 dansmith bauzas: I was just joking
16:16:38 bauzas hah
16:16:53 dansmith but I figured gibi should look too
16:16:58 bauzas anyway, time to call it a wrap
16:17:20 bauzas gibi: please look at https://review.opendev.org/761762 which is going to the gate for the next hours
16:17:28 bauzas anyway, \o
16:18:31 bauzas man, I just found we powerplayed /o\
16:18:53 bauzas apologies
16:19:33 gibi no worries. It looks good to me too
16:19:35 openstackgerrit Sylvain Bauza proposed openstack/nova master: rbd: Only log import failures when the RbdDriver is used https://review.opendev.org/761762
16:20:09 gibi then I will add the Closes-Bug tag the the commit message ^^
16:20:34 gibi or not :D
16:20:39 gibi whatever
16:21:08 gibi bauzas: thanks for the commit message update
16:46:56 stephenfin sean-k-mooney: comment on https://review.opendev.org/#/c/751642/8/nova/virt/libvirt/driver.py@1588
16:53:45 sean-k-mooney i was asked to log it and assert the excption is caught
16:53:53 sean-k-mooney if i remove the logging i cant do either
16:54:14 sean-k-mooney im useing log.debug so i dont think that prints the stack trace the way log.excpetion does
16:55:58 sean-k-mooney stephenfin: im expecting it to log the excption message not the traceback
16:57:06 stephenfin sean-k-mooney: aha, right you are. You need to pass a flag for that to happen
17:21:58 openstackgerrit Merged openstack/os-vif stable/victoria: Refactor code of linux_net to more cleaner and increase performace https://review.opendev.org/756406
17:21:59 openstackgerrit Merged openstack/os-vif stable/victoria: Fix - os-vif fails to get the correct UpLink Representor https://review.opendev.org/759255
17:44:38 stephenfin gmann, johnthetubaguy: Are either of you going to have time to review https://review.opendev.org/#/q/topic:secure-rbac+(status:open+OR+status:merged)+project:openstack/placement with me. Would definitely benefit from your input
17:47:03 stephenfin lyarwood: I can respin https://review.opendev.org/#/c/761762/ real quick if you want?
17:51:47 openstackgerrit Stephen Finucane proposed openstack/nova master: rbd: Only log import failures when the RbdDriver is used https://review.opendev.org/761762
18:08:52 lyarwood stephenfin: yeah thanks, slightly confused how this worked before without this?
18:09:26 stephenfin you were only reading the value of the variable
18:09:38 stephenfin the global keyword is necessary when you write to it
18:10:39 stephenfin def bar():
18:10:39 stephenfin foo = None
18:10:40 stephenfin print(foo)
18:10:40 stephenfin bar()
18:10:40 stephenfin foo = 'test'
18:10:50 stephenfin ^
18:12:18 lyarwood right so the error is misleading, it's not the read in the conditional that's at fault, it's the import trying to overwrite it?
18:12:44 stephenfin sort of
18:12:54 stephenfin by trying to write to it, the variable becomes locally scoped
18:13:02 lyarwood yup got it
18:13:38 stephenfin flake8 can't catch these kind of issues, unfortunately
18:14:54 sean-k-mooney lyarwood: before it was locally importing every time and using the local import if it was not visable
18:15:08 sean-k-mooney you can read gloabls without the keyword

Earlier   Later