Skip to content

rgw/admin: support check-bucket-index #1098

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.

Sign up for GitHub

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

Jump to bottom
Merged
mergify merged 2 commits into from
Apr 15, 2025
Merged

rgw/admin: support check-bucket-index #1098

mergify merged 2 commits into from
Apr 15, 2025

Conversation

@sj14
Copy link
Contributor

@sj14 sj14 commented Apr 3, 2025 •
edited
Loading

Add the RadosGW Check Bucket Index call.
I'm not quite sure if my implementation is correct, the returned status is always an empty string.

Sample output from radosgw-admin bucket check --bucket <BUCKET> --fix --format=json:

{
    "invalid_multipart_entries": [],
    "check_result": {
        "existing_header": {
            "usage": {
                "rgw.main": {
                    "size": 151095539,
                    "size_actual": 151097344,
                    "size_utilized": 151095539,
                    "size_kb": 147555,
                    "size_kb_actual": 147556,
                    "size_kb_utilized": 147555,
                    "num_objects": 1
                },
                "rgw.multimeta": {
                    "size": 0,
                    "size_actual": 0,
                    "size_utilized": 0,
                    "size_kb": 0,
                    "size_kb_actual": 0,
                    "size_kb_utilized": 0,
                    "num_objects": 0
                }
            }
        },
        "calculated_header": {
            "usage": {
                "rgw.main": {
                    "size": 151095539,
                    "size_actual": 151097344,
                    "size_utilized": 151095539,
                    "size_kb": 147555,
                    "size_kb_actual": 147556,
                    "size_kb_utilized": 147555,
                    "num_objects": 1
                }
            }
        }
    }
}

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? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Apr 3, 2025

Ran make api-update to record new APIs --> I get found new unexpected stable apis: API.CheckBucketIndex how to fix this?

That's directly related the checkbox above. We have a policy of marking APIs preview for two releases. Preview apis are subject to change. So we ask users of go-ceph to opt-in to the preview apis by using a build tag.

You need to add lines to your new files marking them as preview. For example:

go-ceph/rados/rados_getaddrs.go

Line 1 in 8dac64f

//go:build ceph_preview

Also see https://github.com/ceph/go-ceph/blob/master/docs/development.md#documentation-conventions

@sj14 sj14 force-pushed the check-bucket-index branch from 54e84f2 to d35dfc7 Compare April 3, 2025 14:00
@sj14
Copy link
Contributor Author

sj14 commented Apr 3, 2025

That's directly related the checkbox above. We have a policy of marking APIs preview for two releases. Preview apis are subject to change. So we ask users of go-ceph to opt-in to the preview apis by using a build tag.

Thank you very much. Somehow I have misinterpreted this is for new/unstable APIs in Ceph itself.

@sj14
Copy link
Contributor Author

sj14 commented Apr 7, 2025

From the CI logs, it looks like the response does not match the response entity from the docs.
I will have another look.

@sj14 sj14 force-pushed the check-bucket-index branch 3 times, most recently from 0dc6abe to 56fe0d7 Compare April 8, 2025 08:10
@sj14
Copy link
Contributor Author

sj14 commented Apr 8, 2025

The JSON returned from Ceph Octopus is different and fails to unmarshal, thus I'm skipping this version in the tests and added a warning that the method is not compatible with Octopus.

[]{}{"existing_header":{"usage":{}},"calculated_header":{"usage":{}}}

@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Apr 8, 2025

If the API doesn't work on octopus you should add a build tag, such as //go:build ceph_preview && !ceph_octopus to the new file. Don't just disable the test on octopus.

@sj14 sj14 force-pushed the check-bucket-index branch from 56fe0d7 to 0ceff3a Compare April 8, 2025 13:40
@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Apr 8, 2025
@sj14 sj14 force-pushed the check-bucket-index branch 2 times, most recently from 2cc6d10 to 9820b7d Compare April 9, 2025 06:30
@sj14
Copy link
Contributor Author

sj14 commented Apr 12, 2025

If the API doesn't work on octopus you should add a build tag, such as //go:build ceph_preview && !ceph_octopus to the new file. Don't just disable the test on octopus.

Adjusted.

phlogistonjohn
phlogistonjohn reviewed Apr 12, 2025
View reviewed changes
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.

Please seperate the typo correction in the implements README.md from the new api. Have them be two separate commits. Thanks!

@sj14 sj14 force-pushed the check-bucket-index branch from 9820b7d to 5cf1301 Compare April 14, 2025 07:30
@sj14
Copy link
Contributor Author

sj14 commented Apr 14, 2025

Please seperate the typo correction in the implements README.md from the new api. Have them be two separate commits. Thanks!

Adjusted.

@phlogistonjohn phlogistonjohn added the extended-review A submitter or reviewer feels the PR needs an extended review period label Apr 14, 2025
phlogistonjohn
phlogistonjohn approved these changes Apr 14, 2025
View reviewed changes
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.

based on my small knowledge of rgw, this looks OK to me.

@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Apr 14, 2025

Since this is RGW I want more eyes on this one.
@anoopcs9, @ansiwen, or @thotz PTAL when you have a moment. Thanks

ansiwen
ansiwen approved these changes Apr 15, 2025
View reviewed changes
Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

LGTM in general. I set do-no-merge to give @thotz still a chance to have a look if he's around.

@phlogistonjohn phlogistonjohn removed do-not-merge extended-review A submitter or reviewer feels the PR needs an extended review period labels Apr 15, 2025
@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Apr 15, 2025

@Mergifyio rebase

sj14 added 2 commits April 15, 2025 13:43
@sj14 @phlogistonjohn
contrib: fix typo in Make command
6ee5860 
Signed-off-by: Simon Jürgensmeyer <simon.juergensmeyer@hetzner-cloud.de>
@sj14 @phlogistonjohn
rgw/admin: support check-bucket-index
fd7ea90 
Signed-off-by: Simon Jürgensmeyer <simon.juergensmeyer@hetzner-cloud.de>
@mergify
Copy link

mergify bot commented Apr 15, 2025

rebase

✅ Branch has been successfully rebased

@mergify
Copy link

mergify bot commented Apr 15, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify mergify bot merged commit c987830 into ceph:master Apr 15, 2025
15 checks passed
@phlogistonjohn phlogistonjohn mentioned this pull request Apr 15, 2025
Merged
@sj14 sj14 deleted the check-bucket-index branch April 16, 2025 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ansiwen ansiwen ansiwen approved these changes

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

@thotz thotz Awaiting requested review from thotz

@anoopcs9 anoopcs9 Awaiting requested review from anoopcs9

Assignees

No one assigned

Labels

API This PR includes a change to the public API of a go-ceph package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sj14 @phlogistonjohn @ansiwen