15:01:59 <enriquetaso> #startmeeting cinder_bs
15:02:01 <openstack> Meeting started Wed May  5 15:01:59 2021 UTC and is due to finish in 60 minutes.  The chair is enriquetaso. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:02:02 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
15:02:04 <openstack> The meeting name has been set to 'cinder_bs'
15:02:14 <enriquetaso> #link https://etherpad.opendev.org/p/cinder-bug-squad-meeting
15:02:15 <rosmaita> o/
15:02:21 <enriquetaso> I don't have much for today's meeting. Cinder has eleven more reported bugs from 2021-04-21 to 2021-05-05.
15:02:22 <whoami-rajat> hi
15:02:28 <enriquetaso> :)
15:02:29 <enriquetaso> You can check them in the mailing list. Topic: '[cinder] Bug deputy report for week of 2021-05-05 - OpenStack-dev mailing list'
15:02:39 <enriquetaso> #topic bug_1 'Cinder backup python3 encoding issue'
15:02:47 <enriquetaso> #link https://bugs.launchpad.net/cinder/+bug/1925809
15:02:47 <openstack> Launchpad bug 1925809 in Cinder "cinder backup python3 encoding issue" [Medium,New]
15:02:54 <enriquetaso> Summary: Looks like there may be a couple of encoding python3 issues in the backup ceph driver.
15:03:15 <enriquetaso> This can be fixed by encoding the zero stream like:
15:03:15 <enriquetaso> zeroes = '\0' * self.chunk_size
15:03:15 <enriquetaso> zeroes = zeroes.encode('utf-8')
15:03:15 <enriquetaso> though I'm not 100% sure if that does anything untoward or properly zeroes the remaining space.  What do you think about this fix?
15:03:46 <eharney> if it's a bytes vs str confusion bug we should fix it
15:04:38 <enriquetaso> OK
15:04:49 <eharney> (and then land type annotations that would prevent that...)
15:05:00 <enriquetaso> yes, mypy is important
15:05:32 <enriquetaso> #action (enriquetaso) proposed a fix for that
15:05:38 <rosmaita> i thought \0 was the null character?
15:05:47 <enriquetaso> i think so
15:06:00 <rosmaita> that should be the same in ascii and utf-8, i think
15:06:48 <eharney> it failed with a TypeError indicating it wants bytes rather str regardless of if they're the same
15:06:52 <eharney> rather than*
15:06:54 <rosmaita> current error is 2021-04-23 09:43:02.803 20 ERROR cinder.backup.manager TypeError: a bytes-like object is required, not 'str'
15:07:30 <eharney> i think that may come from some of the C ceph lib bindings which check such things now
15:07:45 <eharney> but it's not too obvious from the backtrace
15:08:14 <enriquetaso> need to debug a bit more then
15:08:21 <tosky> do we know which version of ceph *and* libraries?
15:08:44 <eharney> i don't think it matters too much, it probably would have been better to pass bytes on the older versions too
15:08:53 <enriquetaso> cinder (16.2.1) (though I believe it will affect all versions including master)
15:09:19 <eharney> one of the bugs i fixed for ceph pacific support was a similar thing where we used to pass str and it no longer allowed it
15:10:25 <enriquetaso> OK, i'll search your patch to see how did you fix it
15:11:07 <enriquetaso> any other suggestions? moving on..
15:11:32 <enriquetaso> #topic Open Discussion
15:11:35 <rosmaita> only thing i'm wondering is wehther we should do the conversion first
15:12:04 <rosmaita> what i mean is, we create a chunk_size str and then convert to bytes
15:12:07 <enriquetaso> rosmaita, good question, let me check the pacific patch quick
15:12:34 <rosmaita> i wonder whether we should create a chunk_size byte array to begin with?
15:13:09 <eharney> for "zeroes"?  yes probably
15:13:29 <rosmaita> yeah
15:13:32 <eharney> we don't need to make a str and then encode it..
15:13:42 <enriquetaso> https://review.opendev.org/c/openstack/cinder/+/773694
15:14:58 <rosmaita> also looks like we'll need to fix this line: https://github.com/openstack/cinder/blob/stable/ussuri/cinder/backup/drivers/ceph.py#L394
15:15:47 <eharney> i'm not sure what you want to get from 773694.  i was just noting that current ceph libraries enforce type checks about bytes vs. str where they didn't previously
15:16:15 <rosmaita> well, it does remind me that b'\o
15:16:24 <rosmaita> i mean
15:16:31 <rosmaita> b'\0'
15:16:42 <rosmaita> will do what we want without encoding anything
15:17:46 <eharney> iirc you can just do bytearray(chunk_size) to get what is needed
15:18:32 <rosmaita> even better
15:18:42 <eharney> because it inits to 0s
15:18:48 <eharney> https://docs.python.org/3/library/stdtypes.html#bytearray
15:19:04 <enriquetaso> cool, thanks
15:19:09 <rosmaita> i like that way better
15:19:18 <rosmaita> much clearer about what we're trying to accomplish there
15:19:55 <enriquetaso> eharney++
15:19:58 <enriquetaso> rosmaita++
15:20:02 <enriquetaso> guess we don't have any other topic for today
15:20:17 <eharney> i'll mention one bug
15:21:07 <eharney> https://bugs.launchpad.net/cinder/+bug/1926630 is a good one that i just patched up
15:21:07 <openstack> Launchpad bug 1926630 in Cinder "Creating an encrypted volume type without a cipher accepted but will fail to create an encrypted volume" [Medium,In progress] - Assigned to Eric Harney (eharney)
15:21:10 <eharney> (mostly asking for reviews i guess)
15:21:38 <eharney> i added validation around the issues directly pointed to by the bug, but i wouldn't be surprised if there are one or two more corner cases to find in this API or related ones
15:22:13 <enriquetaso> #link https://bugs.launchpad.net/cinder/+bug/1926630
15:22:13 <openstack> Launchpad bug 1926630 in Cinder "Creating an encrypted volume type without a cipher accepted but will fail to create an encrypted volume" [Medium,In progress] - Assigned to Eric Harney (eharney)
15:22:29 <eharney> but basically an admin is currently allowed to create encryption types that don't work, which seems to not be dangerous, but is definitely annoying since the CLI clients don't really make you do the right thing
15:22:51 <rosmaita> i was looking at that yesterday
15:23:06 <eharney> my first patch hardens this up for existing types, and the second prevents admins from making bad types, basically
15:23:11 <rosmaita> is there a canonical list of acceptable ciphers in x-y-z format?
15:23:44 <eharney> i think not, but tbh we've never advised that anyone use anything other than aes-xts-plain64
15:24:09 <rosmaita> ok, definitely good to fix this
15:24:10 <eharney> a few others are definitely possible and should work, but... probably not best practice
15:26:16 <enriquetaso> so far, all the bugs related to encryption uses --cipher aes-xts-plain64 . There's a bug using cryptsetup --batch-mode luksFormat --key-file=- --cipher aes-xts-plain64 --key-size 256 /dev/dm-19
15:26:17 <eharney> btw, i tried to use our validation schema code to check some of this, and wrote my own code instead when the schema required fields specs didn't seem to work like i'd expect them to
15:26:35 <eharney> enriquetaso: that is the only configuration we've ever documented using
15:27:02 <eharney> except for when we used to document 512 bit keys i guess, but those don't even work in barbican
15:28:23 <enriquetaso> so, the current validation schema code isn't working properly ?
15:28:59 <eharney> the current validation schema for encrypted type creation doesn't require one or two fields that probably should be required
15:29:57 <enriquetaso> OK, thanks for working on this eharney
15:30:11 <rosmaita> eharney: and it does require an optional field!
15:30:18 <eharney> does it?
15:31:03 <rosmaita> https://opendev.org/openstack/cinder/src/branch/master/cinder/api/schemas/volume_type_encryption.py
15:31:13 <rosmaita> line 35
15:31:36 <rosmaita> bet it was a typo, they meant provider and cipher?
15:31:54 <eharney> well, requiring control_location is reasonable, but tbh the whole control_location thing probably needs a revisit anyway
15:32:33 <rosmaita> we're showing control_loc as optional in the api-ref: https://docs.openstack.org/api-ref/block-storage/v3/?expanded=create-an-encryption-type-detail#create-an-encryption-type
15:32:47 <eharney> i'm really not sure what the current state is of control_location=back-end
15:33:00 <eharney> rosmaita: humm
15:33:36 <eharney> it's been hardwired into my brain for so many years to pass it from the CLI that i probably haven't really thought about it..
15:34:47 <eharney> should probably go investigate control_location=back-end and make sure it doesn't hold nasty surprises for an admin that doesn't really know how this works
15:35:04 <enriquetaso> i should fill a bug for the control_location optional vs required
15:35:18 <rosmaita> enriquetaso: ++
15:35:21 <eharney> rosmaita: i guess it would make sense to make cipher optional and default to the one that everyone wants, maybe worth digging into
15:35:35 <rosmaita> we need to figure this out
15:36:06 <eharney> i'm a little unclear on how much should be validated via API schema vs. in the code etc...
15:36:31 <enriquetaso> i have the same concern
15:36:38 <eharney> a general sense i had while working on this is that if it's in the code, it's at least in front of you in the right place to understand it more explicitly, and you can throw more specific errors
15:37:15 <eharney> because for something like encryption details, even if the API schema validates it, we probably need to validate it everywhere else anyway
15:37:16 <rosmaita> well, we can at least get the required/optional elements and their general datatypes into the schema correctly
15:37:32 <eharney> i think we need to decide which ones are really required/optional to do that
15:37:59 <eharney> also our api-ref says key_size is usually 256 but shows 128 in the example   *facepalm*
15:38:49 <rosmaita> what happens if you use --key_size None ?
15:39:16 <rosmaita> i mean, the default isn't None, wouldn't it be an actual integer?
15:39:18 <eharney> i think i tried that yesterday and found that it is validated
15:39:45 <eharney> because i was poking around trying to find concerns similar to what the bug reported
15:40:13 <rosmaita> key_size = {'type': ['string', 'integer', 'null'],
15:40:19 <rosmaita> so yeah
15:41:00 <eharney> but it has minimum: 0, so presumably it can't actually be null?  i don't know..
15:41:13 <eharney> using MAX_INT for the maximum is pretty funny though
15:41:32 <rosmaita> that would be quite a key
15:42:31 <enriquetaso> #action (enriquetaso) fix the key_size in the example https://docs.openstack.org/api-ref/block-storage/v3/?expanded=create-an-encryption-type-detail#create-an-encryption-type
15:42:37 <rosmaita> ok, so it looks like we need some doc + schema + api-ref cleanup around encryption
15:43:09 <enriquetaso> wow
15:43:23 <eharney> i think this still leaves my current patches ok to go but i'll look over them again
15:43:33 <enriquetaso> thanks eharney
15:44:05 <enriquetaso> OK, so control_location is not optinal?
15:44:15 <rosmaita> yes, i agree that your patches are good for now, and then we need to follow up
15:44:40 <rosmaita> enriquetaso: it looks like the schema-validation will require it
15:44:46 <eharney> enriquetaso: it's probably reasonable for it to be optional, i'm not sure if it currently is or not
15:44:50 <rosmaita> but that doesn't mean that it's really required
15:45:02 <rosmaita> i mean in the "big picture" sense of required
15:45:02 <enriquetaso> oh, ok
15:46:23 <enriquetaso> any other topic/bug to discuss ?
15:46:55 <rosmaita> nothing from me
15:48:02 <enriquetaso> OK, i'll end up the meeting then
15:48:10 <enriquetaso> See you next week
15:48:38 <enriquetaso> #endmeeting