Skip to content
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

cephfs-shell: getxattr fail while the xattr's length > 256 #53126

Merged
merged 1 commit into from
Nov 3, 2023
Merged

cephfs-shell: getxattr fail while the xattr's length > 256 #53126

merged 1 commit into from
Nov 3, 2023

Conversation

tengjie5
Copy link
Contributor

Fixes: https://tracker.ceph.com/issues/62545
Signed-off-by: teng jie tengjie5@asiainfo.com
Please enter the commit message for your changes. Lines starting

Contribution Guidelines

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

@github-actions github-actions bot added the cephfs Ceph File System label Aug 24, 2023
@vshankar vshankar requested a review from a team September 7, 2023 10:33
mchangir
mchangir reviewed Sep 26, 2023
View reviewed changes
src/tools/cephfs/shell/cephfs-shell
@@ -1633,7 +1633,7 @@ class CephFSShell(Cmd):
"""
try:
poutput('{}'.format(cephfs.getxattr(args.path,
to_bytes(args.name)).decode('utf-8')))
to_bytes(args.name), size=65536).decode('utf-8')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a problem with the xattr name or value ?
Please mention the same as part of the commit message as the description in the tracker is confusing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is about length of xattr's value.when the value's length >256, report this error.I've read code of function getxattr，default value of size is 256，so I set it to 65536 like linux.

Copy link
Contributor

Choose a reason for hiding this comment

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

Linux also uses MAX_SIZE as power 16, seems ok to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename the commit title to:
cephfs-shell: bump up acceptable xattr value len to 64K

Also, remove the spurious line in the commit message:
Please enter the commit message for your changes. Lines starting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mchangir Title has been modified，thanks for the review.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is about length of xattr's value.when the value's length >256, report this error.I've read code of function getxattr，default value of size is 256，so I set it to 65536 like linux.

I think it'll be useful to add this detail to the commit message and to the tracker ticket.

@tengjie5 tengjie5 force-pushed the origin/tmp2 branch 2 times, most recently from 9d11ae1 to 2befcd2 Compare October 10, 2023 02:12
@tengjie5
Copy link
Contributor Author

@neesingh-rh hi，excuse me，Is there a problem with the API test？it reported error as follows，i don't think it caused by my pr.

AssertionError: timeout expired in wait_for_all_osds_up
Traceback (most recent call last):
File "/home/jenkins-build/build/workspace/ceph-api/build/../qa/tasks/vstart_runner.py", line 1537, in
exec_test()
File "/home/jenkins-build/build/workspace/ceph-api/build/../qa/tasks/vstart_runner.py", line 1392, in exec_test
LocalCephCluster(LocalContext()).mon_manager.wait_for_all_osds_up(timeout=30)
File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/ceph_manager.py", line 2778, in wait_for_all_osds_up
assert time.time() - start < timeout,
AssertionError: timeout expired in wait_for_all_osds_up

@joscollin
Copy link
Member

jenkins test api

@tengjie5
Copy link
Contributor Author

jenkins test api

@joscollin api test has a new error，could you help me to take a look? thanks.

@joscollin
Copy link
Member

jenkins test api

@joscollin api test has a new error，could you help me to take a look? thanks.

Could you please rebase your branch and force push again?
(git pull and force push)

@tengjie5
Copy link
Contributor Author

jenkins test api

@joscollin api test has a new error，could you help me to take a look? thanks.

Could you please rebase your branch and force push again? (git pull and force push)

It's ok now, thanks.

rishabh-d-dave
rishabh-d-dave approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

@rishabh-d-dave rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Oct 13, 2023
kotreshhr
kotreshhr reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@kotreshhr kotreshhr left a comment

Choose a reason for hiding this comment

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

Could we add a testcase for this in qa/tasks/cephfs/test_cephfs_shell.py?

rishabh-d-dave
rishabh-d-dave approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

CephFS integration tests ran successfully - https://tracker.ceph.com/projects/cephfs/wiki/Main#3-Nov-2023

@rishabh-d-dave rishabh-d-dave merged commit 32593ed into ceph:main Nov 3, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kotreshhr kotreshhr kotreshhr left review comments

@mchangir mchangir mchangir left review comments

@rishabh-d-dave rishabh-d-dave rishabh-d-dave approved these changes

@joscollin joscollin joscollin approved these changes

@neesingh-rh neesingh-rh neesingh-rh approved these changes

Assignees
No one assigned
Labels
cephfs Ceph File System needs-review wip-rishabh-testing Rishabh's testing label
Projects
None yet
Milestone
No milestone
6 participants
@tengjie5 @joscollin @rishabh-d-dave @kotreshhr @mchangir @neesingh-rh