rgw: cksum: implement support for new CRC64NVME checksum algorithm and related fixes by mattbenjamin · Pull Request #61878 · ceph/ceph

mattbenjamin · 2025-02-18T16:39:34Z

Fixes: https://tracker.ceph.com/issues/70040

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

mattbenjamin · 2025-02-24T23:03:58Z

(there is no relationship to nvmeof; one commit incorrectly changed the nvmeof/gateway submodule, that's fixed)

cbodley · 2025-02-26T14:54:18Z

now that you're using the madler crc library, do we still need the bits from spdk? if so, is there a reason we can't pull up our spdk submodule and link to that?

are the madler files coming from this https://github.com/madler/crcany repo? why not add that as a submodule?

p.s. please update COPYING with the added licenses

mattbenjamin · 2025-03-02T00:27:32Z

some todo-list items:

handle client requested checksum "type" (COMPOSITE or FULL_OBJECT)
improve the abstraction around "type" without undue ugliness
unit tests for combiners--would like a full matrix of algorithms and optional type
add to copying (thanks)
add s3-tests

mattbenjamin · 2025-03-02T00:30:10Z

spdk: as discussed, the checksums here have simd acceleration and do seem to be in both isa-l standalone and the most recent versions of spdk, so there will be some way to remove these, but right now, I seem to need the table versions here. Can be improved with some build work. crcany (madler) provides table-based combining methods that work, I intend to rely on them only for that, and as cross checks on the others.

mattbenjamin · 2025-03-02T22:15:28Z

crcany: it could be a submodule; is it worth it?

mattbenjamin · 2025-03-03T18:39:18Z

related s3-tests branch: ceph/s3-tests#626

mattbenjamin · 2025-03-03T21:02:31Z

@cbodley just rebased, am having confusing misbehavior. I've successfully completed a multipart upload with CRC64NVME checksum. but when my s3-test does a HEAD request with ChecksumMode = ENABLED, it is getting the right data from the stored checksujm it is adding the correct information to the formatter (below), but the s3-test logic is failing with

FAILED s3tests_boto3/functional/test_s3.py::test_multipart_use_cksum_helper_crc64nvme - KeyError: 'ChecksumCRC64NVME'

here's what the rgw log says:
2025-03-03T15:56:30.627-0500 7f809261b6c0 20 req 8392445594298461084 0.004000062s s3:get_obj Read xattr rgw_rados: user.rgw.x-amz-checksum-type 2025-03-03T15:56:30.627-0500 7f809261b6c0 10 req 8392445594298461084 0.004000062s cache get: name=default.rgw.log++script.getdata. : hit (negative entry) 2025-03-03T15:56:30.627-0500 7f809261b6c0 15 req 8392445594298461084 0.004000062s Encryption mode: 2025-03-03T15:56:30.627-0500 7f809261b6c0 16 req 8392445594298461084 0.004000062s s3:get_obj INFO: send_response_data ChecksumMode==ENABLED element-name ChecksumCRC64NVME value i+6LR0y3eFo= 2025-03-03T15:56:30.627-0500 7f809261b6c0 2 req 8392445594298461084 0.004000062s s3:get_obj completing 2025-03-03T15:56:30.627-0500 7f809261b6c0 10 req 8392445594298461084 0.004000062s cache get: name=default.rgw.log++script.postrequest. : hit (negative entry) 2025-03-03T15:56:30.627-0500 7f809261b6c0 2 req 8392445594298461084 0.004000062s s3:get_obj op status=0 2025-03-03T15:56:30.627-0500 7f809261b6c0 2 req 8392445594298461084 0.004000062s s3:get_obj http status=200 2025-03-03T15:56:30.627-0500 7f809261b6c0 1 ====== req done req=0x7f804fd75180 op=get_obj bucket=mattb-buspqoc7wdy5sjs4v14y0go-1 status=0 http_status=200 latency=0.004000062s request_id=tx000007477f4cd8cf3ab9c-0067c6177e-4209-default ======

mattbenjamin · 2025-03-03T21:03:36Z

I've debugged this, the head request is succeeding, but just doesn't have the checksum element in it

mattbenjamin · 2025-03-03T21:50:32Z

@cbodley ok never mind, thanks for spotting the issue in slack

src/rgw/rgw_rest_s3.cc

cbodley · 2025-03-12T12:15:29Z

src/rgw/rgw_rest_s3.cc

+			  __func__, cksum.element_name(), cksum.to_armor());
+	    dump_header(s, cksum.header_name(), armored_cksum);
 	  }
+	  dump_header(s, "ChecksumType", std::get<1>(cksum_type));


ChecksumType -> x-amz-checksum-type

mattbenjamin · 2025-03-26T03:41:30Z

wtf? in ceph-api-tests:

mattbenjamin · 2025-03-26T03:42:18Z

also, github is saying there are modified submodules, but I don't think that's the case

mattbenjamin · 2025-03-26T03:43:37Z

I'll check into whether jenkins is broken tomorrow, before rebasing

cbodley · 2025-03-26T12:47:41Z

wtf? in ceph-api-tests:

not your fault. there's a fix in #62515

rzarzynski · 2025-03-26T15:04:40Z

Should be good now; retriggering the check bots.

rzarzynski · 2025-03-26T15:05:00Z

jenkins retest this please

Signed-off-by: Matt Benjamin <[email protected]>

Support uses conditionally compiled implementations from spdk and ISA-L. Validated against SPDK test vectors and one example generated and passed via awscliv2 (AWS SDK) version 2.24.5. Restored the ability of unittest_rgw_cksum to create a binary input file for external checksum testing, but only when requested via cmdline option. Fixes: https://tracker.ceph.com/issues/70040 Signed-off-by: Matt Benjamin <[email protected]>

These constructions provide conforming implemenations of Amazon S3 CRC32 (ISO hdlc), CRC32C (iscsi), and CRC64/NVME checksums, in particular, linear combining of adjacent checksums, by Mark Adler. Source: https://github.com/madler/crcany License: approximate BSD with "optional" advertising clause Signed-off-by: Matt Benjamin <[email protected]>

mattbenjamin · 2025-03-27T01:53:36Z

jenkins test submodules

mattbenjamin · 2025-03-27T22:09:35Z

hopeful suite run: https://pulpito.ceph.com/mbenjamin-2025-03-27_22:06:02-rgw-wip-cksum-badalg-distro-default-gibba/

mattbenjamin · 2025-03-28T02:10:18Z

the earlier run passed, but was missing some validation steps, re-run (also, rebased to the latest s3-tests/master):
https://pulpito.ceph.com/mbenjamin-2025-03-28_01:54:23-rgw-wip-cksum-badalg-distro-default-gibba/

cbodley · 2025-03-31T13:42:17Z

consider squashing some of the fix commits? especially if you're interested in backports

mattbenjamin · 2025-03-31T14:49:00Z

latest passing run: https://pulpito.ceph.com/mbenjamin-2025-03-28_20:54:12-rgw-wip-cksum-badalg-distro-default-gibba/

* internally compensate for at-rest byteswapped crc64 representation (e.g., before combine step) * generalize Cksum crc api for 32bit and 64bit crcs, other cleanup * prototype abstract cksum::Combiner workflow * add support for forward and backward handling of composite vs full object checksums by marking the composites and the update flag day * clean up checksum formatting and checksum type reporting * add unit tests for Combiner interface * validate and track requested checksum type (i.e., composite or full), plus unit test fixture for combinations of full matrix of cksum types * doh. GET/HEAD checksums are in headers * add crcany license to COPYING * return ChecksumType as header in GET/HEAD Found by Casey in review * avoid fmt of char* null when no checksum header supplied A cksum_hdr_t(nullptr, nullptr) results in this case, and is intended, but its components obviously can't be presented to std::format unless cast to a safe pointer type. * fail checksum mismatch with BadDigest When uploading an S3 object with an invalid checksum, the return code should be BadDigest to mirror more closely the AWS S3 implementation. See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/checking-object-integrity.html Fixes: https://tracker.ceph.com/issues/70614 Reported by Alex Wojno <[email protected]> * fix comparison and display of composite checksums A string comparision bounds error and a bitmask comparison error are fixed. * fix build on centos9 On gcc(?13?) we can't declare rgw::cksum::FLAG_NONE and also rgw::cksum::Cksum::FlAG_NONE. SAD!! * include <variant> invariantly * fix checksum type return from complete-multipart This one is in the XML response * aieee, don't leak Combiners Use unique_ptr to polymorphic type...correctly. Signed-off-by: Matt Benjamin <[email protected]>

mattbenjamin · 2025-03-31T17:43:35Z

jenkins test make check

cbodley · 2025-03-31T17:44:01Z

from https://jenkins.ceph.com/job/ceph-pull-requests/155043/

34 tests failed out of 310

added a comment to https://tracker.ceph.com/issues/70700

mattbenjamin added the bug-fix label Feb 18, 2025

mattbenjamin self-assigned this Feb 18, 2025

mattbenjamin requested a review from a team as a code owner February 18, 2025 16:39

github-actions bot added build/ops nvmeof rgw tests labels Feb 18, 2025

mattbenjamin force-pushed the wip-cksum-badalg branch 3 times, most recently from 716488b to d6b81e1 Compare February 24, 2025 19:44

mattbenjamin removed the nvmeof label Feb 24, 2025

mattbenjamin force-pushed the wip-cksum-badalg branch from bd16d85 to f95b874 Compare March 3, 2025 20:53

mattbenjamin force-pushed the wip-cksum-badalg branch from d396c19 to c8fadc9 Compare March 3, 2025 21:48

mattbenjamin requested a review from a team as a code owner March 5, 2025 18:57

github-actions bot added the documentation label Mar 5, 2025

cbodley reviewed Mar 5, 2025

View reviewed changes

src/rgw/rgw_rest_s3.cc Outdated Show resolved Hide resolved

mattbenjamin force-pushed the wip-cksum-badalg branch from 355a56b to e03b659 Compare March 12, 2025 11:38

cbodley reviewed Mar 12, 2025

View reviewed changes

mattbenjamin force-pushed the wip-cksum-badalg branch from e03b659 to 4ad280b Compare March 12, 2025 21:20

mattbenjamin force-pushed the wip-cksum-badalg branch from 28e51ee to f89aa0c Compare March 26, 2025 06:55

mattbenjamin added 4 commits March 26, 2025 15:29

rgw: borrow crc64_nvme code from spdk 24.9.0

51140d6

Signed-off-by: Matt Benjamin <[email protected]>

rgw: handle bad/unknown checksum case in cksum::Factory

07afdc1

Signed-off-by: Matt Benjamin <[email protected]>

mattbenjamin force-pushed the wip-cksum-badalg branch from d3c7ca4 to 6aa4ec9 Compare March 26, 2025 22:52

mattbenjamin closed this Mar 27, 2025

mattbenjamin reopened this Mar 27, 2025

mattbenjamin force-pushed the wip-cksum-badalg branch 2 times, most recently from 51625bc to 2c037bf Compare March 27, 2025 03:14

mattbenjamin force-pushed the wip-cksum-badalg branch from dafec13 to c2c0555 Compare March 28, 2025 23:09

cbodley approved these changes Mar 31, 2025

View reviewed changes

cbodley mentioned this pull request Mar 31, 2025

squid: rgw: implement S3 additional checksum support #62312

Open

14 tasks

mattbenjamin force-pushed the wip-cksum-badalg branch from c2c0555 to ee03b50 Compare March 31, 2025 15:10

cbodley added TESTED ready-to-merge labels Mar 31, 2025

cbodley merged commit f9a8403 into ceph:main Mar 31, 2025
12 checks passed

cbodley mentioned this pull request Mar 31, 2025

<rgw> Ensure the ETag format is consistent with AWS S3 API #60477

Merged

14 tasks

Comments

Conversation

mattbenjamin commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

mattbenjamin commented Feb 24, 2025

Uh oh!

cbodley commented Feb 26, 2025

Uh oh!

mattbenjamin commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbenjamin commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbenjamin commented Mar 2, 2025

Uh oh!

mattbenjamin commented Mar 3, 2025

Uh oh!

mattbenjamin commented Mar 3, 2025

Uh oh!

mattbenjamin commented Mar 3, 2025

Uh oh!

mattbenjamin commented Mar 3, 2025

Uh oh!

Uh oh!

cbodley Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

mattbenjamin Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

mattbenjamin commented Mar 26, 2025

Uh oh!

mattbenjamin commented Mar 26, 2025

Uh oh!

mattbenjamin commented Mar 26, 2025

Uh oh!

cbodley commented Mar 26, 2025

Uh oh!

rzarzynski commented Mar 26, 2025

Uh oh!

rzarzynski commented Mar 26, 2025

Uh oh!

mattbenjamin commented Mar 27, 2025

Uh oh!

mattbenjamin commented Mar 27, 2025

Uh oh!

mattbenjamin commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Mar 31, 2025

Uh oh!

mattbenjamin commented Mar 31, 2025

Uh oh!

mattbenjamin commented Mar 31, 2025

Uh oh!

cbodley commented Mar 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattbenjamin commented Feb 18, 2025 •

edited

Loading

mattbenjamin commented Mar 2, 2025 •

edited

Loading

mattbenjamin commented Mar 2, 2025 •

edited

Loading

mattbenjamin commented Mar 28, 2025 •

edited

Loading