mds: Implement remove for ceph vxattrs by chrisphoffman · Pull Request #53755 · ceph/ceph

chrisphoffman · 2023-10-02T18:47:41Z

For a variety of ceph xattrs, the setfattr remove
isn't handled and "No such attribute" is returned.

Add support to remove xattrs to match where docs
say to clear value, one can remove or set to X value.
When removing, add "default" value back to xattr depending
on xattr name.

Fixes: https://tracker.ceph.com/issues/62793
Signed-off-by: Christopher Hoffman choffman@redhat.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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

chrisphoffman · 2023-10-03T13:33:31Z

In some kernels on test machines, vxattrs getxattr isn't supported. I've added a TODO, to uncomment getfattrs for failing xattrs when they are supported.

Here's the failure:
http://qa-proxy.ceph.com/teuthology/choffman-2023-09-29_15:56:41-fs:workload-wip-choffman-62793-distro-default-smithi/7406444/teuthology.log

Here's a working run with latest version (failure unrelated to test):
http://qa-proxy.ceph.com/teuthology/choffman-2023-10-02_17:48:08-fs:workload-wip-choffman-62793-distro-default-smithi/7408606/teuthology.log

dparmar18 · 2023-10-03T14:09:38Z

@chrisphoffman just a quick question, what about not providing any values with setfattr -n, will it set it to 0 in that case?

chrisphoffman · 2023-10-03T14:14:35Z

@chrisphoffman just a quick question, what about not providing any values with setfattr -n, will it set it to 0 in that case?

Yes in that case, it will set to default value. I provided a value because I wanted to test the value changing from something back to default. Are you suggesting something different?

dparmar18 · 2023-10-03T19:24:32Z

@chrisphoffman just a quick question, what about not providing any values with setfattr -n, will it set it to 0 in that case?

Yes in that case, it will set to default value. I provided a value because I wanted to test the value changing from something back to default. Are you suggesting something different?

So something like setfattr -n ceph.dir.pin foo would work and set it's value to -1, i'm not sure about the correctness but from user's perspective it looks like a bug where without providing any value it sets a value. General sense is that whatever sets something must accept something right and this although might not be that problematic if we mention this somewhere in our docs as a note but this will surely create some doubt in user's mind in first glance.

What i'm thinking is to only accept no values as valid arg only in case for removal of vxattr. So check if the client req is to set or remove the vxattr by looking at request's head op. Something like this

      if (value.length() == 0 && req->get_op() == CEPH_MDS_OP_RMXATTR) {
        value = "<reset_value>";
      }

So now we have the expected behaviour:

$ setfattr -n ceph.dir.pin -v 2 mnt/foo/
$ getfattr -n ceph.dir.pin mnt/foo/
# file: mnt/foo/
ceph.dir.pin="2"

$ setfattr -n ceph.dir.pin mnt/foo/
setfattr: mnt/foo/: Invalid argument
$ getfattr -n ceph.dir.pin mnt/foo/
# file: mnt/foo/
ceph.dir.pin="2"

$ setfattr -x ceph.dir.pin mnt/foo/
$ getfattr -n ceph.dir.pin mnt/foo/
# file: mnt/foo/
ceph.dir.pin="-1"

batrick · 2023-10-03T21:19:26Z

src/mds/Server.cc

+          || name == "ceph.dir.pin.distributed"
+          || name == "ceph.dir.pin.random"
+          || name == "ceph.dir.subvolume") {
+    handle_set_vxattr(mdr, cur);


It seems the comment above this if block is no longer correct. How about we change the code to:

if (name.find("ceph.") == 0) { if (req->get_data().length()) { respond_to_request(mdr, -CEPHFS_EINVAL); return; } handle_set_vxattr(mdr, cur); }

Fix all vxattrs that should not allow removal to reject the RPC (EINVAL).

That works for me. Thanks for the suggestion!

@chrisphoffman don't forget:

Fix all vxattrs that should not allow removal to reject the RPC (EINVAL).

Include in your test that setfattr -x ceph.X <path>. fails for vxattrs where removal makes no sense. I think ceph.quota is another example of a vxattr which should support rmxattr.

Took a while to unpack all of this. I did an audit of all xattrs handled. All ceph.* xattrs should either reset to def value, or reject for xattrs that shouldn't be removed (my judgement). If neither case, it is unknown xattr and will reject again with EINVAL.

batrick · 2023-10-03T21:20:26Z

src/mds/Server.cc


    bool val;
    try {
+      if (value.length() == 0) {


For readability, put at the start of this method:

bool is_rmxattr = (value.length() == 0);

Then use that.

I ended up using bool is_rmxattr = (req->get_op() == CEPH_MDS_OP_RMXATTR); instead. That way seemed more straight forward.

batrick · 2023-10-05T18:17:41Z

src/mds/Server.cc

+          || name == "ceph.dir.pin.distributed"
+          || name == "ceph.dir.pin.random"
+          || name == "ceph.dir.subvolume") {
+    handle_set_vxattr(mdr, cur);


@chrisphoffman don't forget:

Fix all vxattrs that should not allow removal to reject the RPC (EINVAL).

Include in your test that setfattr -x ceph.X <path>. fails for vxattrs where removal makes no sense. I think ceph.quota is another example of a vxattr which should support rmxattr.

github-actions · 2023-10-10T01:41:19Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

chrisphoffman · 2023-10-10T01:57:17Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Somehow extra commits got into the push and triggered this message. I removed the label and repushed as I don't believe there are any conflicts.

dparmar18 · 2023-10-10T06:45:06Z

doc/cephfs/quota.rst

   running ``getfattr /some/dir -n ceph.<some-xattr>``.

-To remove a quota, set the value of extended attribute to ``0``::
+To remove a quota, remove the respective extended attribute or set the value to ``0``::


i know it's quite trivial but how about adding example illustrating removal using setfattr -x, maybe helpful for naive users i guess and also the reason that we illustrate setting val to 0; this might be a good addition

dparmar18 · 2023-10-10T07:01:20Z

qa/workunits/fs/misc/general_vxattrs.sh

+#ceph.quota, def value 0, reset val 0
+setfattr -n ceph.quota.max_bytes -v 100000000 dir
+setfattr -n ceph.quota.max_files -v 10000 dir
+setfattr -x ceph.quota.max_bytes dir
+setfattr -x ceph.quota.max_files dir


we should check for the change using getfattr here, or maybe add a comment for now since you've mentioned the test machines don't support it yet?

* refs/pull/53755/head: PendingReleaseNotes: add note about CephFS set_vxattrs doc/cephfs: Update docs to match remove functionality and respective vxattrs qa: Add test coverage for vxattr behavior qa: Add removexattr to support setfattr removal. mds: Implement remove for ceph vxattrs Reviewed-by: Dhairya Parmar <dparmar@redhat.com> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> Reviewed-by: Milind Changire <mchangir@redhat.com>

vshankar

PTAL at the failure @chrisphoffman

qa/workunits/fs/misc/general_vxattrs.sh

For a variety of ceph xattrs, the setfattr remove isn't handled and "No such attribute" is returned. Add support to remove xattrs to match where docs say to clear value, one can remove or set to X value. When removing, add "default" value back to xattr depending on xattr name. Simplify current code to make clearer and minimize code reuse. Fixes: https://tracker.ceph.com/issues/62793 Signed-off-by: Christopher Hoffman <choffman@redhat.com>

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

Add tests to validate default and remove xattr behaviors. Signed-off-by: Christopher Hoffman <choffman@redhat.com>

…vxattrs Signed-off-by: Christopher Hoffman <choffman@redhat.com>

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

vshankar · 2024-06-03T09:05:17Z

jenkins retest this please

vshankar · 2024-06-03T11:03:36Z

This PR is under test in https://tracker.ceph.com/issues/66327.

chrisphoffman · 2024-06-10T12:23:04Z

jenkins test windows

chrisphoffman · 2024-06-10T18:49:38Z

jenkins render docs

chrisphoffman · 2024-06-10T18:50:18Z

jenkins test api

chrisphoffman · 2024-06-10T18:50:36Z

jenkins test make check

chrisphoffman · 2024-06-11T12:20:04Z

jenkins test make check

chrisphoffman · 2024-06-11T13:53:45Z

jenkins test docs

vshankar · 2024-06-17T09:44:01Z

This PR is under test in https://tracker.ceph.com/issues/66522.

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#wip-vshankar-testing-20240617094253-debug

(https://tracker.ceph.com/issues/66522)

chrisphoffman · 2024-06-24T14:36:54Z

jenkins retest this please

vshankar · 2024-06-25T07:11:33Z

jenkins test api

chrisphoffman · 2024-06-25T19:20:15Z

jenkins test api

github-actions bot added cephfs Ceph File System tests labels Oct 2, 2023

chrisphoffman requested a review from a team October 2, 2023 18:48

batrick requested changes Oct 3, 2023

View reviewed changes

chrisphoffman force-pushed the wip-62793 branch from 8739aec to 4aa8195 Compare October 4, 2023 19:20

batrick requested changes Oct 5, 2023

View reviewed changes

chrisphoffman force-pushed the wip-62793 branch from 4aa8195 to 53e869a Compare October 10, 2023 01:40

chrisphoffman requested review from a team as code owners October 10, 2023 01:40

github-actions bot added crimson documentation rbd needs-rebase labels Oct 10, 2023

chrisphoffman force-pushed the wip-62793 branch from 53e869a to 7b057af Compare October 10, 2023 01:45

github-actions bot removed the needs-rebase label Oct 10, 2023

chrisphoffman added documentation and removed documentation rbd tests crimson labels Oct 10, 2023

chrisphoffman removed the request for review from a team October 10, 2023 01:50

dparmar18 reviewed Oct 10, 2023

View reviewed changes

chrisphoffman force-pushed the wip-62793 branch from 7b057af to 76b53c4 Compare October 16, 2023 14:48

vshankar requested changes May 28, 2024

View reviewed changes

qa/workunits/fs/misc/general_vxattrs.sh Show resolved Hide resolved

vshankar removed the wip-vshankar-testing2 label May 28, 2024

chrisphoffman added 5 commits May 29, 2024 13:43

qa: Add removexattr to support setfattr removal.

26a352a

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

qa: Add test coverage for vxattr behavior

697c2fc

Add tests to validate default and remove xattr behaviors. Signed-off-by: Christopher Hoffman <choffman@redhat.com>

doc/cephfs: Update docs to match remove functionality and respective …

7d3da6e

…vxattrs Signed-off-by: Christopher Hoffman <choffman@redhat.com>

PendingReleaseNotes: add note about CephFS set_vxattrs

f5fe6a5

Signed-off-by: Christopher Hoffman <choffman@redhat.com>

chrisphoffman force-pushed the wip-62793 branch from 0b3931a to f5fe6a5 Compare May 30, 2024 12:19

chrisphoffman added the needs-test label May 30, 2024

vshankar added the wip-vshankar-testing3 label Jun 3, 2024

vshankar approved these changes Jun 24, 2024

View reviewed changes

vshankar merged commit cac7dcd into ceph:main Jun 27, 2024

vshankar removed the wip-vshankar-testing3 label Jun 27, 2024

This was referenced Jun 27, 2024

pacific: mds: Implement remove for ceph vxattrs #58325

Closed

reef: mds: Implement remove for ceph vxattrs #58350

Merged

quincy: mds: Implement remove for ceph vxattrs #58351

Closed

chrisphoffman mentioned this pull request Nov 15, 2024

squid: mds: Implement remove for ceph vxattrs #60752

Merged

Conversation

chrisphoffman commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

chrisphoffman commented Oct 3, 2023

Uh oh!

dparmar18 commented Oct 3, 2023

Uh oh!

chrisphoffman commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dparmar18 commented Oct 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 10, 2023

Uh oh!

chrisphoffman commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dparmar18 Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vshankar commented Jun 3, 2024

Uh oh!

vshankar commented Jun 3, 2024

Uh oh!

chrisphoffman commented Jun 10, 2024

Uh oh!

chrisphoffman commented Jun 10, 2024

Uh oh!

chrisphoffman commented Jun 10, 2024

Uh oh!

chrisphoffman commented Jun 10, 2024

Uh oh!

chrisphoffman commented Jun 11, 2024

Uh oh!

chrisphoffman commented Jun 11, 2024

Uh oh!

vshankar commented Jun 17, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

chrisphoffman commented Jun 24, 2024

Uh oh!

vshankar commented Jun 25, 2024

Uh oh!

chrisphoffman commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chrisphoffman commented Oct 2, 2023 •

edited

Loading

chrisphoffman commented Oct 3, 2023 •

edited

Loading

chrisphoffman commented Oct 10, 2023 •

edited

Loading

dparmar18 Oct 10, 2023 •

edited

Loading