Skip to content

Blockstorage extensions limits#2084

Merged
jtopjian merged 6 commits intogophercloud:masterfrom
TenSt:blockstorage-extensions-limits
Jan 5, 2021
Merged

Blockstorage extensions limits#2084
jtopjian merged 6 commits intogophercloud:masterfrom
TenSt:blockstorage-extensions-limits

Conversation

@TenSt
Copy link
Copy Markdown
Contributor

@TenSt TenSt commented Dec 28, 2020

For #1702

All links are already included in the issue.

Notes on Limits struct:
The original code contains fields only for max values, but API docs states that there are also total values. I've tested out on OpenStack Ussuri and I see that API docs are valid. I'm not very good at python, so might've missed where exactly total fields are added to the response.
Original code: https://github.com/openstack/cinder/blob/master/cinder/api/views/limits.py#L42-L48
API docs: https://docs.openstack.org/api-ref/block-storage/v3/index.html?#show-absolute-limits-for-project

Also, I've left "rate" as interface because it is not clear from the code what will return there and docs example is just an empty array (as well as response from real OpenStack).

Note on acceptance test: the hard-coded values are default ones, so should work for any OpenStack Project with default values.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 29, 2020

Coverage Status

Coverage increased (+0.01%) to 79.755% when pulling 1411c2b on TenSt:blockstorage-extensions-limits into 810f391 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 29, 2020

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

@TenSt Thank you for working on this.

The OpenLab failure is from the acceptance test you've added. You can see the log here: https://logs.openlabtesting.org/logs/84/2084/1f255e0aca45b3545996a6691c5c93121b8e213e/check/gophercloud-acceptance-test/6eee85a/job-output.txt.gz and search for the string --- FAIL.

The cause of the error looks to be from the backup created in the previous test not deleting in time for the new test to run. I recommend changing the assertion to compare against snapshots, maybe.

Also, I've left "rate" as interface because it is not clear from the code what will return there and docs example is just an empty array (as well as response from real OpenStack).

The code that builds the rate can be found here: https://github.com/openstack/cinder/blob/master/cinder/api/views/limits.py#L56-L91

It looks like there's a unit test within Cinder that details how the data structure should look: https://github.com/openstack/cinder/blob/84a084d1bdc88c649c1f3b728beb36dd0707dc22/cinder/tests/unit/api/v2/test_limits.py#L119-L156

You can use this code for your own test fixture, which should allow you to create a detailed struct for the rate.

Please let me know if you have any questions.

@TenSt
Copy link
Copy Markdown
Contributor Author

TenSt commented Jan 3, 2021

@jtopjian Thank you for the review and pointing out where to find rate object.

Rate part of Limits:
Added rate struct and updated unit tests accordingly.

Acceptance tests:
Updated acceptance tests to verify that current values greater or equal than 0 and lesser or equal than the max value. This way the test is more flexible to dynamic values of current usage of blockstorage in the project.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 3, 2021

Build failed.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 4, 2021

Build succeeded.

@TenSt
Copy link
Copy Markdown
Contributor Author

TenSt commented Jan 4, 2021

@jtopjian I've fixed acceptance tests, so PR is ready for the next review round.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 4, 2021

Build succeeded.

Copy link
Copy Markdown
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you!

@jtopjian jtopjian merged commit 017c152 into gophercloud:master Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants