Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rgw admin more actions #654

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

maksim-kharitonov
Copy link

@maksim-kharitonov maksim-kharitonov commented Feb 16, 2022

This change implements

  • listing of user's buckets (also with stats),
  • user credentials management,
  • individual bucket quota APIs
    of rgw/admin.

Related to #653

Signed-off-by: maxharitonov [email protected]

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Is this new API marked PREVIEW?

rgw/admin/bucket.go Outdated Show resolved Hide resolved
leseb
leseb previously requested changes Feb 22, 2022
rgw/admin/user_bucket_test.go Outdated Show resolved Hide resolved
rgw/admin/user_bucket.go Outdated Show resolved Hide resolved
rgw/admin/user_bucket.go Outdated Show resolved Hide resolved
rgw/admin/keys.go Outdated Show resolved Hide resolved
rgw/admin/keys.go Outdated Show resolved Hide resolved
rgw/admin/bucket_quota_test.go Show resolved Hide resolved
docs/api-status.md Outdated Show resolved Hide resolved
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Just minor comments, almost there, thanks!

rgw/admin/bucket.go Outdated Show resolved Hide resolved
rgw/admin/bucket_quota_test.go Show resolved Hide resolved
rgw/admin/bucket_quota.go Outdated Show resolved Hide resolved
rgw/admin/keys.go Outdated Show resolved Hide resolved
rgw/admin/keys.go Outdated Show resolved Hide resolved
@mergify
Copy link

mergify bot commented Mar 14, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@sebastianriese
Copy link
Contributor

Sorry for creating conflicts with your PR by having my changes merged. I hope I can be helpful by giving hints on what must be done to get this code running again when rebasing it atop master which now includes #644.

The valueToURLParams function now requires a second parameter of type []string that lists the allowed parameters for the API calls. The allowed values are not fully documented in the API docs, so the best way to extract the full list for each API call is to consult the ceph-rgw source code. The relevant files are the ones called rgw_rest_*.cc in the path https://github.com/ceph/ceph/tree/master/src/rgw. E.g. the following block parses the parameters for the bucket link operation: https://github.com/ceph/ceph/blob/master/src/rgw/rgw_rest_bucket.cc#L142 – and you can read off which parameters are interpreted by the ceph rgw admin ops API.

The convention now is not to include parameters in the allowed parameter list, that we don't currently support in our data structures, but to document this in a comment like here: https://github.com/ceph/go-ceph/pull/644/files#diff-694c344990ad130009eec2ff3c457832c960d480dfee9d1b675a2ea6138c5990R120.

@phlogistonjohn
Copy link
Collaborator

Hello there, if you wish us to revisit this PR please manually resolve the conflicts. Also, a new version has been released so the versions in docs will need updating. We're looking to simplify this process but at the moment it still requires manual fixing.

@maksim-kharitonov
Copy link
Author

Hello there, if you wish us to revisit this PR please manually resolve the conflicts. Also, a new version has been released so the versions in docs will need updating. We're looking to simplify this process but at the moment it still requires manual fixing.

Thank you for your assistance. Sorry for the delay in fixing, I'll try to come back with edits asap

@phlogistonjohn
Copy link
Collaborator

Hi @maksim-kharitonov - Just doing a ping in case you are still planning on working on this. I'd suggest that if you don't think you'll get to it in the next month or so, we can close this PR - and you can always reopen it in the future when you are ready.
Having it closed helps us maintainers not have to do things like pester you with messages like these and it keeps our list of things to look at smaller.
Thanks!

@maksim-kharitonov maksim-kharitonov force-pushed the rgw-admin-more-actions branch from 4fd08d9 to bab060a Compare July 5, 2022 15:15
@maksim-kharitonov
Copy link
Author

Hi @maksim-kharitonov - Just doing a ping in case you are still planning on working on this. I'd suggest that if you don't think you'll get to it in the next month or so, we can close this PR - and you can always reopen it in the future when you are ready. Having it closed helps us maintainers not have to do things like pester you with messages like these and it keeps our list of things to look at smaller. Thanks!

Thank you for your patience, just resolved conflicts and fixed. Can you please re-check pr?

@maksim-kharitonov maksim-kharitonov requested a review from leseb July 6, 2022 08:21
@phlogistonjohn
Copy link
Collaborator

Thanks for working on this again! I'm pinging our rgw admin experts @leseb and @thotz to take a look at the current version.
In the meantime there's a minor style issue detected by the check rule in our CI that needs to be fixed. Thanks!

rgw/admin/bucket_quota.go Show resolved Hide resolved
rgw/admin/bucket_quota.go Outdated Show resolved Hide resolved
rgw/admin/bucket_quota.go Show resolved Hide resolved
rgw/admin/keys.go Outdated Show resolved Hide resolved
rgw/admin/keys.go Show resolved Hide resolved
rgw/admin/keys.go Show resolved Hide resolved
rgw/admin/user_bucket.go Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator

FYI, things have changed a little around preview api documentation since you first filed the PR. The "PREVIEW" term in the comment is no longer needed. Instead, preview status is entirely derived from the build tag. Please remove the "PREVIEW" lines from the comments when you have a moment.

Copy link
Collaborator

@thotz thotz left a comment

Choose a reason for hiding this comment

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

Almost there need to update the test case for listbucketstat, apart from that PR LGTM

rgw/admin/user_bucket_test.go Show resolved Hide resolved
@anoopcs9 anoopcs9 added the API This PR includes a change to the public API of a go-ceph package label Jul 26, 2022
@maksim-kharitonov maksim-kharitonov requested a review from thotz July 26, 2022 14:09
thotz
thotz previously approved these changes Jul 27, 2022
@anoopcs9
Copy link
Collaborator

@maksim-kharitonov

It looks better if you can squash various commits in situations like:

  • Addressing review comments
  • Fixing style issues
  • Adding new APIs via individual files (just one or two commits adding particular set of APIs and tests)

Let me know if you need more clarification.

@mergify mergify bot dismissed thotz’s stale review July 27, 2022 09:20

Pull request has been modified.

@maksim-kharitonov
Copy link
Author

@maksim-kharitonov

It looks better if you can squash various commits in situations like:

  • Addressing review comments
  • Fixing style issues
  • Adding new APIs via individual files (just one or two commits adding particular set of APIs and tests)

Let me know if you need more clarification.

@anoopcs9 rebased my pr to new master and squashed to single commit

@anoopcs9
Copy link
Collaborator

@maksim-kharitonov
It looks better if you can squash various commits in situations like:

  • Addressing review comments
  • Fixing style issues
  • Adding new APIs via individual files (just one or two commits adding particular set of APIs and tests)

Let me know if you need more clarification.

@anoopcs9 rebased my pr to new master and squashed to single commit

I'm sorry if I was not clear earlier. I didn't mean to squash everything into one large commit rather have meaningful and related changes included as fewer individual commits like:

  • bucket_quota API addition and tests(with related changes to existing sources)
  • user_bucket API addition and tests(with related changes to existing sources)
  • keys API addition and tests(with related changes to existing sources)
  • docs change for api-status

Also please note that ceph_preview build tags are still missing from newly added bucket_quota_test.go, keys_test.go and user_bucket_test.go sources.

And finally changes to bucket.go have rejected hunks when applied on current master. Please rebase and resolve correctly on latest master.

@maksim-kharitonov maksim-kharitonov force-pushed the rgw-admin-more-actions branch 2 times, most recently from f0fa500 to 83a6679 Compare July 27, 2022 11:02
@maksim-kharitonov
Copy link
Author

@maksim-kharitonov
It looks better if you can squash various commits in situations like:

  • Addressing review comments
  • Fixing style issues
  • Adding new APIs via individual files (just one or two commits adding particular set of APIs and tests)

Let me know if you need more clarification.

@anoopcs9 rebased my pr to new master and squashed to single commit

I'm sorry if I was not clear earlier. I didn't mean to squash everything into one large commit rather have meaningful and related changes included as fewer individual commits like:

  • bucket_quota API addition and tests(with related changes to existing sources)
  • user_bucket API addition and tests(with related changes to existing sources)
  • keys API addition and tests(with related changes to existing sources)
  • docs change for api-status

Also please note that ceph_preview build tags are still missing from newly added bucket_quota_test.go, keys_test.go and user_bucket_test.go sources.

And finally changes to bucket.go have rejected hunks when applied on current master. Please rebase and resolve correctly on latest master.

  1. splitted into commits with more explicit meaning
  2. added tags to test-files
  3. rebased to the latest master

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Almost there...please see my comment.

docs/api-status.md Outdated Show resolved Hide resolved
@anoopcs9
Copy link
Collaborator

Just noticed that commit messages are not proper(not visible in a single line). Can we have that also corrected please? Sorry for not pointing it out earlier.

rgw/admin: Individual bucket quota support
rgw/admin: Key create and delete api support
rgw/admin: List user's bucket support
docs: Update API status

@maksim-kharitonov
Copy link
Author

Just noticed that commit messages are not proper(not visible in a single line). Can we have that also corrected please? Sorry for not pointing it out earlier.

rgw/admin: Individual bucket quota support
rgw/admin: Key create and delete api support
rgw/admin: List user's bucket support
docs: Update API status

fixed

@anoopcs9 anoopcs9 requested a review from phlogistonjohn July 29, 2022 10:20
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

The API json (and doc) are not up to date. You can either manually fix both files or you can try resetting the patch and re-running make api-update.

docs/api-status.json Outdated Show resolved Hide resolved
@phlogistonjohn
Copy link
Collaborator

Please note that the CI is experiencing issues do to a problem in the most recent ceph quincy hotfix release. If it's not fixed by ceph upstream soon, I plan to apply a workaround. Thanks for your patience.

@anoopcs9 anoopcs9 linked an issue Jul 30, 2022 that may be closed by this pull request
@anoopcs9
Copy link
Collaborator

Please note that the CI is experiencing issues do to a problem in the most recent ceph quincy hotfix release. If it's not fixed by ceph upstream soon, I plan to apply a workaround. Thanks for your patience.

This is now fixed with latest Quincy release v17.2.3

@maksim-kharitonov If possible can you also verify outstanding unresolved conversations and take action accordingly(mark as resolved)?

@anoopcs9 anoopcs9 requested a review from thotz July 30, 2022 14:32
@anoopcs9 anoopcs9 added the extended-review A submitter or reviewer feels the PR needs an extended review period label Jul 30, 2022
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for following up on the annoying managerial tasks.

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Aug 1, 2022

@Mergifyio rebase

@anoopcs9 anoopcs9 removed the extended-review A submitter or reviewer feels the PR needs an extended review period label Aug 1, 2022
@mergify
Copy link

mergify bot commented Aug 1, 2022

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the rgw-admin-more-actions branch from db4c397 to 9460417 Compare August 1, 2022 15:37
@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Aug 1, 2022

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the rgw-admin-more-actions branch from 9460417 to f1ce4e6 Compare August 1, 2022 17:06
@mergify mergify bot merged commit 5b42837 into ceph:master Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rgw: support more rgw admin API methods
6 participants