mds: check relevant caps for fs include root_squash by batrick · Pull Request #57192 · ceph/ceph

batrick · 2024-05-01T01:45:42Z

I'm not satisfied with this approach completely:

There should be a way to override the MDS and allow the broken root_squash caps to continue working. Evicting all affected clients is not acceptable.
Setting the client feature bit on the MDSMap has no effect.

Will think about this more tomorrow and start some discussions with the team.

Checklist

Tracker (select at least one)
- References tracker ticket
Component impact
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
Tests (select at least one)
- Includes unit test(s)

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

batrick · 2024-05-03T01:07:08Z

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

github-actions · 2024-05-03T09:11:28Z

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

leonid-s-usov

A speedy turnaround with this one, great job, Patrick!
As usual, I'm cautious about adding a managed cache to the internal state where const calculation is cheap. We can afford to scan a list of a few tens of sessions once in 5 seconds and thus avoid inflation of the managed state and potential bugs this can bring.

src/mds/SessionMap.h

leonid-s-usov · 2024-05-03T11:20:13Z

src/mds/mdstypes.cc

-    n += bits_per_block;
-    n /= bits_per_block;
-    _vec.resize(n, 0);
+     n += bits_per_block;


nit: something's wrong with the indentation in this function

Yes, I'll fix it.

src/mds/MDSAuthCaps.h

qa/tasks/cephfs/test_admin.py

chrisphoffman · 2024-05-03T18:55:01Z

doc/cephfs/health-messages.rst

+    and a client upgrade is required.
+
+    This is a HEALTH_ERR warning because of the danger of inconsistency and lost
+    data. It is recommended to either upgrade your clients, discontinue using


lost data

What sort of scenario would this happen?

https://tracker.ceph.com/issues/57154

actually: https://tracker.ceph.com/issues/56067

rishabh-d-dave · 2024-05-04T04:28:44Z

@batrick I know it's going to be squashed so it doesn't matter, but sign-off in last commit is missing. Signed-off-by check in CI failed due to it

lxbsz

LGTM.

rishabh-d-dave · 2024-05-06T17:35:56Z

qa/tasks/cephfs/mount.py

        self.update_attrs(**kwargs)

-        retval = self.mount(mntopts=mntopts, check_status=check_status)
+        retval = self.mount(mntopts=mntopts, check_status=check_status, **kwargs)


Passing kwargs is redundant because each k-v pair in kwargs are set object attributes by update_attrs()

See commit message in qa: pass kwargs to mount from remount.

rishabh-d-dave · 2024-05-06T17:45:09Z

qa/tasks/cephfs/test_admin.py

+        # should succeed
+        with self.assert_cluster_log("with broken root_squash implementation"):
+            keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
+            self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs.name)


nit: same, will be better if wrap such lines before 81 chars.

rishabh-d-dave · 2024-05-06T17:45:17Z

qa/tasks/cephfs/test_admin.py

+
+        CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK = 21
+        # all but CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK
+        features = ",".join([str(i) for i in range(CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK)])


nit: same, will be better if wrap such lines before 81 chars.

rishabh-d-dave · 2024-05-06T17:48:38Z

qa/tasks/cephfs/test_admin.py


+    def test_rootsquash_nofeature(self):
+        """
+        That having root_squash on an fs without the feature bit raises an HEALTH_ERR.


How this line would currently look in teuthology.log -

2024-05-01T05:49:57.520 INFO:tasks.cephfs_test_runner:Starting test: test_add_already_in_use_data_pool (tasks.cephfs.test_admin.TestAddDataPool) That command try to add data pool which is already in use with another fs.

So I suggest slight modification -

Suggested change

That having root_squash on an fs without the feature bit raises an HEALTH_ERR.

Test that having root_squash on an fs without the feature bit raises an HEALTH_ERR.

"Test ..." is implicit in this construction. I'm using the style begun by John Spray.

Okay, but it's not printed in teuthology.log. I've copied an example from recent run.

Anyways, I don't really mind. Since this PR is urgent, I've approved it regardless of these nits.

rishabh-d-dave · 2024-05-06T17:49:21Z

qa/tasks/cephfs/test_admin.py

        # should succeed
-        keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
-        self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs1.name)
+        with self.assert_cluster_log("report clients with broken root_squash", present=False):


nit: same, will be better if wrap such lines before 81 chars.

rishabh-d-dave · 2024-05-06T17:49:28Z

qa/tasks/cephfs/test_admin.py

-        self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs1.name)
+        with self.assert_cluster_log("report clients with broken root_squash", present=False):
+            keyring_path = self.mount_a.client_remote.mktemp(data=keyring)
+            self.mount_a.remount(client_id=self.client_id, client_keyring_path=keyring_path, mntargs=mntargs, cephfs_name=self.fs1.name)


nit: same, will be better if wrap such lines before 81 chars.

batrick · 2024-05-06T18:28:06Z

Folks, I'm not considering further nits on this PR in the interest of time as this PR is blocking v18.2.3.

batrick · 2024-05-06T18:28:26Z

I will be rebasing and squashing fixes now.

For testing purposes. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

* refs/pull/57192/head: PendingReleaseNotes: add note on the client incompatibility health warning and feature bit doc/cephfs: add client_mds_auth_caps client feature bit doc/cephfs: add missing client feature bits doc/cephfs: document MDS_CLIENTS_BROKEN_ROOTSQUASH health error qa: add tests for MDS_CLIENTS_BROKEN_ROOTSQUASH mds: raise health warning if client lacks feature for root_squash mon/MDSMonitor: add note about missing metadata inclusion mds: check relevant caps for fs include root_squash mds: refactor out fs_name match in MDSAuthCaps qa: test for root_squash with multiple caps qa: pass kwargs to mount from remount qa: simplify update_attrs and only update relevant keys client: allow overriding client features

vshankar · 2024-05-07T05:33:59Z

Folks, I'm not considering further nits on this PR in the interest of time as this PR is blocking v18.2.3.

Agreed and +1 for this.

So we can just pass the caller's kwargs to update_attrs. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

So we can pass mntargs. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Where the client has root_squash for one cap but not for another. The fs without root_squash should not necessarily reject the client. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

When denying client reconnects because the MDS caps include root_squash and the client features do not include CEPHFS_FEATURE_MDS_AUTH_CAPS_CHECK, ensure those caps are only for the file system the MDS is joined to. Fixes: https://tracker.ceph.com/issues/65733 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

There is a "client_count" metadata on the health warning that apparently was intended to be used for aggregating warnings but never was. Add a TODO item for that. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Rather than evict all clients lacking this feature bit, raise a health error that pushes the administrator to address it. This avoids the surprise of having all affected clients suddenly evicted in the cluster. Fixes: https://tracker.ceph.com/issues/65733 Fixes: 954ed30 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

…rning and feature bit Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick · 2024-05-07T14:13:18Z

jenkins test make check arm64

batrick · 2024-05-07T14:15:56Z

https://tracker.ceph.com/issues/65771#note-3

batrick · 2024-05-07T17:21:45Z

jenkins test make check

batrick · 2024-05-07T17:22:18Z

jenkins test make check arm64

batrick added the cephfs Ceph File System label May 1, 2024

batrick force-pushed the i65733 branch from ebed82e to 5512a1b Compare May 2, 2024 02:57

github-actions bot added common tests labels May 2, 2024

batrick force-pushed the i65733 branch from 5512a1b to 1ba32ca Compare May 3, 2024 00:54

github-actions bot added core documentation mon labels May 3, 2024

batrick force-pushed the i65733 branch from 1ba32ca to f1819f5 Compare May 3, 2024 00:56

batrick marked this pull request as ready for review May 3, 2024 00:57

batrick requested review from a team as code owners May 3, 2024 00:57

batrick requested review from a team, lxbsz and vshankar May 3, 2024 00:57

batrick added the wip-pdonnell-testing label May 3, 2024

github-actions bot added the needs-rebase label May 3, 2024

leonid-s-usov reviewed May 3, 2024

View reviewed changes

batrick force-pushed the i65733 branch from 6fedbe0 to a459d91 Compare May 3, 2024 13:23

github-actions bot removed the needs-rebase label May 3, 2024

batrick requested a review from leonid-s-usov May 3, 2024 13:25

batrick force-pushed the i65733 branch from 4e7ed9b to 9d258f7 Compare May 3, 2024 16:35

rishabh-d-dave reviewed May 3, 2024

View reviewed changes

src/mds/MDSAuthCaps.h Outdated Show resolved Hide resolved

src/mds/MDSAuthCaps.h Outdated Show resolved Hide resolved

src/mds/MDSAuthCaps.h Outdated Show resolved Hide resolved

batrick requested a review from rishabh-d-dave May 3, 2024 18:27

chrisphoffman reviewed May 3, 2024

View reviewed changes

batrick force-pushed the i65733 branch from 3ea971f to 2f88110 Compare May 4, 2024 13:55

lxbsz approved these changes May 6, 2024

View reviewed changes

batrick requested a review from chrisphoffman May 6, 2024 15:22

rishabh-d-dave approved these changes May 6, 2024

View reviewed changes

client: allow overriding client features

d9239f9

For testing purposes. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick force-pushed the i65733 branch from 355c2ff to db761e1 Compare May 6, 2024 18:33

batrick added 12 commits May 7, 2024 08:19

qa: simplify update_attrs and only update relevant keys

597ff3c

So we can just pass the caller's kwargs to update_attrs. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa: pass kwargs to mount from remount

afcbfc0

So we can pass mntargs. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa: test for root_squash with multiple caps

bccc8ce

Where the client has root_squash for one cap but not for another. The fs without root_squash should not necessarily reject the client. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

mds: refactor out fs_name match in MDSAuthCaps

247b4fe

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

mon/MDSMonitor: add note about missing metadata inclusion

6517b70

There is a "client_count" metadata on the health warning that apparently was intended to be used for aggregating warnings but never was. Add a TODO item for that. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

qa: add tests for MDS_CLIENTS_BROKEN_ROOTSQUASH

9d0ab23

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

doc/cephfs: document MDS_CLIENTS_BROKEN_ROOTSQUASH health error

b810bc9

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

doc/cephfs: add missing client feature bits

79ba897

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

doc/cephfs: add client_mds_auth_caps client feature bit

536b47c

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

PendingReleaseNotes: add note on the client incompatibility health wa…

e70f005

…rning and feature bit Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick force-pushed the i65733 branch from db761e1 to e70f005 Compare May 7, 2024 12:19

batrick merged commit b54c9e8 into ceph:main May 7, 2024

batrick deleted the i65733 branch May 7, 2024 19:49

This was referenced May 7, 2024

squid: mds: check relevant caps for fs include root_squash #57342

Merged

reef: mds: check relevant caps for fs include root_squash #57343

Merged

batrick mentioned this pull request May 16, 2024

qa/cephfs: add MDS_CLIENTS_BROKEN_ROOTSQUASH to ignorelist #57528

Merged

14 tasks

	That having root_squash on an fs without the feature bit raises an HEALTH_ERR.
	Test that having root_squash on an fs without the feature bit raises an HEALTH_ERR.

Conversation

batrick commented May 1, 2024 • edited by leonid-s-usov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

batrick commented May 3, 2024

Uh oh!

github-actions bot commented May 3, 2024

Uh oh!

leonid-s-usov left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

rishabh-d-dave commented May 4, 2024

Uh oh!

lxbsz left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batrick commented May 6, 2024

Uh oh!

batrick commented May 6, 2024

Uh oh!

vshankar commented May 7, 2024

Uh oh!

batrick commented May 7, 2024

Uh oh!

batrick commented May 7, 2024

Uh oh!

batrick commented May 7, 2024

Uh oh!

batrick commented May 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

batrick commented May 1, 2024 •

edited by leonid-s-usov

Loading