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: add creation time to bucket #937

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

peterwillis
Copy link
Contributor

The API endpoint for bucket stats includes a creation_time field which is not included in the admin.Bucket struct. This simply adds the field.

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 phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Dec 6, 2023
Copy link

github-actions bot commented Feb 5, 2024

This Pull Request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remember, a closed PR can always be reopened. Thank you for your contribution.

@github-actions github-actions bot added Stale and removed Stale labels Feb 5, 2024
@ansiwen
Copy link
Collaborator

ansiwen commented Feb 13, 2024

@Mergifyio rebase

Copy link

mergify bot commented Feb 13, 2024

rebase

✅ Branch has been successfully rebased

@peterwillis
Copy link
Contributor Author

I have added a test, but I am not sure it is the right place to add a test for this. It tests that the creation date was within a reasonable duration as I couldn't see a way to make the test server return a known date. It might be better to test the other end. I also made a test with a mock response similar to what is going on in TestUnmarshal, but that would add some burden in the future when the response is changed. Happy to add that instead or aswell?

Copy link

github-actions bot commented May 1, 2024

This Pull Request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remember, a closed PR can always be reopened. Thank you for your contribution.

@github-actions github-actions bot added the Stale label May 1, 2024
@ansiwen
Copy link
Collaborator

ansiwen commented May 7, 2024

I have added a test, but I am not sure it is the right place to add a test for this. It tests that the creation date was within a reasonable duration as I couldn't see a way to make the test server return a known date. It might be better to test the other end. I also made a test with a mock response similar to what is going on in TestUnmarshal, but that would add some burden in the future when the response is changed. Happy to add that instead or aswell?

@phlogistonjohn since you asked for the unit test, you want to take a final look?

@ansiwen
Copy link
Collaborator

ansiwen commented May 7, 2024

And @thotz, you are of course always very welcome to have a look at the rgw changes. ;-)

@ansiwen ansiwen removed the Stale label May 7, 2024
@phlogistonjohn
Copy link
Collaborator

Great, I am happy that there is a test. My lack of knowledge around specific of RGW make me hesitant to approve the PR though. It has my tacit approval now :-)

@anoopcs9 anoopcs9 requested a review from thotz May 17, 2024 09:04
@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

Copy link

mergify bot commented May 20, 2024

rebase

✅ Branch has been successfully rebased

@phlogistonjohn
Copy link
Collaborator

The nautilus test is now failing in the same location:

{"bucket":"test","num_shards":0,"tenant":"","zonegroup":"8fb5e482-7816-4c4a-bf48-c7c0f29bec50","placement_rule":"default-placement","explicit_placement":{"data_pool":"","data_extra_pool":"","index_pool":""},"id":"b872f21e-d86c-4c80-9d87-d6550b207fe0.4151.1","marker":"b872f21e-d86c-4c80-9d87-d6550b207fe0.4151.1","index_type":"Normal","owner":"admin","ver":"0#1","master_ver":"0#0","mtime":"2024-05-20 13:57:38.916311Z","max_marker":"0#","usage":{},"bucket_quota":{"enabled":false,"check_on_raw":false,"max_size":-1,"max_size_kb":0,"max_objects":-1}}
=== NAME  TestRadosGWTestSuite/TestBucket
    bucket_test.go:46: 
        	Error Trace:	/go/src/github.com/ceph/go-ceph/rgw/admin/bucket_test.go:46
        	Error:      	Expected value not to be nil.
        	Test:       	TestRadosGWTestSuite/TestBucket
--- FAIL: TestRadosGWTestSuite (1.56s)
    --- FAIL: TestRadosGWTestSuite/TestBucket (1.56s)
        --- PASS: TestRadosGWTestSuite/TestBucket/list_buckets (0.05s)
        --- PASS: TestRadosGWTestSuite/TestBucket/info_non-existing_bucket (0.04s)
        --- PASS: TestRadosGWTestSuite/TestBucket/info_existing_bucket (0.04s)
        --- FAIL: TestRadosGWTestSuite/TestBucket/existing_bucket_has_valid_creation_date (0.04s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9fee34]

goroutine 41 [running]:
testing.tRunner.func1.2({0xa87fc0, 0x13f2930})
	/opt/go/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/opt/go/src/testing/testing.go:1634 +0x377
panic({0xa87fc0?, 0x13f2930?})
	/opt/go/src/runtime/panic.go:770 +0x132
github.com/ceph/go-ceph/rgw/admin.(*RadosGWTestSuite).TestBucket.func4(0xc000550340?)
	/go/src/github.com/ceph/go-ceph/rgw/admin/bucket_test.go:47 +0x1d4
testing.tRunner(0xc000550340, 0xc00053af30)
	/opt/go/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 21
	/opt/go/src/testing/testing.go:1742 +0x390
FAIL	github.com/ceph/go-ceph/rgw/admin	1.569s
FAIL

I suspect this may be a real regression. Someone please look into this. Thanks!

@anoopcs9
Copy link
Collaborator

The nautilus test is now failing in the same location:

{"bucket":"test","num_shards":0,"tenant":"","zonegroup":"8fb5e482-7816-4c4a-bf48-c7c0f29bec50","placement_rule":"default-placement","explicit_placement":{"data_pool":"","data_extra_pool":"","index_pool":""},"id":"b872f21e-d86c-4c80-9d87-d6550b207fe0.4151.1","marker":"b872f21e-d86c-4c80-9d87-d6550b207fe0.4151.1","index_type":"Normal","owner":"admin","ver":"0#1","master_ver":"0#0","mtime":"2024-05-20 13:57:38.916311Z","max_marker":"0#","usage":{},"bucket_quota":{"enabled":false,"check_on_raw":false,"max_size":-1,"max_size_kb":0,"max_objects":-1}}
=== NAME  TestRadosGWTestSuite/TestBucket
    bucket_test.go:46: 
        	Error Trace:	/go/src/github.com/ceph/go-ceph/rgw/admin/bucket_test.go:46
        	Error:      	Expected value not to be nil.
        	Test:       	TestRadosGWTestSuite/TestBucket
--- FAIL: TestRadosGWTestSuite (1.56s)
    --- FAIL: TestRadosGWTestSuite/TestBucket (1.56s)
        --- PASS: TestRadosGWTestSuite/TestBucket/list_buckets (0.05s)
        --- PASS: TestRadosGWTestSuite/TestBucket/info_non-existing_bucket (0.04s)
        --- PASS: TestRadosGWTestSuite/TestBucket/info_existing_bucket (0.04s)
        --- FAIL: TestRadosGWTestSuite/TestBucket/existing_bucket_has_valid_creation_date (0.04s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9fee34]

goroutine 41 [running]:
testing.tRunner.func1.2({0xa87fc0, 0x13f2930})
	/opt/go/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/opt/go/src/testing/testing.go:1634 +0x377
panic({0xa87fc0?, 0x13f2930?})
	/opt/go/src/runtime/panic.go:770 +0x132
github.com/ceph/go-ceph/rgw/admin.(*RadosGWTestSuite).TestBucket.func4(0xc000550340?)
	/go/src/github.com/ceph/go-ceph/rgw/admin/bucket_test.go:47 +0x1d4
testing.tRunner(0xc000550340, 0xc00053af30)
	/opt/go/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 21
	/opt/go/src/testing/testing.go:1742 +0x390
FAIL	github.com/ceph/go-ceph/rgw/admin	1.569s
FAIL

@thotz Can it be the case that creation_time is not yet part of bucket info with nautilus?

@ansiwen
Copy link
Collaborator

ansiwen commented Jul 11, 2024

@Mergifyio rebase

Copy link

mergify bot commented Jul 11, 2024

rebase

✅ Branch has been successfully rebased

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.

The nautilus test is now failing in the same location:

With nautilus removed from the CI we could consider this change to add creation time to the bucket info structure.

rgw/admin/bucket.go Show resolved Hide resolved
@mergify mergify bot merged commit 9531dd6 into ceph:master Jul 22, 2024
15 of 16 checks passed
@anoopcs9
Copy link
Collaborator

- and:
  - label=API
  - "#approved-reviews-by>=1"
  - "updated-at<10 days ago"

kicked in 😀 .

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.

5 participants