openstack/db: add Trove datastore admin and backup APIs#3767
Conversation
Signed-off-by: stias <seokjun.yang@mycraft.kr>
|
@mandre When you have some time, I would appreciate it if you could review the code. |
winiciusallan
left a comment
There was a problem hiding this comment.
hi @stias, thanks for your contribution! There are a plenty of changes, but for now I left a couple of comments regarding the db/v1/configurations package.
Will take some more time to take a look at the other changes. :)
| // The minimum value for integer parameters. | ||
| MinSize *int | ||
| // The maximum value for integer parameters. | ||
| MaxSize *int |
There was a problem hiding this comment.
Is 0 a valid value for this API? To be honest, I'm not too familiar with Trove API, and the model file in the source code doesn't tell me about it.
I'm asking because this is important to determine if we will use a pointer or not for these fields.
There was a problem hiding this comment.
I checked the Trove API behavior, and I think keeping MinSize/MaxSize as *int is the right approach.
The API ref sample includes a configuration parameter whose min value is 0, so 0 should be treated as a valid value rather than as not provided:
https://github.com/openstack/trove/blob/master/api-ref/source/samples/datastore-version-parameter-list-response.json#L3-L7
The create/update request samples also model min_size/max_size as explicit request fields:
https://github.com/openstack/trove/blob/master/api-ref/source/samples/datastore-version-parameter-create-request.json#L3-L7
https://github.com/openstack/trove/blob/master/api-ref/source/samples/datastore-version-parameter-update-request.json#L3-L7
Trove’s validation logic distinguishes unset min/max from actual values by checking whether min_size/max_size is None before applying integer validation:
https://github.com/openstack/trove/blob/05f5554c9ad073142cb22888645ff800caa90ed7/trove/configuration/service.py#L311-L343
The model also stores max_size/min_size as configuration parameter fields:
https://github.com/openstack/trove/blob/c40a2d54de13d73630af9155f89c0cee3f0938c7/trove/configuration/models.py#L261-L269
Because of that, using *int lets us preserve the distinction between:
- nil: omit the field
- pointer to 0: send "min_size": 0 or "max_size": 0
I also added a test to cover this case:
- openstack/db/v1/configurations/testing/requests_test.go
- openstack/db/v1/configurations/testing/fixtures_test.go
|
coverage: 64.114% (+0.1%) from 64.008% — stias:feature/trove-datastore-backup-apis into gophercloud:main |
Signed-off-by: stias <seokjun.yang@mycraft.kr>
|
Hi @stias, sorry for the late response. Before iterating again over your PR, I would like to keep you aware of two things. Since your PR implements a few API suites (datastore, config, backup, ...), this may take a little while to review all the changes and get it merged. I'm not sure about your plans, but if you need this to be merged soon, we recommend that you break it in separated PRs. If you don't have such a time constraint, you can keep it as it is and we will keep reviewing it. Just let us know. The other thing is we have a gap in our test matrix. Unfortunately, we don't have CI jobs for the Trove service. If you want to address this in the future (or before merging this feature), feel free to create a new PR. We will be happy to help you. I've filled a bug to track this work. |
|
Thank you for your response. As you suggested, I'll split the PR by API area, as that should help with review speed and clarity. I'll mention you again once it’s done. Thanks |
|
@stias Sorry, I forgot to mention that you can use the documentation as a reference. There you can find useful tips, but feel free to ask. |
|
@winiciusallan I split this combined PR by API area as discussed:
I also created #3809 as the tracking issue so each split PR can reference it with the source links required by the contribution guide. I'll close this combined PR to avoid duplicate review. |
Hi @stias, thanks for hard working on splitting the PRs. I'll start reviewing them tomorrow and help you to merge them soon. I have old comments in this PR that I'll leave in the confguration API, thus you already have some feedback 😄. |
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
Trove datastore version and datastore version configuration parameter management routes:
https://opendev.org/openstack/trove/src/branch/master/trove/extensions/routes/mgmt.py#L68-L79
Trove API reference: create/update/delete datastore version configuration parameters:
https://docs.openstack.org/api-ref/database/#create-datastore-version-configuration-parameters
https://docs.openstack.org/api-ref/database/#update-a-datastore-version-configuration-parameter
https://docs.openstack.org/api-ref/database/#delete-a-datastore-version-configuration-parameter
Trove API reference: create/list/show/update/delete datastore versions:
https://docs.openstack.org/api-ref/database/#create-datastore-version
https://docs.openstack.org/api-ref/database/#list-datastore-versions
https://docs.openstack.org/api-ref/database/#show-datastore-version-details
https://docs.openstack.org/api-ref/database/#update-datastore-version-details
https://docs.openstack.org/api-ref/database/#delete-datastore-version
Trove API reference: list/create/show/delete database backups:
https://docs.openstack.org/api-ref/database/#list-database-backups
https://docs.openstack.org/api-ref/database/#create-database-backup
https://docs.openstack.org/api-ref/database/#show-database-backup-details
https://docs.openstack.org/api-ref/database/#delete-database-backup
Trove API reference: list/create/delete backup strategies:
https://docs.openstack.org/api-ref/database/#list-backup-strategies
https://docs.openstack.org/api-ref/database/#create-backup-strategy
https://docs.openstack.org/api-ref/database/#delete-database-strategy