Skip to content

Updated commit hash in ceph tests #9152

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achouhan09
Copy link
Member

@achouhan09 achouhan09 commented Jul 22, 2025

Describe the Problem

After updating the ceph commit hash, s3 tests were failing for both NSFS and non-NSFS configurations. The failures were caused by missing required configuration fields that the updated ceph test suite expects.

Explain the Changes

  1. Added the missing fields in the config file with dummy values currently, we will update it in future when we support the functionality.
  2. Updated the commit hash to latest.
  3. Moved new tests to the blacklist.
  4. Two tests (regressoin) are failing, moved to pending list and updated the doc with the root cause.

s3tests_boto3/functional/test_headers.py::test_object_create_bad_md5_bad
s3tests_boto3/functional/test_headers.py::test_object_create_bad_md5_none

ceph-s3 output:

2025-07-28T12:15:44.2198979Z Jul-28 12:15:44.219 [test_ceph_s3/182] [LOG] CONSOLE:: Finished Running Ceph S3 Tests
2025-07-28T12:15:44.2200527Z Jul-28 12:15:44.219 [test_ceph_s3/182] [INFO] CONSOLE:: CEPH TEST SUMMARY: Suite contains 1008, ran 430 tests, Passed: 289, Skipped: 139, Failed: 2
2025-07-28T12:15:44.2204423Z Jul-28 12:15:44.219 [test_ceph_s3/182] [WARN] CONSOLE:: CEPH TEST SKIPPED TESTS SUMMARY: 139 skipped tests

nsfs-ceph-s3 output:

Jul-22 16:43:03.672 [test_ceph_s3/121] [LOG] CONSOLE:: Finished Running Ceph S3 Tests
Jul-22 16:43:03.672 [test_ceph_s3/121] [INFO] CONSOLE:: CEPH TEST SUMMARY: Suite contains 1005, ran 369 tests, Passed: 230, Skipped: 139, Failed: 0
Jul-22 16:43:03.672 [test_ceph_s3/121] [WARN] CONSOLE:: CEPH TEST SKIPPED TESTS SUMMARY: 139 skipped tests

Issues: Fixed #xxx / Gap #xxx

  1. Fixed: https://issues.redhat.com/browse/MCGI-254
  2. GAP: There are two more tests (regression) which are failing and moved to pending list, but not sure about the root cause need to investigate.

s3tests_boto3/functional/test_headers.py::test_object_create_bad_md5_bad
s3tests_boto3/functional/test_headers.py::test_object_create_bad_md5_none

Testing Instructions:

  1. make test-cephs3 and make test-nsfs-cephs3
  • Doc added/updated
  • Tests added

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Expanded configuration options for Ceph S3 tests, including new IAM-related settings and structured sections for IAM accounts and credentials.
  • Chores

    • Updated the Ceph S3 tests repository to a newer version.
    • Improved configuration setup by standardizing key insertion through placeholder substitution.
    • Enhanced test selection filtering to target specific test identifiers.
    • Added numerous new test cases to pending test lists covering extensive S3 functionality.
    • Extended test blacklists to exclude additional S3 and STS tests due to known incompatibilities.
  • Documentation

    • Added entries to pending tests documentation highlighting known AWS v2 signature issues due to botocore updates.

@achouhan09 achouhan09 requested review from nadavMiz, romayalon, a team and alphaprinz and removed request for a team July 22, 2025 11:36
Copy link

coderabbitai bot commented Jul 22, 2025

"""

Walkthrough

The changes update the Ceph S3 test configuration to include new IAM-related sections and keys, adjust the setup scripts to use placeholder substitution for access/secret keys via sed on macOS and other platforms, modify the test listing command to filter more precisely, expand pending and blacklist test lists with many new entries, update the test deployment script to use a newer commit of the Ceph S3 tests repository, and add documentation entries for known test failures related to botocore updates. No core logic or control flow is altered.

Changes

File(s) Change Summary
Ceph S3 test configuration
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
Added new IAM-related sections and keys to support IAM and sts-related tests.
Setup scripts for config substitution
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js, src/test/external_tests/ceph_s3_tests/test_ceph_nsfs_s3_config_setup.js
Replaced direct key appending with placeholder substitution for access/secret keys using sed on macOS and others.
Test listing command
src/test/external_tests/ceph_s3_tests/test_ceph_s3.js
Modified test listing command to filter lines containing "::test" instead of any "test".
Deployment script
src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh
Updated the Ceph S3 tests repository commit hash to a newer version.
Pending test lists
src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt, src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt
Added new test entries to pending test lists.
Blacklist test lists
src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt, src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_black_list.txt
Extended blacklists with many new test entries covering various S3 and STS features.
Documentation
docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md
Added documentation entries for two new pending tests related to botocore update breaking AWS v2 signature handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~40 minutes

Possibly related PRs

Suggested reviewers

  • nadavMiz
    """

Suggested labels

size/XS

Suggested reviewers

  • nadavMiz

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ed1709 and 063231c.

📒 Files selected for processing (10)
  • docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_black_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_nsfs_s3_config_setup.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf (2 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3.js
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt
  • src/test/external_tests/ceph_s3_tests/test_ceph_nsfs_s3_config_setup.js
  • docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*

⚙️ CodeRabbit Configuration File

src/test/**/*.*: Ensure that the PR includes tests for the changes.

Files:

  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_black_list.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-package-lock-validation
  • GitHub Check: run-jest-unit-tests
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24e11b7 and eb679ea.

📒 Files selected for processing (3)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf (2 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh (1 hunks)
📓 Path-based instructions (1)
src/test/**/*.*

⚙️ CodeRabbit Configuration File

src/test/**/*.*: Ensure that the PR includes tests for the changes.

Files:

  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*

⚙️ CodeRabbit Configuration File

src/test/**/*.*: Ensure that the PR includes tests for the changes.

Files:

  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh (1)

20-20: Validated new CEPH_TESTS_VERSION hash

  • The commit aafacab2017bec1dd1ce3159105455bb7e9447b1 exists in the ceph/s3-tests repository on master.
  • It is only 4 days old, well within the 600-day limit.

No further changes required.

src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js (1)

95-96: Good improvement in credential injection consistency.

The change from appending keys to replacing placeholders using sed improves consistency and aligns well with the updated config file structure. The OS-specific handling for macOS is correctly implemented.

Also applies to: 101-102

src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf (2)

22-28: LGTM: IAM naming configuration added.

The new IAM naming prefixes support the enhanced IAM functionality mentioned in the PR objectives.


62-63: LGTM: Tenant name configuration added.

The tenant field addition supports enhanced tenant testing capabilities.

@achouhan09 achouhan09 marked this pull request as draft July 22, 2025 12:40
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jul 22, 2025
@achouhan09 achouhan09 marked this pull request as ready for review July 23, 2025 04:56
@achouhan09 achouhan09 force-pushed the ceph-fix branch 7 times, most recently from e59a8e5 to 2c0d694 Compare July 28, 2025 10:46
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt (2)

435-471: Collapse the bucket-logging suite with a wildcard entry

Lines 435-471 list 37 separate test_bucket_logging_* cases.
If the blacklist parser accepts pytest-style globs, replace them with one line:

s3tests_boto3/functional/test_s3.py::test_bucket_logging*

This keeps the file short and automatically covers new variants that may appear.


426-434: Wildcard the checksum tests to avoid future omissions

The checksum-related tests (426-434) could similarly be collapsed to:

s3tests_boto3/functional/test_s3.py::test_*checksum*

Should more checksum variants be added upstream, they will be blacklisted automatically.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c03fd4 and 2c0d694.

📒 Files selected for processing (10)
  • docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_black_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_nsfs_s3_config_setup.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf (2 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js (1 hunks)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3.js
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_black_list.txt
🚧 Files skipped from review as they are similar to previous changes (7)
  • docs/dev_guide/ceph_s3_tests/ceph_s3_tests_pending_list_status.md
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh
  • src/test/external_tests/ceph_s3_tests/test_ceph_nsfs_s3_config_setup.js
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/s3_tests_pending_list.txt
  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_pending_list.txt
  • src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*

⚙️ CodeRabbit Configuration File

src/test/**/*.*: Ensure that the PR includes tests for the changes.

Files:

  • src/test/external_tests/ceph_s3_tests/s3-tests-lists/nsfs_s3_tests_black_list.txt

Copy link
Contributor

@alphaprinz alphaprinz left a comment

Choose a reason for hiding this comment

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

That's quite a lot of black listed tests.
Is there a recurring pattern in their failure?

Also, I see that tests still failing.
Annoyingly, I am unable to get the log of the run, it's too long and Chrome hangs.

… list

Signed-off-by: Aayush <aayush@li-2392f9cc-287a-11b2-a85c-9fcc04b05da6.ibm.com>
@achouhan09
Copy link
Member Author

@alphaprinz

  1. The tests which are put in the blacklist are the new tests introduced in the ceph.
  2. There were two more tests (regression) which are failing the CI, so I moved them them to pending list.

@achouhan09 achouhan09 requested a review from alphaprinz July 29, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants