pybind/ceph_argparse: Fix error message for ceph tell command by neesingh-rh · Pull Request #51332 · ceph/ceph

neesingh-rh · 2023-05-03T11:32:18Z

$ceph tell cephfs_1.magna023.yvyxvl client ls
error handling command target: local variable 'poolid' referenced before assignment

The error message local variable 'poolid' referenced before assignment looks not descriptive. The exception was not getting raised by the usage of ArgumentValid which was getting bypassed and hence code asks for poolid when no type has been provided in tell commands.

Now after this fix, we are getting the proper error message as type is unknown for above example.

Fixes: https://tracker.ceph.com/issues/59624
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

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

neesingh-rh · 2023-05-03T11:35:27Z

After experimenting with all the classes inheriting CephArgtype in src/pybind/ceph_argparse.py for exception , I found this works with JsonFormat only. Another option that I found is to directly raise ValueError or TypeError, it works with them too.

tchaikov · 2023-06-09T06:06:38Z

src/pybind/ceph_argparse.py

            t, i = s.split('.', 1)
            if t not in ('osd', 'mon', 'client', 'mds', 'mgr'):
-                raise ArgumentValid('unknown type ' + t)
+                raise JsonFormat('unknown type ' + t)


in addition to the outcome of the fix, please explain the reason why this change fixes the symptom you were seeing in the commit message. and a test case would be great.

per the docstring of JsonFormat

some syntactic or semantic issue with the JSON

but this is not a JSON related error. so i am inclined not using JsonFormat here.

Yes, I'm left confused by this as well. Perhaps ArgumentValid was caught up the stack and this JsonFormat avoids that? This looks hacky.

Yup! I went through the code again and re-debugged each flows, where I found that when find_cmd_target() is trying to validate the command by calling CephName and CephPgid, it is failing in the valid() of CephPgid where it is trying to access a variable poolid in the except clause, which isn't assigned yet due to error there. Now, the code works as expected and we are getting the proper error message for wrong cephname type i.e. unknown type ....

neesingh-rh · 2023-08-24T07:32:42Z

jenkins retest this please

tchaikov

$ceph tell cephfs_1.magna023.yvyxvl client ls
error handling command target: local variable 'poolid' referenced before assignment

the error message local variable 'poolid' referenced before assignment looks not descriptive.
The exception was not getting raised by the usage of ArgumentValid and hence code asks for poolid which is

what's the connection between ArgumentValid and poolid?

i still don't get why / where poolid is referenced

tchaikov · 2023-08-25T14:47:08Z

src/pybind/ceph_argparse.py

            t, i = s.split('.', 1)
            if t not in ('osd', 'mon', 'client', 'mds', 'mgr'):
-                raise ArgumentValid('unknown type ' + t)
+                raise JsonFormat('unknown type ' + t)


per the docstring of JsonFormat

some syntactic or semantic issue with the JSON

but this is not a JSON related error. so i am inclined not using JsonFormat here.

tchaikov · 2023-08-25T14:47:16Z

src/pybind/ceph_argparse.py

                        int(i)
                    except ValueError:
-                        raise ArgumentFormat(f'osd id {i} not integer')
+                        raise JsonFormat(f'osd id {i} not integer')


src/test/pybind/test_ceph_argparse.py

tchaikov · 2023-08-25T14:57:48Z

Error message is not descriptive for ceph tell ……command

the title of the commit message reads like a bug report. ideally, the title of the commit message should use the imperative mood. see also https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes . please use git commit --amend and git push -f for revising the commit message and its title.

batrick · 2023-10-17T15:25:05Z

src/test/pybind/test_ceph_argparse.py

+            validate_command(sigdict, ['tell', 'cephfs.c', 'something'])
+        except JsonFormat as er:
+            pass
+        self._assert_valid_command(['tell', 'mon.a', 'something'])


This should be a separate test.

src/test/pybind/test_ceph_argparse.py

batrick · 2023-10-17T15:27:44Z

src/pybind/ceph_argparse.py

            t, i = s.split('.', 1)
            if t not in ('osd', 'mon', 'client', 'mds', 'mgr'):
-                raise ArgumentValid('unknown type ' + t)
+                raise JsonFormat('unknown type ' + t)


Yes, I'm left confused by this as well. Perhaps ArgumentValid was caught up the stack and this JsonFormat avoids that? This looks hacky.

neesingh-rh · 2023-11-08T11:24:57Z

jenkins test make check

github-actions · 2024-01-07T12:01:57Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

tchaikov · 2024-01-07T13:43:32Z

jenkins test make check

rishabh-d-dave · 2024-07-11T17:11:31Z

Talked with Venky about this PR, he wants approval from core before merging.

vshankar · 2024-07-12T07:25:30Z

Talked with Venky about this PR, he wants approval from core before merging.

Tagging @ljflores

github-actions · 2024-07-15T10:52:43Z

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

tchaikov

lgtm

tchaikov · 2024-07-15T10:56:57Z

@neesingh-rh could you please rebase this change?

Fixes: https://tracker.ceph.com/issues/59624 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>

neesingh-rh · 2024-07-15T11:59:19Z

rebased!!

neesingh-rh · 2024-07-15T12:02:21Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

rishabh-d-dave · 2024-07-19T12:12:23Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

Since RADOS will also be affected by this change, I suppose it's better to do RADOS QA as well? cc @vshankar @ljflores

neesingh-rh · 2024-07-22T09:16:02Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

Since RADOS will also be affected by this change, I suppose it's better to do RADOS QA as well? cc @vshankar @ljflores

@rishabh-d-dave Can we priotize this PR for testing, as we do have BZ targeting 6.1z7 and for this blocker only is tomorrow. If we can by today, otherwise let me know. I will have reply there?

rishabh-d-dave · 2024-07-22T09:26:09Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

Since RADOS will also be affected by this change, I suppose it's better to do RADOS QA as well? cc @vshankar @ljflores

@rishabh-d-dave Can we priotize this PR for testing, as we do have BZ targeting 6.1z7 and for this blocker only is tomorrow. If we can by today, otherwise let me know. I will have reply there?

From CephFS side it's already tested successfully, that's all I can do. Venky and core team are better position to tell whether to merge this or proceed for RADOS QA run.

rishabh-d-dave · 2024-07-22T09:28:04Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

Since RADOS will also be affected by this change, I suppose it's better to do RADOS QA as well? cc @vshankar @ljflores

@vshankar @ljflores This has BZ associated with this, please check if this can be merged.

vshankar · 2024-07-23T07:17:30Z

Talked with Venky about this PR, he wants approval from core before merging.

@rishabh-d-dave We do have an approval from @tchaikov ,it is ready to merge.

Since RADOS will also be affected by this change, I suppose it's better to do RADOS QA as well? cc @vshankar @ljflores

@vshankar @ljflores This has BZ associated with this, please check if this can be merged.

@neesingh-rh mentioned that @tchaikov has approved this, so IMO this is good to merge.

rishabh-d-dave · 2024-07-23T12:07:34Z

@vshankar Had a chat with Venky about this PR, we need to run other QA suites as well.

rishabh-d-dave · 2024-07-23T12:09:29Z

@vshankar Had a chat with Venky about this PR, we need to run other QA suites as well.

@yuriw This PR has passed CephFS QA suits and has been approved by @tchaikov. Can you please include it in your QA runs?

vshankar · 2024-08-01T06:49:17Z

@vshankar Had a chat with Venky about this PR, we need to run other QA suites as well.

@yuriw This PR has passed CephFS QA suits and has been approved by @tchaikov. Can you please include it in your QA runs?

Had a conversation with @neesingh-rh regarding this change. This started out as a different change, but ended up just as a fix in the error message. IMO, this is good to merge.

Comments addressed.

github-actions bot added the pybind label May 3, 2023

vshankar added the needs-qa label May 3, 2023

vshankar requested review from a team May 3, 2023 12:07

gregsfortytwo added the needs-review label May 31, 2023

neesingh-rh requested a review from tchaikov June 7, 2023 07:37

tchaikov reviewed Jun 9, 2023

View reviewed changes

vshankar requested a review from a team June 15, 2023 06:25

neesingh-rh force-pushed the wip_59624 branch from 5cc7cca to 4da750a Compare June 21, 2023 09:18

github-actions bot added the tests label Jun 21, 2023

neesingh-rh force-pushed the wip_59624 branch from 4da750a to 1d5960a Compare June 21, 2023 10:13

neesingh-rh requested a review from tchaikov July 4, 2023 10:44

neesingh-rh force-pushed the wip_59624 branch from 1d5960a to 0f51b49 Compare July 4, 2023 10:49

tchaikov requested changes Aug 25, 2023

View reviewed changes

tchaikov reviewed Aug 25, 2023

View reviewed changes

src/test/pybind/test_ceph_argparse.py Outdated Show resolved Hide resolved

neesingh-rh force-pushed the wip_59624 branch 2 times, most recently from d04de87 to 72da79f Compare August 29, 2023 11:58

neesingh-rh changed the title ~~pybind/ceph_argparse: Error message is not descriptive for ceph tell command~~ pybind/ceph_argparse: Fix error message for ceph tell command Aug 29, 2023

batrick previously requested changes Oct 17, 2023

View reviewed changes

neesingh-rh force-pushed the wip_59624 branch from 72da79f to 2dd6578 Compare November 2, 2023 04:39

neesingh-rh requested review from batrick and tchaikov November 2, 2023 04:56

github-actions bot added the stale label Jan 7, 2024

tchaikov removed the stale label Jan 7, 2024

rishabh-d-dave removed needs-qa wip-rishabh-testing Rishabh's testing label labels Jul 11, 2024

rishabh-d-dave requested a review from a team July 11, 2024 17:11

github-actions bot added the needs-rebase label Jul 15, 2024

tchaikov approved these changes Jul 15, 2024

View reviewed changes

neeraj pratap singh added 2 commits July 15, 2024 17:23

pybind/ceph_argparse: fixing error message for ceph tell command

2811bb0

Fixes: https://tracker.ceph.com/issues/59624 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>

qa: add test for ceph tell with unknown cephtype

decf32e

Fixes: https://tracker.ceph.com/issues/59624 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>

neesingh-rh force-pushed the wip_59624 branch from e0f0afa to decf32e Compare July 15, 2024 11:59

github-actions bot removed the needs-rebase label Jul 15, 2024

rishabh-d-dave added the needs-qa label Jul 23, 2024

vshankar approved these changes Aug 1, 2024

View reviewed changes

vshankar merged commit f4b5465 into ceph:main Aug 1, 2024

This was referenced Aug 2, 2024

quincy: pybind/ceph_argparse: Fix error message for ceph tell command #59004

Closed

squid: pybind/ceph_argparse: Fix error message for ceph tell command #59005

Merged

neesingh-rh mentioned this pull request Aug 13, 2024

reef: pybind/ceph_argparse: Fix error message for ceph tell command #59197

Merged

Conversation

neesingh-rh commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

neesingh-rh commented May 3, 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!

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!

neesingh-rh commented Aug 24, 2023

Uh oh!

tchaikov 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!

Uh oh!

tchaikov commented Aug 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neesingh-rh commented Nov 8, 2023

Uh oh!

github-actions bot commented Jan 7, 2024

Uh oh!

tchaikov commented Jan 7, 2024

Uh oh!

rishabh-d-dave commented Jul 11, 2024

Uh oh!

vshankar commented Jul 12, 2024

Uh oh!

github-actions bot commented Jul 15, 2024

Uh oh!

tchaikov left a comment

Choose a reason for hiding this comment

Uh oh!

tchaikov commented Jul 15, 2024

Uh oh!

neesingh-rh commented Jul 15, 2024

Uh oh!

neesingh-rh commented Jul 15, 2024

Uh oh!

rishabh-d-dave commented Jul 19, 2024

Uh oh!

neesingh-rh commented Jul 22, 2024

Uh oh!

rishabh-d-dave commented Jul 22, 2024

Uh oh!

rishabh-d-dave commented Jul 22, 2024

Uh oh!

vshankar commented Jul 23, 2024

Uh oh!

rishabh-d-dave commented Jul 23, 2024

Uh oh!

rishabh-d-dave commented Jul 23, 2024

Uh oh!

vshankar commented Aug 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

neesingh-rh commented May 3, 2023 •

edited

Loading

neesingh-rh commented May 3, 2023 •

edited

Loading