client: check mds down status before getting mds_gid_t from mdsmap by YiteGu · Pull Request #52258 · ceph/ceph

YiteGu · 2023-06-29T13:53:43Z

Get mds_gid_t from up of MDSMap, will cause to core dump if target mds_rank_t does not exist up map:

from: src/mds/MDSMap.h
const auto& get_info(mds_rank_t m) const {
return mds_info.at(up.at(m));
}

reproduct:

ceph fs fail <fs_name>
curl -X GET "example.com:8080/api/cephfs/1/clients" ...
up.at(m) will cause to core dump.

Fixes: https://tracker.ceph.com/issues/61844
Singed-off-by: Yite Gu yitegu0@gmail.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

YiteGu · 2023-06-29T14:00:11Z

Hi @batrick have a code review

dparmar18 · 2023-06-30T07:16:43Z

src/client/Client.cc

+    if (fsmap->get_filesystem(role.fscid)->mds_map.is_down(role.rank)) {
+      lderr(cct) << __func__ << ": targets rank: " << role.rank
+                 << " is down" << dendl;
+      return -CEPHFS_ENOENT;


I think a better return value would be -CEPHFS_EPERM as we want to say "resolving mds is not allowed when the mds is down".

else other changes looks good.

dparmar18 · 2023-06-30T07:17:27Z

Since you have steps to reproduce this; can we have a test case?

YiteGu · 2023-07-01T18:15:38Z

@dparmar18 Okay, but I lack experience of write test cases, could you provide me an example?

dparmar18 · 2023-07-03T09:44:01Z

@dparmar18 Okay, but I lack experience of write test cases, could you provide me an example?

I'll check and see if i can find one because (at least) I haven't came across any test cases where a restful API is used to check MDS.

YiteGu · 2023-07-03T10:30:20Z

@dparmar18 Okay, but I lack experience of write test cases, could you provide me an example?

I'll check and see if i can find one because (at least) I haven't came across any test cases where a restful API is used to check MDS.

You could directly write a test case to associate this fix, I am looking forward to it very much :)

dparmar18 · 2023-07-04T11:51:18Z

can someone from @ceph/dashboard help identify how and where to add a test case for this since it involves querying dashboard

batrick

The convention is to use lower case in the commit message component/title. Please change to: client: check mds down status bofore getting mds_gid_t from mdsmap

src/client/Client.cc

batrick · 2023-07-04T15:12:22Z

You could write a test:

ceph fs set cephfs max_mds 2
ceph fs set cephfs joinable false
ceph mds fail cephfs:1
ceph tell mds.cephfs:1 session ls # would abort

dparmar18 · 2023-07-05T06:34:48Z

You could write a test:

ceph fs set cephfs max_mds 2
ceph fs set cephfs joinable false
ceph mds fail cephfs:1
ceph tell mds.cephfs:1 session ls # would abort

I think @YiteGu can follow the steps he mentioned in tracker and add a api test in https://github.com/ceph/ceph/blob/main/qa/tasks/mgr/dashboard/test_cephfs.py, it would be as simple as

self.fs.fail()
request mds status using GET - self._get()

thanks to @nizamial09 for helping find the right place.

qa/tasks/mgr/dashboard/test_cephfs.py

github-actions · 2024-02-02T10:43:17Z

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

…fails Fixes: https://tracker.ceph.com/issues/64089 Signed-off-by: Dhairya Parmar <dparmar@redhat.com> (cherry picked from commit 564dba3)

YiteGu · 2024-02-02T11:54:28Z

too-many-lines

for now you can put # pylint: disable=too-many-lines on top of the cephfs.py to disable it
# -*- coding: utf-8 -*-
# pylint: disable=too-many-lines

make check done

vshankar · 2024-02-19T04:43:18Z

https://pulpito.ceph.com/vshankar-2024-02-17_05:57:56-fs-wip-vshankar-testing-20240217.015652-testing-default-smithi/

This run still has quay fetch failures

Command failed on smithi155 with status 1: 'sudo /home/ubuntu/cephtest/cephadm --image quay-quay-quay.apps.os.sepia.ceph.com/ceph-ci/ceph:1c920afa8316ea21877f3186850254947531ea22 pull'

I'll file a tracker for that and merge this batch.

* refs/pull/52258/head: client: check mds down status bofore getting mds_git_t from mdsmap mgr/dashboard: allow sending back error status code fetching clients fails Reviewed-by: Dhairya Parmar <dparmar@redhat.com> Reviewed-by: Xiubo Li <xiubli@redhat.com> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

rishabh-d-dave

@YiteGu Since the PR has been tested and approved already, feel free to skip ignore thi nits.

qa/tasks/mgr/dashboard/test_cephfs.py

src/pybind/mgr/dashboard/controllers/cephfs.py

YiteGu · 2024-02-22T17:34:36Z

@YiteGu Since the PR has been tested and approved already, feel free to skip ignore thi nits.

done, thanks. :)

vshankar · 2024-02-26T06:01:09Z

https://pulpito.ceph.com/vshankar-2024-02-17_05:57:56-fs-wip-vshankar-testing-20240217.015652-testing-default-smithi/

This run still has quay fetch failures

Command failed on smithi155 with status 1: 'sudo /home/ubuntu/cephtest/cephadm --image quay-quay-quay.apps.os.sepia.ceph.com/ceph-ci/ceph:1c920afa8316ea21877f3186850254947531ea22 pull'

I'll file a tracker for that and merge this batch.

Unfortunately it was risky to merge the change without a proper run. The fix for this is under test.

vshankar · 2024-02-26T10:53:22Z

https://pulpito.ceph.com/?branch=wip-vshankar-testing-20240216.060239

mchangir · 2024-02-26T11:15:41Z

@YiteGu nit: I think the typedef is named mds_gid_t and not mds_git_t
please update the:

PR title
PR Description
Commit title
Commit message
appropriately

Get mds_gid_t from up of MDSMap, will cause to core dump if target mds_rank_t does not exist up map: from: src/mds/MDSMap.h const auto& get_info(mds_rank_t m) const { return mds_info.at(up.at(m)); } reproduct: 1. ceph fs fail <fs_name> 2. curl -X GET "example.com:8080/api/cephfs/1/clients" ... up.at(m) will cause to core dump. Fixes: https://tracker.ceph.com/issues/61844 Signed-off-by: Yite Gu <yitegu0@gmail.com>

YiteGu · 2024-02-26T11:57:19Z

@YiteGu nit: I think the typedef is named mds_gid_t and not mds_git_t please update the:

PR title

PR Description

Commit title

Commit message
appropriately

done

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#26th-Feb-2024

vshankar · 2024-02-26T16:15:55Z

jenkins test dashboard

vshankar · 2024-02-27T05:57:44Z

@YiteGu Could you please get the failing dashboard jenkins test resolved. Its not a mandatory check to merge a change, however I'd like to be certain that this change does not break dashboard tests.

YiteGu · 2024-02-27T06:22:03Z

@YiteGu Could you please get the failing dashboard jenkins test resolved. Its not a mandatory check to merge a change, however I'd like to be certain that this change does not break dashboard tests.

I remember @nizamial09 said we could ignore the ceph dashboard tests and ceph dashboard cephadm e2e tests, pls help our check this problem again.

nizamial09 · 2024-02-27T07:24:08Z

we could ignore the ceph dashboard tests and ceph dashboard cephadm e2e tests,

I raised a fix for the dashboard e2e: #55771

and as for the cephadm e2e, its mostly grafana related failures which will be gone when we have a new grafana image or it will go away when this PR gets merged (which removes the need of building grafana image whenever new grafana dashboards are delivered,.

This PR is not affected by the dashboard or dashboard cephadm e2es

YiteGu · 2024-02-27T08:27:04Z

@vshankar we could continue now :)

github-actions bot added the cephfs Ceph File System label Jun 29, 2023

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 2df373b to 812f910 Compare June 29, 2023 13:57

dparmar18 reviewed Jun 30, 2023

View reviewed changes

dparmar18 requested a review from a team June 30, 2023 07:18

dparmar18 changed the title ~~Client: Check mds down status bofore get mds_git_t from up~~ Client: Check mds down status before getting mds_git_t from mds up map Jun 30, 2023

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 812f910 to 17f037e Compare July 1, 2023 17:46

batrick requested changes Jul 4, 2023

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 17f037e to e017e62 Compare July 5, 2023 01:40

YiteGu changed the title ~~Client: Check mds down status before getting mds_git_t from mds up map~~ client: check mds down status before getting mds_git_t from mdsmap Jul 5, 2023

YiteGu force-pushed the check-mds-down-while-get-from-up branch from e017e62 to e62e690 Compare July 5, 2023 14:00

YiteGu requested a review from a team as a code owner July 5, 2023 14:00

YiteGu requested review from avanthakkar and cloudbehl and removed request for a team July 5, 2023 14:00

github-actions bot added dashboard tests labels Jul 5, 2023

YiteGu requested review from batrick and dparmar18 July 5, 2023 14:10

YiteGu force-pushed the check-mds-down-while-get-from-up branch 2 times, most recently from 2ab9f59 to d3de04c Compare July 5, 2023 16:53

batrick reviewed Jul 5, 2023

View reviewed changes

qa/tasks/mgr/dashboard/test_cephfs.py Show resolved Hide resolved

batrick approved these changes Jul 5, 2023

View reviewed changes

github-actions bot added the needs-rebase label Feb 2, 2024

mgr/dashboard: allow sending back error status code fetching clients …

e99486e

…fails Fixes: https://tracker.ceph.com/issues/64089 Signed-off-by: Dhairya Parmar <dparmar@redhat.com> (cherry picked from commit 564dba3)

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 746917a to 67378bd Compare February 2, 2024 10:48

github-actions bot removed the needs-rebase label Feb 2, 2024

vshankar added the wip-vshankar-testing label Feb 6, 2024

rishabh-d-dave approved these changes Feb 22, 2024

View reviewed changes

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 67378bd to 177491e Compare February 22, 2024 17:33

YiteGu force-pushed the check-mds-down-while-get-from-up branch from 177491e to 878463e Compare February 26, 2024 11:55

YiteGu changed the title ~~client: check mds down status before getting mds_git_t from mdsmap~~ client: check mds down status before getting mds_gid_t from mdsmap Feb 26, 2024

vshankar approved these changes Feb 26, 2024

View reviewed changes

nizamial09 approved these changes Feb 27, 2024

View reviewed changes

vshankar merged commit 5dbd3bf into ceph:main Feb 28, 2024

vshankar removed the wip-vshankar-testing label Feb 28, 2024

YiteGu mentioned this pull request Jul 15, 2024

squid: client: check mds down status before getting mds_gid_t from mdsmap #58587

Merged

YiteGu mentioned this pull request Dec 4, 2024

quincy: client: check mds down status before getting mds_gid_t from mdsmap #58505

Closed

14 tasks

Conversation

YiteGu commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

YiteGu commented Jun 29, 2023

Uh oh!

dparmar18 Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

dparmar18 Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

YiteGu Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Jun 30, 2023

Uh oh!

YiteGu commented Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dparmar18 commented Jul 3, 2023

Uh oh!

YiteGu commented Jul 3, 2023

Uh oh!

dparmar18 commented Jul 4, 2023

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

batrick commented Jul 4, 2023

Uh oh!

dparmar18 commented Jul 5, 2023

Uh oh!

Uh oh!

github-actions bot commented Feb 2, 2024

Uh oh!

YiteGu commented Feb 2, 2024

Uh oh!

vshankar commented Feb 19, 2024

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YiteGu commented Feb 22, 2024

Uh oh!

vshankar commented Feb 26, 2024

Uh oh!

vshankar commented Feb 26, 2024

Uh oh!

mchangir commented Feb 26, 2024

Uh oh!

YiteGu commented Feb 26, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Feb 26, 2024

Uh oh!

vshankar commented Feb 27, 2024

Uh oh!

YiteGu commented Feb 27, 2024

Uh oh!

nizamial09 commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YiteGu commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

YiteGu commented Jun 29, 2023 •

edited

Loading

YiteGu commented Jul 1, 2023 •

edited

Loading

nizamial09 commented Feb 27, 2024 •

edited

Loading