search

SovereignCloudStack / standards

SCS standards in a machine readable format
https://scs.community/
Creative Commons Attribution Share Alike 4.0 International
34 stars 23 forks source link

Upstream work on the discoverability of certain recommended volume type aspects. #449

Open josephineSei opened 10 months ago

josephineSei commented 10 months ago

As a user, I want to be able to look into a volume type object and see all aspects it fulfills so that I can choose the best suited volume type for my volumes.

In #265 a standard for volume types is created. Right now SCS, providers and consumers need to rely on Tags in the description of a volume type to discover volume types with the recommended aspects encrypted and replicated.

We want to find a way to use the internal extra_spec in volume types for the description of those two aspects, when they are present in a volume type. If this is not possible, we would like to introduce another property, which has to be set by the admin, when setting the volume type. Only after that we will have the possibility to automatically check for a volume type with replication or encyption.

Definition of Done:

josephineSei commented 1 month ago

I started looking into implementing a filter. When listing all volume types I want to be able to filter by the presence of certain metadata.

josephineSei commented 1 month ago

The filtering mechanism now also works:

stack@devstack:~/devstack$ openstack volume type list --metadata 'tiger'='growl'
+--------------------------------------+------+-----------+
| ID                                   | Name | Is Public |
+--------------------------------------+------+-----------+
| 87f89839-6f55-4548-b041-e2b451dbcf89 | test | True      |
+--------------------------------------+------+-----------+
stack@devstack:~/devstack$ openstack volume type list --metadata 'tiger'='growl' --long
+--------------------------------------+------+-----------+-------------+------------+
| ID                                   | Name | Is Public | Description | Properties |
+--------------------------------------+------+-----------+-------------+------------+
| 87f89839-6f55-4548-b041-e2b451dbcf89 | test | True      | None        |            |
+--------------------------------------+------+-----------+-------------+------------+

But seeing the output, i think I should also adjust the CLI to properly show the metadata in the listing, the information is already given by the API:

RESP BODY: {"volume_types": [{"id": "87f89839-6f55-4548-b041-e2b451dbcf89", "name": "test", "is_public": true, "description": null, "metadata": {"tiger": "growl", "cat": "miau"}, "extra_specs": {}, "qos_specs_id": null, "os-volume-type-access:is_public": true}]}
GET call to volumev3 for http://192.168.23.161/volume/v3/db05e1116cb04ac689d82c738a5f103c/types?metadata=%7B%27tiger%27%3A+%27growl%27%7D&is_public=None used request id req-5a8f83db-9c56-4d61-afeb-ce5eaf7c64f7
+--------------------------------------+------+-----------+
| ID                                   | Name | Is Public |
+--------------------------------------+------+-----------+
| 87f89839-6f55-4548-b041-e2b451dbcf89 | test | True      |
+--------------------------------------+------+-----------+
josephineSei commented 1 month ago

I started taking a look into the failing tests and will also add unit tests.

josephineSei commented 1 month ago

I rebased the patches and added unit tests for the cinderclient. I will continue with the other patches.

josephineSei commented 1 month ago

The pipeline for the client patches is green. I am now investigating the Errors on the Cinder patches, to prepare for the next PTG.

josephineSei commented 4 weeks ago

The patch, which adds the database table (https://review.opendev.org/c/openstack/cinder/+/918316) and just also ask for the metadata table content, when the volume types are listed / showed - has only one failing unit test, which is not due to the changes, some mypy failures (also in code parts I did not change) and tempest tests, which require the tempest change to be in place.

The first things cannot be handled by me, and the tempest tests - i looked through the failing tests and it seems that all (also the failing plugin tests) seem to fail validation, which needs the schema adjustment in tempest. So there are two patches which rely on each other, that cannot be merged without each other - I will ask Cinder, how they deal with such things.

Also I think there is no more unit tests needed for this patch then the ones I adjusted and the one I wrote, that checks the db upgrade itself.

I now start looking into the other Cinder patch, that focuses on adding new API endpoints, and the handling between endpoints and the db.

josephineSei commented 4 weeks ago

While implementing the missing shown metadata column in the openstack volume type list --long answer and trying to fix the unit tests therefore, I once again came to the point to think about microversions. Cinder requires microversions for each new API endpoint / changes response, etc... So the Cinder API part will definitely need a microversion and therefor also the openstackclient and the unit tests in the openstack client have to be aware of that microversion.

I will hold on further implementing unit tests for the client and start digging into the microversion stuff more. So this will not yet be commited to upstream patches:

stack@devstack:~/devstack$ openstack volume type list --long
+--------------------------------------+-------------+-----------+--------------------------------------------------------------------------+------------+
| ID                                   | Name        | Is Public | Description                                                              | Properties |
+--------------------------------------+-------------+-----------+--------------------------------------------------------------------------+------------+
| 87f89839-6f55-4548-b041-e2b451dbcf89 | test        | True      | None                                                                     |            |
| 15002270-b9ed-4b7f-ba08-cea35f3acc1f | LUKS        | True      | [scs:encrypted, replicated] Content will be replicated three times to    |            |
|                                      |             |           | ensure consistency and availability for your data. LUKS encryption is    |            |
|                                      |             |           | used.                                                                    |            |
| 74241c6f-6678-4388-9f69-d16d91793cc1 | ceph        | True      | None                                                                     |            |
| 41a4e540-0660-4562-aeb1-e3891121c88f | __DEFAULT__ | True      | Default Volume Type                                                      |            |
+--------------------------------------+-------------+-----------+--------------------------------------------------------------------------+------------+
clean_up ListVolumeType: 
END return value: 0
stack@devstack:~/devstack$ nano /opt/stack/data/venv/lib/python3.10/site-packages/openstackclient/volume/v3/volume_type.py
stack@devstack:~/devstack$ openstack volume type list --long
+--------------------------------------+-------------+-----------+------------------------------------------------------------+------------+---------------------------+
| ID                                   | Name        | Is Public | Description                                                | Properties | Metadata                  |
+--------------------------------------+-------------+-----------+------------------------------------------------------------+------------+---------------------------+
| 87f89839-6f55-4548-b041-e2b451dbcf89 | test        | True      | None                                                       |            | cat='miau', tiger='growl' |
| 15002270-b9ed-4b7f-ba08-cea35f3acc1f | LUKS        | True      | [scs:encrypted, replicated] Content will be replicated     |            |                           |
|                                      |             |           | three times to ensure consistency and availability for     |            |                           |
|                                      |             |           | your data. LUKS encryption is used.                        |            |                           |
| 74241c6f-6678-4388-9f69-d16d91793cc1 | ceph        | True      | None                                                       |            |                           |
| 41a4e540-0660-4562-aeb1-e3891121c88f | __DEFAULT__ | True      | Default Volume Type                                        |            |                           |
+--------------------------------------+-------------+-----------+------------------------------------------------------------+------------+---------------------------+
josephineSei commented 4 weeks ago

I started adding a microversion for my patch by following this documentation: /home/kiari/Dokumente/upstream/cinder/api-ref/source/v3/samples/volume_type/volume-type-show-response.json I am working on point 8 of the "other necessary changes" before I will add the decorators. I pushed my current state of work to this patch: https://review.opendev.org/c/openstack/cinder/+/928794

josephineSei commented 3 weeks ago

I need to adjust all the patches to work properly with microversions (that means also adjust the openstackclient). But as of right now it seems to work (at least for that command):

stack@devstack:~/devstack$ openstack volume type show test
+--------------------+--------------------------------------+
| Field              | Value                                |
+--------------------+--------------------------------------+
| access_project_ids | None                                 |
| description        | None                                 |
| id                 | 87f89839-6f55-4548-b041-e2b451dbcf89 |
| is_public          | True                                 |
| name               | test                                 |
| properties         |                                      |
| qos_specs_id       | None                                 |
+--------------------+--------------------------------------+
stack@devstack:~/devstack$ openstack volume type show test --os-volume-api-version 3.72
versions supported by client: 3.0 - 3.71
stack@devstack:~/devstack$ nano /opt/stack/data/venv/lib/python3.10/site-packages/cinderclient/api_versions.py 
stack@devstack:~/devstack$ openstack volume type show test --os-volume-api-version 3.72
+--------------------+-------------------------------------------------------+
| Field              | Value                                                 |
+--------------------+-------------------------------------------------------+
| access_project_ids | None                                                  |
| description        | None                                                  |
| id                 | 87f89839-6f55-4548-b041-e2b451dbcf89                  |
| is_public          | True                                                  |
| metadata           | {'fox': 'ehehehehe', 'tiger': 'growl', 'cat': 'miau'} |
| name               | test                                                  |
| properties         |                                                       |
| qos_specs_id       | None                                                  |
+--------------------+-------------------------------------------------------+

This may also make adjustments of test easier.

josephineSei commented 3 weeks ago

After introducing the microversion I was able to simplify the first patch to add the database table: https://review.opendev.org/c/openstack/cinder/+/918316

This is good, because smaller changes are easier to review :)

josephineSei commented 3 weeks ago

I looked through the failing tests in the database and the API patch, most of them seem to come from other problems. I made a few adjustments to the API patch to fix the failing ones, my patch was responsible for. I now start adding unit tests for the new API version, hopefully having some ready for the PTG this week

josephineSei commented 3 weeks ago

I am working on unit tests but can only find small amounts of time due to the PTG and other stuff.

josephineSei commented 2 weeks ago

We discussed a few points at the PTG and the Cinder teams promised me reviews. I already started implementing some feedback - like kicking out notifications. Another point is, that I somehow need to test the performance of the db calls, because, with many types and and request the db calls might have a bigger impact on performance. I need to look through this and whether I can test this on my devstack.

josephineSei commented 1 week ago

Back on working on the unit test and thinking about the db performance

josephineSei commented 1 week ago

I marked the spec as active and need to investigate a bit more in unit tests, the first test seems to run on my machine: https://review.opendev.org/c/openstack/cinder/+/928794

josephineSei commented 1 week ago

I stumbled upon a functional unit test, that has a very strange behavior:

When I execute the functional unit test to check the extensions I get an ERROR:

$ tox -efunctional-py39  -vv -- cinder.tests.functional.api_sample_tests.test_extensions.ExtensionsSampleJsonTest.test_extensions
....
....
==============================
Failed 1 tests - output below:
==============================

cinder.tests.functional.api_sample_tests.test_extensions.ExtensionsSampleJsonTest.test_extensions
-------------------------------------------------------------------------------------------------

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 394, in _verify_response
    response_result = self._compare_result(template_data,

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 280, in _compare_result
    matched_value = self._compare_dict(

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 183, in _compare_dict
    res = self._compare_result(expected[key], result[key],

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 284, in _compare_result
    matched_value = self._compare_list(

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 238, in _compare_list
    raise NoMatch('\n'.join(error))

    cinder.tests.functional.api_samples_test_base.NoMatch: Extra list items in Response:
{'alias': 'os-types-metadata', 'description': 'Type metadata support.', 'links': [], 'name': 'TypesMetadata', 'updated': '2024-09-09T00:00:00+00:00'}

When I try to add the new microversion in the extension-list-response sample, I get that Error:

==============================
Failed 1 tests - output below:
==============================

cinder.tests.functional.api_sample_tests.test_extensions.ExtensionsSampleJsonTest.test_extensions
-------------------------------------------------------------------------------------------------

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 416, in _verify_response
    self._compare_result(template_data, sample_data, "Sample")

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 280, in _compare_result
    matched_value = self._compare_dict(

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 183, in _compare_dict
    res = self._compare_result(expected[key], result[key],

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 284, in _compare_result
    matched_value = self._compare_list(

      File ".../cinder/cinder/tests/functional/api_samples_test_base.py", line 238, in _compare_list
    raise NoMatch('\n'.join(error))

    cinder.tests.functional.api_samples_test_base.NoMatch: Extra list items in template:
{'alias': 'os-types-metadata', 'description': 'Type metadata support.', 'links': [], 'name': 'TypesMetadata', 'updated': '%(extension_update)s'}

The failing part here is not in the original test file, but in the abstract test-base class, where three things are compared:

  1. The response from the actual call to the function
  2. The template I first adjusted
  3. a document in the api-ref

This is hard to detect, because the original test does not indicate the not only 2 things are compared, but in fact 3. This can only be concluded, when going through the base class.

So whenever there is a template to be adjusted, it may not be the only thing to adjust in Cinder so that your unit tests will be successful.

josephineSei commented 1 week ago

I am continuing working on unit tests and attended the Cinder weekly meeting to raise attention for reviews.

josephineSei commented 6 days ago

unit tests adjustments to reflect the new microversion is going into a good direction: https://review.opendev.org/c/openstack/cinder/+/928794

josephineSei commented 5 days ago

Only a few more unit tests to add here: https://review.opendev.org/c/openstack/cinder/+/928794/17/cinder/tests/unit/api/contrib/test_types_metadata.py

josephineSei commented 2 days ago

I finished unit tests, I think. I will take another look in the functional and maybe tempest tests.

josephineSei commented 1 day ago

After fixing some failures in the OpenStack zuuil pipeline, I started looking into creating tempest tests with a microversion. This leads to the question, whether tests should reside in tempest itself or rather in the tempest plugin from Cinder. For now I think it should be tempest itself, but it could be argued, that these are just API and database calls and thus should rather reside in the cinder tempest plugin. You can read here when tests should be part of tempest and when not.

josephineSei commented 11 hours ago

I again raised awareness for the patches in the cinder team meeting and also told them in the cinder channel, that we will not have that much time any more from december on to work on upstream items. Beside that I worked on the tempest tests.