mds: fix session/client evict command.#49974
Conversation
3538650 to
e2ccb2f
Compare
e2ccb2f to
4108806
Compare
|
So this PR makes a pretty fundamental change to how admin sockets command parsing works: namely, it refuses to pass commands to the admin socket if there are "--" options in the command line. Are people okay with this? If not, we will need to handle stuff like that in every individual daemon, or build a more complicated harness that detects stuff like "--help" and then...doesn't invoke the command. |
The |
maybe we can add an exception here for value |
idryomov
left a comment
There was a problem hiding this comment.
So this PR makes a pretty fundamental change to how admin sockets command parsing works: namely, it refuses to pass commands to the admin socket if there are "--" options in the command line. Are people okay with this?
Absolutely not. I don't think admin socket command accept --foo options but this breaks use cases like ceph --cluster my-other-cluster daemon osd.0 perf dump, etc.
Okay, if there are use cases which uses ceph flags with daemon commands like |
|
Spoke to @neesingh-rh regarding this - the We could add filter checks in ceph.in but that doesn't look neat (those need to be updated/maintained if we add/remove asok commands). |
|
I also checked the daemon command along with the ceph flags which need arguments to be passed , its buggy with them too ( not with all but some of them atleast). |
I guess this happens since the asok command is tried before even connecting (and authenticating the user) to the cluster. EDIT: BTW, this has helped us in situations where for some reason its not possible to connect to the cluster but we want to invoke a command (in the mds), so using asok was immensely helpful. |
|
Looks like we never made progress on this since nothing was finalised w.r.t. the approach to fix this. I've also lost some context on this - will revisit asap. |
|
Discussed this with @neesingh-rh - One approach would be to add |
Yup, sounds reasonable to me. |
|
Would be good to have tests for this |
77370fd to
c18004b
Compare
batrick
left a comment
There was a problem hiding this comment.
Please ping me on slack when you've addressed this.
qa/tasks/cephfs/test_misc.py
Outdated
| self.assertEqual(len(info), 0) # multiple clients are evicted | ||
|
|
||
| def test_session_evict(self): | ||
| self._session_client_evict_without_filter(['session']) |
There was a problem hiding this comment.
Something like:
@classhook('_add_session_evictions')
class TestSessionClientEvict
...
def _add_session_evictions(cls):
tests = [
"_session_client_evict_without_filter"
...
]
def create_test(t, cmd):
def test(self):
getattr(self, t)([cmd])
for t in tests:
setattr(cls, 'test_session_'+t, create_test(t, ['session']))
setattr(cls, 'test_client_'+t, create_test(t, ['client']))
(fix formatting but that's roughly what it'd look like)
batrick
left a comment
There was a problem hiding this comment.
mds: fix client evict command needs to explain the change. The title would be better as mds: require filter for evict command.
qa: delete the symbolic link 1-mds-2-client.yaml needs an explanation in the commit message.
qa: add test for fix of client/session evict command looks good.
Please post results from a vstart_runner.py run to confirm the QA tests work as expected before requesting general QA.
This commit fixes the issues in client/session evict command using below solutions: (1) client evict without filter is forbidden (2) SessionFilter::parse is modified to support id=* (id=0 is not allowed) (3) Invalid id error is handled User can use client evict id=* to evict all clients. Fixes: https://tracker.ceph.com/issues/58619 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Since the tests in the class test_misc.TestSessionClientEvict requires 3 clients, this should be run with a yaml that configures 3 clients. Hence, removing the link to 1-mds-2-client.yaml from qa/suits/fs/multiclient/clusters/ Fixes: https://tracker.ceph.com/issues/58619 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Adds a test class test_misc.TestSessionClientEvict which contains test for the issues mentioned in this PR. Fixes: https://tracker.ceph.com/issues/58619 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
|
vstart_runner test results: |
|
jenkins test api |
|
jenkins test api |
vshankar
left a comment
There was a problem hiding this comment.
LGTM (TIL about classhook :)
|
This PR is under test in https://tracker.ceph.com/issues/66521. |
Daemon commands are getting executed ignoring the ceph flags
like
--help, --statusetc.For eg:
ceph --admin-daemon $socketfile client evict --helpis runignoring the
--helpflag which the user doesn't want and can bedangerous sometimes as with the above command which will
terminate all active connections which isn't supposed to be.
This PR fixes the issues using these solutions:
(1) client evict without filter is forbidden
(2) SessionFilter::parse is modified to support id=* (id=0 is not allowed)
(3) Invalid id error is handled
User can use client evict id=* to evict all clients.
Fixes: https://tracker.ceph.com/issues/58619
Signed-off-by: Neeraj Pratap Singh neesingh@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
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows