Skip to content

Bugfixes, tests, and tools around checksums #2259

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
merged 6 commits into from
Apr 29, 2025
Merged

Bugfixes, tests, and tools around checksums #2259

merged 6 commits into from
Apr 29, 2025

Conversation

bbockelm
Copy link
Collaborator

This PR improves the checksum functionality:

  • Adds a bugfix that prevents checksum failures from not being reported as checksum mismatch errors (@jhiemstrawisc - I think this is probably important enough to put into a 7.16 RC).
  • Records the client and server-side checksums into the plugin outputs. This will allow ElasticSearch to detect the frequency of errors.
  • Adds a flag to pelican object stat that allows CLI users to get a checksum back.

Example:

pelican object stat --json --checksums md5 pelican://`hostname`:8444/tmp/test/hello_world.txt
{"Name":"/tmp/test/hello_world.txt","Size":12,"ModTime":"2025-04-26T15:16:26Z","IsCollection":false,"checksums":{"md5":"6f5902ac237024bdd0c176cb93063dc4"}}

@bbockelm bbockelm added bug Something isn't working enhancement New feature or request client Issue affecting the OSDF client critical High priority for next release labels Apr 26, 2025
@bbockelm bbockelm added this to the v7.16 milestone Apr 26, 2025
@bbockelm bbockelm requested a review from jhiemstrawisc April 26, 2025 16:15
jhiemstrawisc
jhiemstrawisc requested changes Apr 28, 2025
View reviewed changes
client/handle_http_test.go
}

// Test behavior when checksum is missing
func TestChecksumMissing(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Each of these new checksum tests is largely a copy-paste-tweak of the others. Instead of using three separate tests, can you create one test that handles a slice of test cases?

bbockelm added 5 commits April 29, 2025 13:27
@bbockelm
Improve the ClassAd string printer to handle deeply nested ClassAds.
3728efc
@bbockelm
Improve handling of checksums
d45c7b6 
- Export checksum type / name information outside the client library.
- Add standardized error types and singletons for failure checking.
- Do not overwrite checksum mismatch error once it occurs.
- Truncate checksum byte array to provided data length.
@bbockelm
Add unit test coverage for checksums
1bf19c2
@bbockelm
Add checksum output to stat
e74c984
@bbockelm
Record checksum results in developer data
5e99561
@bbockelm bbockelm linked an issue Apr 29, 2025 that may be closed by this pull request
Add checksum support to the client #2206
Closed
@jhiemstrawisc jhiemstrawisc self-requested a review April 29, 2025 20:24
jhiemstrawisc
jhiemstrawisc approved these changes Apr 29, 2025
View reviewed changes
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

LGTM. I'll add this as a 7.16 patch as well.

@jhiemstrawisc jhiemstrawisc merged commit 5103884 into PelicanPlatform:main Apr 29, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhiemstrawisc jhiemstrawisc jhiemstrawisc approved these changes

Assignees
No one assigned
Labels
bug Something isn't working client Issue affecting the OSDF client critical High priority for next release enhancement New feature or request
Projects
None yet
Milestone
v7.16
Development

Successfully merging this pull request may close these issues.

Add checksum support to the client
2 participants
@bbockelm @jhiemstrawisc