Volume type standard

[josephineSei]

This creates the new standard for volume types as mentioned in ticket #265

[artificial-intelligence]

I'll do another review later today.

Thanks for all the work you put into this!

[artificial-intelligence]

LGTM

[anjastrunk]

Please make the order of the keyword encrypted, replicated, number of replicas, ... in volume's description and name explicitly, similar to flavor naming standard.

[josephineSei]

1. I doubt that optional parameters like the number of replicas will ever be made a required standard. From my discussion with csps I think, they will stay optional. Therefore I would not like to include them into some keyword standard. But it should be enough if it is stated in the description.
2. That leaves us with only two aspects to standardize: [encrypted] and [replicated]. They both can occur together or alone. So possible beginnings for the description would be:
   • [encrypted]
   • [replicated]
   • [encrypted][replicated]
   • [replicated][encrypted]
3. We only recommend, that both required aspects are present, but not whether they should be combined or can be stand alone. That makes it harder to require a certain sequence.

So if you insist on having a special sequence I would add this, but I don't see any positive side effects from this.

[anjastrunk]

The positive side effect of having a dedicated order of keywords is at user side, IMO. As volume type properties are currently defined as string, a cloud service consumer has to parse strings in order to find a volume type with special behavior or properties. Having a standardized order, string parsing will be facilitated.

[artificial-intelligence]

@anjastrunk do you have a particular language in mind that relies on a certain order of arguments to facilitate string parsing?

imho most languages nowadays support some kind of simple pattern matching where the order is not that important?

overall I have no strong opinion if we should enforce the order, but we maybe should in either case explicitly define if this string is ordered or unordered.

[fkr]

@anjastrunk please comment

[mbuechse]

The text in the paragraphs above clearly states:

The description needs to begin with [encrypted] ...

The description needs to begin with [replicated] ...

Well, only one of these can be true. This does need some more work!

[josephineSei]

I adjusted the document so that a special order is required now.

[anjastrunk]

@fkr You are right, regular expression will not require any order. So I'm fine with extending the standard by adding, that key word are unordered.

[josephineSei]

I edited the standear as we discussed with @anjastrunk and @mbuechse

[josephineSei]

i changed it.

[josephineSei]

I added and adjusted the tests from @mbuechse s proposal, so we have one unified PR.
I also tested the tests on my devstack:

stack@devstack:~/devstack$ openstack volume type list --long
+---------------------+-------------+-----------+---------------------+----------------------+
| ID                  | Name        | Is Public | Description         | Properties           |
+---------------------+-------------+-----------+---------------------+----------------------+
| 1ed91a96-6db1-4746- | test-type   | True      | None                |                      |
| b413-bfc42e3919c2   |             |           |                     |                      |
| d0ff021e-9379-452d- | LUKS3       | True      | [scs:encrypted]     | encryption_enabled=' |
| 8596-f1669447edbd   |             |           | blabla              | 1'                   |
| 08ca1605-26c0-4e50- | LUKS2       | True      | [scs:encrypted]     |                      |
| 8ffd-cb1b59ae42e4   |             |           | blabla              |                      |
| 39bc174e-c075-4296- | LUKS        | True      | [scs:encrypted]     | cat='miau'           |
| 8a09-f3b274ae0caa   |             |           | blabla              |                      |
| 75056530-6107-4bed- | ceph        | True      | [scs:replicated]    | volume_backend_name= |
| a473-f6ebe5f80ff1   |             |           | replicated          | 'ceph'               |
| afa6fbd5-0c4d-4928- | __DEFAULT__ | True      | Default Volume Type |                      |
| a50d-d18e49a35327   |             |           |                     |                      |
+---------------------+-------------+-----------+---------------------+----------------------+
stack@devstack:~/devstack$ python3 test-volume-types.py --os-cloud devstack --debug
DEBUG: Connecting to cloud 'devstack'
DEBUG: LUKS3: feature list ['encrypted', 'replicated']
DEBUG: LUKS2: feature list ['encrypted']
DEBUG: LUKS: feature list ['encrypted']
DEBUG: ceph: feature list ['replicated']
DEBUG: volume types by feature: {'encrypted': ['LUKS3', 'LUKS2', 'LUKS'], 'replicated': ['LUKS3', 'ceph']}
DEBUG: Total critical / error / warning: 0 / 0 / 0

It correctly identifies all volume types with one or multiple aspects and skips the one without a description.

[mbuechse]

Now I'm really irritated that you copied my code verbatim, but you removed the paragraphs (empty lines) from my docstrings. (These are more or less suggested by PEP-8.)
Also, it would have been nice to at least have a Co-authored-by line in the commit.
Please reintroduce the newlines and include the Co-authored-by line.

[mbuechse]

Drop this paragraph. If it's unclear by now that other features are not recognized by this standard, and that arbitrary volume types may still exist, then we must improve the relevant places.

[anjastrunk]

LGTM
Just, found three minor typos.

[mbuechse]

I found a few typos and inconsistencies. I think they should be addressed, but I approve nonetheless so I don't block the process any further. Some of my remarks from earlier reviews still stand, particularly regarding language.

[artificial-intelligence]

LGTM but all open comments should be addressed in any form before this is merged.

only slightly related to this change:
I also just noticed that seemingly all tests (so actual code) don't seem to have some distinct copyright/licence notice so would fall under the general licence used in this repository, which happens to be a CC licence.
In general this is a good licence but should imho not really be used for code.

I also raised the licence topic simultaneously with @mbuechse via Matrix Chat to put it onto the SIG Standardization Agenda.

In the meantime it might be worth to add an SPDX Licence Header to the test code.

[fkr]

Thanks @josephineSei for your work on this.
Thanks everyone else who has worked on this!