Add support for blockstorage volume_type quota by nikParasyr · Pull Request #2109 · gophercloud/gophercloud

nikParasyr · 2021-02-01T18:06:13Z

For #2072

Api docs:

https://docs.openstack.org/api-ref/block-storage/v3/?expanded=update-quotas-for-a-project-detail#quota-sets-extension-os-quota-sets

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

coveralls · 2021-02-01T18:16:55Z

Coverage decreased (-0.008%) to 79.831% when pulling 41a444f on nikParasyr:volume_type_quota into 633d735 on gophercloud:master.

theopenlab-ci · 2021-02-01T19:04:16Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 38s
gophercloud-acceptance-test : POST_FAILURE in 49m 03s
gophercloud-acceptance-test-ironic : POST_FAILURE in 51m 23s

jtopjian · 2021-02-01T20:26:27Z

recheck

theopenlab-ci · 2021-02-01T22:14:41Z

Build failed.

gophercloud-unittest : SUCCESS in 5m 14s
gophercloud-acceptance-test : FAILURE in 1h 47m 09s
gophercloud-acceptance-test-ironic : SUCCESS in 47m 25s

jtopjian · 2021-02-02T04:04:55Z

@nikParasyr Thanks for working on this. I haven't looked it over since it's still a draft, but wanted to mention that the acceptance test failure is an actual failure. You can see details about the failure by clicking on the gophercloud-acceptance-test link above, and then going to the job-output.txt.gz file and searching for:

--- FAIL: TestQuotasetUpdate

Let me know if you have any questions or need any help. One common issue seen is that people will test on their own local OpenStack deployment which might act differently than a "vanilla" devstack environment. If this is your situation, it might be helpful to launch a vm, install devstack and go and work on the test from within that environment.

nikParasyr · 2021-02-02T08:36:09Z

Hello @jtopjian. Thank you for your input. I'd like some more input and help if possible. The acceptance test fails for 2 reasons. I was aware of both of them from running it locally but i wanted to see the openlab result as well as get an opinion.

Currently I have updated TestQuotasetUpdate to create a volume type foo in order to test that the patch works as intended. That isn't rly workable though because Extra is populated by volumes_/snapshots_/gigabytes_<volume_type_name> for every volumeType that exists on the testing cloud. So it doesn't only contain for the foo volume type but also for all existing volumeTypes. The volumeTypes differ from cloud to cloud. So hardcoding the expected Extra map won't work. Therefore to be able to test this properly we probably need to create Extra dynamically based on the existing volumeTypes found. That will sort of increase the complexity of the acceptance test but it's the only way i can think of to make it run properly on any testing cloud. Let me know if i should work towards that direction.
Secondly, even when i hardcoded the Extra based on my testing cloud volume types I still get the following error which I'm a bit unsure how to handle. Same error occurs on openlab. It's a type error. Even more "weird" is that between runs I get different results. One run only gigabytes_foo is complaining, then all of them, then only volumes_foo etc. I'm probably missing something very basic here...

	=== RUN   TestQuotasetUpdate
	TestQuotasetUpdate: convenience.go:236: Failure in quotaset_test.go, line 122:at .Extra[gigabytes_foo] expected 100, but got 100
	TestQuotasetUpdate: convenience.go:236: Failure in quotaset_test.go, line 122:at .Extra[volumes_foo] expected 10, but got 10
	TestQuotasetUpdate: convenience.go:236: Failure in quotaset_test.go, line 122:at .Extra[snapshots_foo] expected 10, but got 10
	TestQuotasetUpdate: convenience.go:236: Failure in quotaset_test.go, line 122:at .Extra expected <nil>, but got map[string]interface {}{"gigabytes_foo":100, "snapshots_foo":10, "volumes_foo":10}
	TestQuotasetUpdate: convenience.go:35: Failure in quotaset_test.go, line 122: The structures were different.
	--- FAIL: TestQuotasetUpdate (0.88s)
	FAIL

jtopjian · 2021-02-03T02:03:52Z

Currently I have updated TestQuotasetUpdate to create a volume type foo in order to test that the patch works as intended. That isn't rly workable though because Extra is populated by volumes_/snapshots_/gigabytes_<volume_type_name> for every volumeType that exists on the testing cloud.

This is OK. Take a look at the other acceptance tests for different examples. A lot of them contain a pattern that defines a variable called found and then only checks for a specific resource, ignoring other resources. This is to account for existing resources in the cloud.

Secondly, even when i hardcoded the Extra based on my testing cloud volume types I still get the following error which I'm a bit unsure how to handle.

At first glance, this looks like some kind of type assertion issue. I would double-check the type that is being marshaled into "Extra" and make sure it's being compared to a type of the same kind.

I hope this helps, but let me know if you need more details.

nikParasyr · 2021-02-03T12:42:24Z

@jtopjian Thanks for all the help. This is ready for a first review.
Tried to follow the contributing guide as much as possible but I might have missed some spots.

theopenlab-ci · 2021-02-03T13:31:54Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 43s
gophercloud-acceptance-test : FAILURE in 51m 55s
gophercloud-acceptance-test-ironic : FAILURE in 28m 27s

nikParasyr · 2021-02-03T13:32:57Z

recheck

theopenlab-ci · 2021-02-03T14:12:48Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 30s
gophercloud-acceptance-test : FAILURE in 38m 28s
gophercloud-acceptance-test-ironic : FAILURE in 28m 10s

nikParasyr · 2021-02-03T14:25:07Z

recheck

theopenlab-ci · 2021-02-03T15:19:55Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 59s
gophercloud-acceptance-test : FAILURE in 53m 27s
gophercloud-acceptance-test-ironic : FAILURE in 22m 23s

nikParasyr · 2021-02-03T16:01:56Z

recheck

theopenlab-ci · 2021-02-03T16:40:44Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 35s
gophercloud-acceptance-test : FAILURE in 37m 46s
gophercloud-acceptance-test-ironic : FAILURE in 28m 54s

jtopjian · 2021-02-03T16:49:41Z

@nikParasyr There might be an issue with OpenLab. I recommend trying again in a few hours. I'll try to get some time to review this either today or tomorrow.

nikParasyr · 2021-02-03T20:20:31Z

recheck

theopenlab-ci · 2021-02-03T21:47:12Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 33s
gophercloud-acceptance-test : FAILURE in 1h 20m 00s
gophercloud-acceptance-test-ironic : FAILURE in 17m 46s

jtopjian · 2021-02-04T03:43:43Z

openstack/blockstorage/extensions/quotasets/results.go

+
+	// Collect other fields and bundle them into Extra
+	// but only if a field titled "extra" wasn't sent.
+	if s.Extra != nil {


This is specific to the Keystone/Identity API and can be removed here. The body of the else clause should only be used.

jtopjian · 2021-02-04T03:43:54Z

recheck

theopenlab-ci · 2021-02-04T05:12:59Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 57s
gophercloud-acceptance-test : FAILURE in 1h 26m 36s
gophercloud-acceptance-test-ironic : FAILURE in 48m 42s

theopenlab-ci · 2021-02-04T08:43:26Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 49s
gophercloud-acceptance-test : FAILURE in 1h 25m 06s
gophercloud-acceptance-test-ironic : FAILURE in 47m 58s

nikParasyr · 2021-02-04T22:58:37Z

hmmmm openlab is consistently failing. maybe this is related? not the occm stuff but openlab upgrading to focal? ill stop rechecking for the time being

jtopjian · 2021-02-05T03:06:43Z

Yes, it's possible. I've subscribed to the issue and will keep an eye on it.

In the meantime, I have run the tests in my own devstack environment and everything looks good to me. If you're finished with this, I can go ahead and merge it.

nikParasyr · 2021-02-05T07:19:12Z

Yes. I've finished with this one. Thank you for all the help :)

jtopjian

LGTM - thank you!

jtopjian · 2021-02-05T22:02:29Z

@nikParasyr Thank you for submitting this - this was really good work.

nikParasyr force-pushed the volume_type_quota branch from 51890c1 to 46d6446 Compare February 3, 2021 12:38

nikParasyr marked this pull request as ready for review February 3, 2021 12:40

jtopjian reviewed Feb 4, 2021

View reviewed changes

Add support for blockstorage volume_type quota

41a444f

nikParasyr force-pushed the volume_type_quota branch from 46d6446 to 41a444f Compare February 4, 2021 07:15

jtopjian approved these changes Feb 5, 2021

View reviewed changes

jtopjian merged commit 281f27b into gophercloud:master Feb 5, 2021

jtopjian mentioned this pull request Feb 5, 2021

Add support for volume_type blockstorage quota #2072

Closed

nikParasyr deleted the volume_type_quota branch February 8, 2021 07:28

This was referenced Feb 24, 2021

blockstorage_quotaset_v3: Add volume type quota support terraform-provider-openstack/terraform-provider-openstack#1185

Merged

blockstorage_quotaset_v2: Add volume type quota support terraform-provider-openstack/terraform-provider-openstack#1187

Merged

Conversation

nikParasyr commented Feb 1, 2021

Uh oh!

coveralls commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Feb 1, 2021

Uh oh!

jtopjian commented Feb 1, 2021

Uh oh!

theopenlab-ci bot commented Feb 1, 2021

Uh oh!

jtopjian commented Feb 2, 2021

Uh oh!

nikParasyr commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Feb 3, 2021

Uh oh!

nikParasyr commented Feb 3, 2021

Uh oh!

theopenlab-ci bot commented Feb 3, 2021

Uh oh!

nikParasyr commented Feb 3, 2021

Uh oh!

theopenlab-ci bot commented Feb 3, 2021

Uh oh!

nikParasyr commented Feb 3, 2021

Uh oh!

theopenlab-ci bot commented Feb 3, 2021

Uh oh!

nikParasyr commented Feb 3, 2021

Uh oh!

theopenlab-ci bot commented Feb 3, 2021

Uh oh!

jtopjian commented Feb 3, 2021

Uh oh!

nikParasyr commented Feb 3, 2021

Uh oh!

theopenlab-ci bot commented Feb 3, 2021

Uh oh!

jtopjian Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

nikParasyr Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

jtopjian commented Feb 4, 2021

Uh oh!

theopenlab-ci bot commented Feb 4, 2021

Uh oh!

theopenlab-ci bot commented Feb 4, 2021

Uh oh!

nikParasyr commented Feb 4, 2021

Uh oh!

jtopjian commented Feb 5, 2021

Uh oh!

nikParasyr commented Feb 5, 2021

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

jtopjian commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Feb 1, 2021 •

edited

Loading

nikParasyr commented Feb 2, 2021 •

edited

Loading