14:01:27 <rosmaita> #startmeeting cinder 14:01:27 <opendevmeet> Meeting started Wed Aug 11 14:01:27 2021 UTC and is due to finish in 60 minutes. The chair is rosmaita. Information about MeetBot at http://wiki.debian.org/MeetBot. 14:01:27 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 14:01:27 <opendevmeet> The meeting name has been set to 'cinder' 14:01:32 <rosmaita> #topic roll call 14:01:36 <simondodsley> hi 14:01:38 <sfernand> hi 14:01:41 <walshh_> hi 14:01:43 <fabiooliveira> hi 14:01:43 <eharney> hi 14:01:47 <TusharTgite> hi 14:01:49 <shoffmann> Hi 14:02:25 <enriquetaso> hi 14:02:42 <tosky> hi 14:03:00 <rosmaita> good turnout 14:03:09 <rosmaita> #link https://etherpad.opendev.org/p/cinder-xena-meetings 14:03:52 <rosmaita> ok, let's get started 14:03:56 <rosmaita> #topic announcements 14:04:05 <rosmaita> os-brick Xena release next week 14:04:21 <rosmaita> 2 things happening in os-brick, both around the nvmeof 14:04:30 <rosmaita> agent 14:04:39 <rosmaita> #link https://review.opendev.org/c/openstack/os-brick/+/802691 14:04:48 <rosmaita> nvmeof multiple volume support 14:04:58 <rosmaita> #link https://review.opendev.org/c/openstack/os-brick/+/800014 14:05:17 <rosmaita> those are the highest priority reviews right now 14:05:26 <rosmaita> so, please review accordingly 14:05:51 <rosmaita> also, would be good if each of the teams working on the two patches mentioned reviewed each other's work 14:06:47 <rosmaita> and, of course, it would be good if cinder people in general review the two mentioned patches 14:07:11 <rosmaita> i think that's it for announcements 14:07:12 <eharney> 800014 shows merge conflict, fyi 14:07:24 <rosmaita> yeah, i noticed that 14:07:45 <rosmaita> pro tip for driver developers: merge conflicts are a real turn-off 14:07:54 <e0ne> :) 14:08:00 <rosmaita> important to get those resolved quickly or people won't review 14:08:45 <rosmaita> ok, that's all for announcements, i think 14:09:02 <rosmaita> #topic How to solve retype error with nas_secure_file_operations 14:09:06 <rosmaita> shoffmann: that's you 14:09:15 <shoffmann> Hi, thanks. 14:09:47 <shoffmann> We saw the issue, that retype of available volumes fail when using nfs share, if nas_secure options are used. 14:10:03 <shoffmann> means, root is not allowed to write to that share. 14:10:17 <shoffmann> #link https://bugs.launchpad.net/cinder/+bug/1938196 14:10:34 <shoffmann> My question is, how we can solve this in a nice way. 14:11:11 <eharney> my -2 was against adding a new config option, so i think that no longer applies 14:11:12 <shoffmann> use the existing nas_secure_file_operations option but also in volume/manage.py (where it is currently not defined for all backends) or introduce a new option. 14:11:25 <shoffmann> Maybe there are also other ideas. 14:11:41 <shoffmann> #link https://review.opendev.org/c/openstack/cinder/+/802882 14:12:35 <shoffmann> Right now, i check, if the option is defined, but maybe this is a bit ugly, because the nas_secure option is related to some backend types only. 14:12:42 <eharney> i'm not sure that the current patch will work in all scenarios 14:13:07 <eharney> IIRC root access for _qemu_img_info is needed when volumes are attached, regardless of the nas_secure options 14:13:19 <eharney> (because they may be owned by the qemu user via nova) 14:14:06 <shoffmann> In our setup, we configured nova to not change the owner of the file. Not sure, if it is possible, to enable nas_secure on NFS share and let nova change the owner. 14:14:19 <shoffmann> Because at NetApp only one user and one group can be allowed. 14:14:28 <shoffmann> So we use cinder:nova 14:14:47 <eharney> this code is in the generic nfs driver 14:15:24 <eharney> years ago i think we generally felt that the nas_secure options weren't fully implemented correctly, do we know if there are other fixes needed like this to successfully use nas_secure? 14:16:33 <shoffmann> Till now we didn't saw other issues. But maybe others know more. 14:17:32 <shoffmann> Is it possible for generic/other nfs drivers to define nas_secure with many allowed users? 14:17:39 <enriquetaso> I haven't seen any other bug related to nas_secure, but I'll check 14:17:46 <eharney> enriquetaso: there have been many 14:18:02 <eharney> yes, i think generic nfs configurations commonly do that 14:18:58 <eharney> these changes are also risky because there's a matrix of configuration options (two nas_secure* options), the generic nfs driver, netapp nfs drivers, whether you configure root_squash, etc.... it's hard to know that a change like this won't break something (which i think it does) 14:19:31 <rosmaita> what was the new option (that eric doesn't like)? 14:20:04 <shoffmann> Something like "secure_file_operation" or so. 14:20:34 <shoffmann> Adding a NetApp specific option also sounds bad. 14:20:44 <rosmaita> what was it going to do that you couldn't do with the current options? 14:20:47 <eharney> the purpose of the new option overlapped with the existing option 14:21:04 <rosmaita> and yea, we want to avoid vendor-specific options 14:21:51 <enriquetaso> On the reported bug side: i think i can start tagging all this bugs with 'Tags: nas_secure' or something like that to have them all together 14:22:02 <rosmaita> enriquetaso: ++ 14:22:07 <eharney> i wonder if the easier fix for the qemu_img_info case wouldn't be to just attempt running it both as root and non-root, or maybe look at the files being inspected to decide which to do 14:22:12 <eharney> rather than trying to tie it to config options 14:23:07 <shoffmann> Are there use cases, using the nas_secure_* but allow root to write to the nfs share? 14:23:43 <shoffmann> eharney I also can implement this 14:23:59 <shoffmann> same for _connect_device in volume/manager.py? 14:25:25 <eharney> we may need a list of use cases for all 4 combinations of the existing nas_secure options, i'm not sure if we even know that they are all valid combinations 14:27:09 <rosmaita> and what's the deal with 'auto'? seems like we would always be using True for both on fresh installs 14:28:00 <eharney> whether auto ends up as true or false depends mostly on how the permissions on the share are configured 14:28:19 <rosmaita> oh, ok 14:28:46 <eharney> we basically let deployers decide if they want to have NFS volume files owned as root or as a different user 14:29:11 <rosmaita> so that's what you were saying about inspect the files to decide what to do 14:29:18 <eharney> which is the decision that the nas_secure_file_permissions option is about 14:29:34 <eharney> the nas_secure_file_operations option is about whether to run commands as root via sudo or just as the cinder user 14:29:58 <eharney> yes 14:31:18 <rosmaita> slightly off topic, we have a devstack-plugin-nfs-tempest-full zuul job ... i wonder how hard it would be to reconfigure it to test this change 14:31:40 <eharney> we could do that, but we'd also need to examine what coverage we get from tempest tests 14:31:47 <eharney> should not be hard to do, though 14:32:28 <rosmaita> good point about what exactly tempest is checking 14:33:03 <rosmaita> but it looks like we will really need a "live" test of this change to see what happens 14:33:14 <eharney> yeah, there are a lot of details about file permissions with volumes that are attached, migrating, etc 14:33:42 <shoffmann> than I will try to implement checking file permissions. If I should add/change tests to tempest, please tell me (e.g. at the change or bug) 14:34:19 <shoffmann> Or should I wait for what we will do with all the nfs share issues? 14:35:11 <rosmaita> i think the file checking permissions is a good start 14:35:37 <rosmaita> but it would be helpful if you think about the nfs share issues in general, especially because you work a lot with nfs 14:36:27 <rosmaita> anybody want to volunteer to see what tempest and/or cinder-tempest-plugin test that will be relevant for this change? 14:37:12 <rosmaita> shoffmann: could be helpful if you could list some scenarios where this bug surfaces as a start 14:37:35 <shoffmann> sure, I can help 14:37:50 <shoffmann> What is a good place for this? mailing list or next meeting? 14:38:17 <rosmaita> shoffmann: i think put some scenarios into an etherpad and then send a note to the mailing list 14:38:25 <enriquetaso> i need to get familiar with this, i'd like to group all the related bugs first and list the scenarios we are currently having, but I can help with the tempest coverage as well :) 14:38:33 <enriquetaso> rosmaita++ 14:38:40 <enriquetaso> sounds better 14:38:56 <rosmaita> but shoffmann we should continue to follow this at the cinder meetings too 14:39:06 <rosmaita> (it's not an either/or is what i mean) 14:39:23 <rosmaita> ok, thanks, let's move on 14:39:23 <sfernand> enriquetaso yes me too, but I can check the nas_secure option with the netapp nfs driver 14:39:24 <eharney> fyi all of this interacts w/ cinder-backup too 14:39:52 <rosmaita> eharney: good point! we don't want to break backups 14:39:59 <rosmaita> shoffmann: ^^ 14:40:25 <rosmaita> #topic Cinder matrix - Clarification on Snapshot Attachment 14:40:29 <rosmaita> walshh_: that's you 14:40:34 <walshh_> Apologies for bringing this one up again. Differing opinions 14:40:37 <rosmaita> #link https://review.opendev.org/c/openstack/cinder/+/800015 14:40:55 <walshh_> This functionality is exposed via rpcapi (as a cinder backup enhancement) and not cinder API so maybe it should not be in the matrix at all. 14:41:14 <walshh_> Only a couple of drivers can attach snapshots, most cannot. 14:41:44 <walshh_> Just wondering is removing it might be the best option as it is certainly causing confusion with customers 14:41:49 <rosmaita> there was some confusion about this on the mailing list, too 14:42:04 <walshh_> and confusing us also :-) 14:42:05 <rosmaita> i think you may be right about removing it from the matrix 14:42:11 <eharney> i think that makes sense, since it isn't actually a feature that is exposed to users 14:42:18 <simondodsley> If this is only for cinder-backup use then I will remove my +1 and agree that it be removed completely rom the matrix 14:42:34 <walshh_> And customers do not like to see x's against features 14:42:39 <rosmaita> maybe move it over somewhere in the driver development docs? so people at least know to think about it 14:42:45 <walshh_> even if it is not possible to implement in a driver 14:42:49 <rosmaita> where by 'people' i mean driver developers 14:43:19 <walshh_> sure I can do that if I am pointed in the correct direction 14:44:04 <rosmaita> ok, why don't you go ahead and revise your patch to just remove it, and if someone finds a good place to add a statement, they can tell you on the review 14:44:13 <walshh_> so if there is no other strong opinions for it to remain, I will remove it 14:44:30 <rosmaita> sounds good ... if anyone has 2nd thoughts, leave comments on the review 14:44:34 <rosmaita> thanks helen 14:44:38 <walshh_> thanks all 14:44:49 <rosmaita> #topic RBD fixes that we really need 14:44:53 <rosmaita> enriquetaso: you're up 14:45:11 <enriquetaso> I'd like to point out these two patches here that I think are ready for review. (1) Currently the Ceph backend fails when restoring non ceph volumes. 14:45:24 <enriquetaso> #link https://review.opendev.org/c/openstack/cinder/+/750782 14:45:30 <enriquetaso> and.. 14:45:36 <enriquetaso> (2) When creating an encrypted volume from an encrypted image the volume gets truncated and it's inaccessible after the operation. 14:45:44 <rosmaita> that is a bad one 14:45:47 <enriquetaso> #link https://review.opendev.org/c/openstack/cinder/+/801522 14:46:03 <enriquetaso> yes, i think rbd really needs both asap 14:46:14 <rosmaita> ok, reminder that RBD is a community driver, so it deserves extra love 14:46:22 <enriquetaso> :) 14:46:27 <enriquetaso> thanks! 14:46:27 <rosmaita> thanks sofia 14:46:41 <rosmaita> #topic Fix for PowerVC 14:46:47 <rosmaita> simondodsley: you have the floor 14:46:52 <simondodsley> Thanks - This is an XS patch that we need to merge so we can create an urgent backport to Wallaby. PowerVC will use Wallaby for its next release. PowerVC uses `provider_id` heavily and so we need it to be correctly associated with all volumes in consistency groups. 14:47:14 <simondodsley> https://review.opendev.org/c/openstack/cinder/+/803046 14:47:51 <rosmaita> ok, reminder to all driver maintainers -- it is helpful to review other driver patches 14:48:02 <simondodsley> I try to :) 14:48:09 <rosmaita> (hint to anyone asking in #openstack-cinder for reviews) 14:48:35 <rosmaita> simondodsley: that wasn't aimed at you, i have noticed your name showing up on all sorts of revieww 14:48:57 <rosmaita> thanks simone 14:49:03 <rosmaita> sorry about the type 14:49:06 <rosmaita> typo 14:49:09 <simondodsley> lol 14:49:23 <rosmaita> #topic mypy inline comments 14:49:37 <rosmaita> i just saw these on https://review.opendev.org/c/openstack/cinder/+/802882/7/cinder/volume/manager.py 14:49:54 <rosmaita> iirc, pylint did this too for a while but we turned it off somehow 14:50:23 <rosmaita> just want to see what people's opinions are about what we as a team want here 14:50:49 <rosmaita> inline comments are certainly in your face and motivate fixing things 14:51:11 <rosmaita> so maybe leave them? 14:51:52 <rosmaita> ok, not hearing an outburst of opinions here, let's leave them for now 14:52:03 <eharney> fwiw, the comments on https://review.opendev.org/c/openstack/cinder/+/802882/7/cinder/volume/manager.py look kind of odd 14:52:13 <rosmaita> although, let's review and merge this patch that fixes the particular problem 14:52:16 <eharney> are they on the right lines? 14:52:27 <rosmaita> #link https://review.opendev.org/c/openstack/cinder/+/784453 14:52:33 <rosmaita> i don't know, looking now 14:52:47 <Roamer`> personal opinion from somebody whose colleagues can sometimes get tired of "please also run this check before pushing to Gerrit": I like advice about improvements, and I like to keep my code as clean as possible, but then I suppose that's common among people gathered here :) 14:53:57 <eharney> the comments also seem to be reported twice 14:54:03 <rosmaita> eharney: line 980 doesn't seem to make sense, and the duplication is not nice 14:54:29 <eharney> i guess they're appeared twice because there was a recheck and the job ran twice 14:55:00 <Roamer`> but yeah, in this particular case the line numbers in https://zuul.opendev.org/t/openstack/build/8b0af55849674691ac9b5c032ac2ec81 and the quoted "return True" statements do not seem to correspond to what is shown in Gerrit 14:55:09 <rosmaita> just as a side note, there's a patch listed in https://tiny.cc/CinderPriorities where mypy would have helped avert a bug 14:55:22 <rosmaita> so i encourage everyone to continue to review eharney's patches 14:55:23 <Roamer`> is there any chance mypy was running on the wrong files?! 14:55:34 <eharney> Roamer`: this could be an artifact of zuul doing a merge before the mypy run, need to check the logs for that 14:56:11 <Roamer`> eharney, oh, of course 14:56:27 <Roamer`> eharney, yeah, don't know why I didn't realize that when I fight with Zuul merges in our CI weekly :) 14:56:42 <Roamer`> hm, so this means that the line numbers will *almost certainly* be wrong 14:57:16 <rosmaita> that would seem to indicate that inline comments won't be helpful 14:57:21 <Roamer`> right 14:57:28 <eharney> i can't remember if zuul does that in the check queue or just the gate queue 14:58:40 <Roamer`> eharney, I think it has to do it in the check queue, too... I don't see how things would work without that (it is quite often that the Gerrit change is not made against the tip of the branch) 14:58:47 <Roamer`> (at least it does in our CI) 14:58:57 <eharney> i think it does: https://zuul.opendev.org/t/openstack/build/8b0af55849674691ac9b5c032ac2ec81/log/job-output.txt#334 14:59:27 <eharney> because it's running on a merge commit of 802882 and not the commit itself... so yeah i'm not sure how the line number thing would actually work 15:00:10 <Roamer`> yeah, if the merge is kinda-sorta-trivial, then one could think of writing some tool that parses the Python AST and figures out the line number deltas, but even that could go wrong 15:00:32 <rosmaita> ok, looks like we should turn off the inline comments 15:00:58 <rosmaita> i will follow up on that 15:01:06 <rosmaita> all right, we are out of time 15:01:14 <rosmaita> everybody: os-brick reviews!!! 15:01:31 <Roamer`> BTW something that I didn't add to the etherpad (I thought it was too trivial, maybe I should have)..... what are the chances for a StorPool trivial performance improvement - implement revert-to-snapshot natively? https://review.opendev.org/c/openstack/cinder/+/680889 15:01:48 <Roamer`> (and yes, I realize that I should do more reviews myself, sorry about that) 15:01:56 <rosmaita> :) 15:02:33 <rosmaita> seems ok ... file a launchpad BP because we are coming up on M-3 feature freeze soon 15:02:42 <rosmaita> that will help us prioritize 15:02:45 <rosmaita> thanks everyone! 15:02:46 <Roamer`> I did, it's at https://blueprints.launchpad.net/openstack/?searchtext=storpool-revert-to-snapshot 15:02:52 <Roamer`> but I only did it the day before :) 15:02:54 <Roamer`> and thanks 15:03:03 <rosmaita> ok, thanks i will make sure it's in the xena mix 15:03:08 <rosmaita> #endmeeting