List oids null by nickjanus · Pull Request #663 · ceph/go-ceph

nickjanus · 2022-04-05T18:10:32Z

👋 We've been using a patch that's similar to this in production for a while now, which has helped with our internal tooling to clean up broken MPUs created by RGW. Ideas welcome on how I could go about testing the changes to ListObjects. I don't know of a way to create an object with a null char in the key short of getting a new CLS function committed.

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?

phlogistonjohn

Thank you very much for your contribution! I have a few comments below. Overall, it looks like a nice improvement.

phlogistonjohn · 2022-04-05T19:51:49Z

rados/rados_test.go

-		"empty":         []byte(""),
+		"key1":             []byte("value1"),
+		"key2":             []byte("value2"),
+		"prefixed\x00key3": []byte("value3"),


I'd prefer to see the new key in addition to the existing key, thanks.

phlogistonjohn · 2022-04-05T20:02:43Z

rados/ioctx.go

-	ret := C.rados_nobjects_list_open(ioctx.ioctx, &ctx)
-	if ret < 0 {
-		return getError(ret)
+	page_results := C.size_t(1000)


Since this is a "magic" value. Let's make it a const at the file level. Name it something like defaultListObjectsResultSize.

phlogistonjohn · 2022-04-05T20:04:35Z

rados/ioctx.go

+
+	next := C.rados_object_list_begin(ioctx.ioctx)
+	if next == nil {
+		return errors.New("Pool does not exist")


It might be better to return ErrNotFound here.

phlogistonjohn · 2022-04-05T20:10:35Z

rados/ioctx.go

+			listFn(C.GoStringN(item.oid, (C.int)(item.oid_length)))
+		}
+
+		if ret := C.rados_object_list_is_end(ioctx.ioctx, next); int(ret) == 1 {


Again, it would make things clearer to have the "magic" value 1 as a real constant such as const listEndSentinel = C.int(1). Also, since ret isn't used elsewhere again there's no need to assign ret, just do:

if C.rados_object_list_is_end(...) == listEndSentinel { ... }

phlogistonjohn · 2022-04-05T20:15:16Z

Regarding the tests, are there no functions in go-ceph that currently allow you to create the object name with a null? I'm not clear what a "CLS function" is.

nickjanus · 2022-04-06T00:28:45Z

re testing: Sorry, I'm probably not using the correct terminology, I meant an OSD class method (https://docs.ceph.com/en/latest/rados/api/librados/#c.rados_exec). From the librados documentation, I don't see how to specify an oid with a length, so that it can include a null character. I had assumed that this was only possible using some osd class method, but maybe I'm missing something.

phlogistonjohn · 2022-04-06T13:49:46Z

re testing: Sorry, I'm probably not using the correct terminology, I meant an OSD class method (https://docs.ceph.com/en/latest/rados/api/librados/#c.rados_exec).

OK, thanks. I was vaguely aware of that method but don't know much about it.

From the librados documentation, I don't see how to specify an oid with a length, so that it can include a null character.

I just skimmed the list of function definitions and I agree, there's nothing obvious there to create a name with a null. Obviously, it must be possible since you're seeing them. However, it may be done at a layer "below" librados and therefore difficult to access via go-ceph.

I had assumed that this was only possible using some osd class method, but maybe I'm missing something.

Maybe, if you research it and find it is possible that way, I'd certainly be open to having something like rados_write_op_exec and/or rados_exec to the library. I am somewhat concerned that it may not do what you want as the inline docs say "The OSD has a plugin mechanism for performing complicated
operations on an object atomically" and const char *oid is in the function defintion. Both imply that the name of the object is supplied as a null-terminated string up front.

Another approach you might take is to ask the RGW developers how it was done (regardless of if it was indented).

Finally, while I'd like to have regression tests for everything, I'd be fine not having coverage for this case and still taking these patches. The existing test will cover the common use case. So don't worry about the patches being declined just for the lack of a test for the "extra nulls" case.

phlogistonjohn

It looks like you may have overlooked make check failing when you updated the patches based on my earlier feedback. New code should have typical golang naming standards (no underscores).

Additionally, there's still a "bare 1" rather than a listEndSentinel. Since this is a suggestion, not a requirement, feel free to say if you don't want to do that. At the moment though, I don't know if you forgot or chose not to do that.

nickjanus · 2022-04-06T18:50:37Z

It looks like you may have overlooked make check failing when you updated the patches based on my earlier feedback. New code should have typical golang naming standards (no underscores).

Additionally, there's still a "bare 1" rather than a listEndSentinel. Since this is a suggestion, not a requirement, feel free to say if you don't want to do that. At the moment though, I don't know if you forgot or chose not to do that.

I'll fix up the formatting, and that was an oversight on my part, so I'll replace with a non-magic value, thanks for the review! I should be able to push an updated branch this evening.

nickjanus · 2022-04-07T21:08:38Z

It looks like there a test failure when iterating over an omap with a iteratorSize of 1. Going to play around with the new null char key to see if that ordering makes a difference and debug from there.

@phlogistonjohn Do you think it would be valuable to have a make command that ran tests in a docker container with a given ceph version's packages installed?

phlogistonjohn · 2022-04-08T13:45:57Z

It looks like there a test failure when iterating over an omap with a iteratorSize of 1. Going to play around with the new null char key to see if that ordering makes a difference and debug from there.

Sounds reasonable.

@phlogistonjohn Do you think it would be valuable to have a make command that ran tests in a docker container with a given ceph version's packages installed?

I'm not sure what you mean by "a given ceph version's packages installed". If you just mean trying out different major version of ceph you should already be able to do that. Some examples:

make test-containers-test CEPH_VERSION=nautilus
make test-containers-clean # teardown

make test-containers-test CEPH_VERSION=octopus
make test-containers-clean # teardown

make test-containers-test CEPH_VERSION=pacific
make test-containers-clean # teardown

Everything the CI does should be doable on one's desktop/laptop.

You can narrow down the scope of what's being tested too. To run only a particular sub-package of go-ceph use ENTRYPOINT_ARGS=''--test-pkg=rados" for example: make test-containers-test CEPH_VERSION=octopus ENTRYPOINT_ARGS='--test-pkg=rados'

You can even make custom containers to use, but doing so is not direct or easy. If I'm misunderstanding your request, please feel free to correct me.

nickjanus · 2022-04-08T13:55:42Z

Ah nope, that's exactly what I was asking about, thanks! I hadn't looked at the existing Makefile closely enough.

phlogistonjohn · 2022-04-08T17:27:28Z

There's https://github.com/ceph/go-ceph/blob/master/docs/development.md#testing too. but I forgot to link to it earlier. Also don't hesitate to file issues if the above docs are wrong/incomplete or you simply want to request that we document something!

nickjanus · 2022-04-13T03:20:04Z

Looks like the test failure is just a limitation of librados - start_after is a c string, so listing will end early if a key has a null char and max_return is too small.

nickjanus · 2022-04-13T20:27:32Z

This should be ready for CI again.

nickjanus · 2022-04-15T14:31:05Z

👋 The test failure for Pacific looks unrelated to these commit. Let me know if there's any other changes I should make, and thanks for looking!

phlogistonjohn

Looks good to me. Just because it's a bit of a subtle bunch of changes I would like a 2nd reviewer to look at it. That's the only reason for the dnm label at this time.

@ansiwen PTAL and remove the 'do-not-merge' label if you approve. Thanks!

ansiwen

Well, I don't like the rados_object_list API, but that's not the faut of this PR. ;-) (Why you have use a finish cursor, if you anyway have to check for the end of the list with ..._is_end? 🙄 )

phlogistonjohn · 2022-04-21T13:44:04Z

@Mergifyio rebase

The previous `rados_nobjects_list_open` used for listing could not return oids that included null characters. Although this isn't a common case, having go-ceph able to list such objects is useful for bugs such as https://tracker.ceph.com/issues/48874. Signed-off-by: Nick Janus <github@nondesignated.com>

Previous changes added support for operating on omap keys that include null characters (e.g. rados_omap_get_next2, rados_write_op_omap_rm_keys2). This change adds some basic test coverage those operations' null character support. Signed-off-by: Nick Janus <github@nondesignated.com>

mergify · 2022-04-21T13:44:53Z

rebase

✅ Branch has been successfully rebased

phlogistonjohn · 2022-04-21T19:25:51Z

@Mergifyio requeue

mergify · 2022-04-21T19:26:24Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

nickjanus · 2022-04-22T18:01:31Z

Thanks, folks!

phlogistonjohn requested changes Apr 5, 2022

View reviewed changes

nickjanus force-pushed the list_oids_null branch from 248aea0 to 85abd74 Compare April 6, 2022 00:31

phlogistonjohn requested changes Apr 6, 2022

View reviewed changes

nickjanus force-pushed the list_oids_null branch from 85abd74 to e80ca00 Compare April 7, 2022 21:05

nickjanus force-pushed the list_oids_null branch from e80ca00 to a5e72a0 Compare April 13, 2022 03:15

nickjanus force-pushed the list_oids_null branch from a5e72a0 to fb7a38f Compare April 13, 2022 03:20

phlogistonjohn added the do-not-merge label Apr 15, 2022

phlogistonjohn approved these changes Apr 15, 2022

View reviewed changes

ansiwen approved these changes Apr 21, 2022

View reviewed changes

ansiwen removed the do-not-merge label Apr 21, 2022

Nick Janus added 2 commits April 21, 2022 13:44

leseb force-pushed the list_oids_null branch from fb7a38f to 7c70f13 Compare April 21, 2022 13:44

phlogistonjohn added the no-API This PR does not include any changes to the public API of a go-ceph package label Apr 21, 2022

mergify bot merged commit e1f75a4 into ceph:master Apr 21, 2022

nickjanus deleted the list_oids_null branch April 22, 2022 18:01

Conversation

nickjanus commented Apr 5, 2022

Checklist

Uh oh!

phlogistonjohn left a comment

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn commented Apr 5, 2022

Uh oh!

nickjanus commented Apr 6, 2022

Uh oh!

phlogistonjohn commented Apr 6, 2022

Uh oh!

phlogistonjohn left a comment

Choose a reason for hiding this comment

Uh oh!

nickjanus commented Apr 6, 2022

Uh oh!

nickjanus commented Apr 7, 2022

Uh oh!

phlogistonjohn commented Apr 8, 2022

Uh oh!

nickjanus commented Apr 8, 2022

Uh oh!

phlogistonjohn commented Apr 8, 2022

Uh oh!

nickjanus commented Apr 13, 2022

Uh oh!

nickjanus commented Apr 13, 2022

Uh oh!

nickjanus commented Apr 15, 2022

Uh oh!

phlogistonjohn left a comment

Choose a reason for hiding this comment

Uh oh!

ansiwen left a comment

Choose a reason for hiding this comment

Uh oh!

phlogistonjohn commented Apr 21, 2022

Uh oh!

mergify bot commented Apr 21, 2022

✅ Branch has been successfully rebased

Uh oh!

phlogistonjohn commented Apr 21, 2022

Uh oh!

mergify bot commented Apr 21, 2022

❌ This pull request head commit has not been previously disembarked from queue.

Uh oh!

nickjanus commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants