Skip to content

src/test/TestRados: add max-attr-len to control the max length of attributes sent to OSDs#56114

Merged
Matan-B merged 2 commits intoceph:mainfrom
xxhdx1985126:wip-seastore-teuthology-max-test-attr-len
May 19, 2024
Merged

src/test/TestRados: add max-attr-len to control the max length of attributes sent to OSDs#56114
Matan-B merged 2 commits intoceph:mainfrom
xxhdx1985126:wip-seastore-teuthology-max-test-attr-len

Conversation

@xxhdx1985126
Copy link
Contributor

@xxhdx1985126 xxhdx1985126 commented Mar 11, 2024 •
edited
Loading

SeaStore can't handle attributes that are too big, so we need to constrain it during tests

Signed-off-by: Xuehan Xu xuxuehan@qianxin.com

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)
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

@xxhdx1985126
test/osd/TestRados: add parameter max-attr-len
45a6d3f 
SeaStore can't handle too big attributes

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
@xxhdx1985126 xxhdx1985126 requested a review from athanatos March 11, 2024 08:56
@xxhdx1985126 xxhdx1985126 requested a review from a team as a code owner March 11, 2024 08:56
@xxhdx1985126 xxhdx1985126 marked this pull request as draft March 12, 2024 05:21
@xxhdx1985126 xxhdx1985126 force-pushed the wip-seastore-teuthology-max-test-attr-len branch from f00cb29 to 413fadd Compare March 12, 2024 09:59
@xxhdx1985126 xxhdx1985126 marked this pull request as ready for review March 12, 2024 10:00
@xxhdx1985126 xxhdx1985126 requested a review from Matan-B March 12, 2024 10:00
Matan-B
Matan-B requested changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment •
edited
Loading

Choose a reason for hiding this comment

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

max-attr-len should be also added to qa/tasks/rados.py so that we could parse qa yaml files correctly.

@athanatos
Copy link
Contributor

athanatos commented Mar 12, 2024

This is a reasonable stop-gap for now, but we do need to support xattrs longer than 8k -- let's open a bug and note it in the yaml next to the config. What's the current architectural limitation restricting us to 8k?

@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Mar 13, 2024 •
edited
Loading

This is a reasonable stop-gap for now, but we do need to support xattrs longer than 8k -- let's open a bug and note it in the yaml next to the config. What's the current architectural limitation restricting us to 8k?

It's limited by the size of the omap leaf node; actually, even with the current omap leaf node size (64KB), the limit is not necessarily 8k, I just wanted to run the teuthology test, so just set the limit to a definitely workable value.

@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Mar 13, 2024

max-attr-len should be also added to qa/tasks/rados.py so that we could parse qa yaml files correctly.

Um, I think I've added it.

@Matan-B
Copy link
Contributor

Matan-B commented Mar 13, 2024

Let's change the max-attr-len values in the crimson-rados-experimental suite since it's seastore specific.
#56150 (review)

@xxhdx1985126 xxhdx1985126 mentioned this pull request Mar 13, 2024
Merged
14 tasks
Matan-B
Matan-B reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

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

My suggestion would be to only introduce max_attr_len in this PR.
After this one is merged, we can make use of it in the other seastore PR.
I would prefer not using this option with bluestore tests.

@xxhdx1985126
qa/suites/crimson-rados(-experimental): set max-attr-len to 8192 for …
4594355 
…crimson thrash

tests

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
@xxhdx1985126 xxhdx1985126 force-pushed the wip-seastore-teuthology-max-test-attr-len branch from 413fadd to 4594355 Compare March 14, 2024 06:33
@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Apr 30, 2024

Hi, everyone, is this PR good to merge? Thanks.

@athanatos athanatos added the wip-sjust-crimson-testing Test run pending by Sam label May 1, 2024
@Matan-B
Copy link
Contributor

Matan-B commented May 19, 2024

https://pulpito.ceph.com/matan-2024-05-15_11:06:44-crimson-rados-wip-sjust-crimson-testing-2024-05-09-distro-crimson-smithi/

all small-objects* passed with the new param.

@Matan-B Matan-B merged commit 14a1ef9 into ceph:main May 19, 2024
@Matan-B Matan-B mentioned this pull request Jun 13, 2024
Merged
@xxhdx1985126 xxhdx1985126 mentioned this pull request Aug 14, 2024
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@athanatos athanatos athanatos approved these changes

@Matan-B Matan-B Matan-B approved these changes

Assignees

No one assigned

Labels

core crimson needs-crimson-qa tests wip-sjust-crimson-testing Test run pending by Sam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xxhdx1985126 @athanatos @Matan-B