mgr/volumes: handle bad arguments during subvolume create by mchangir · Pull Request #53989 · ceph/ceph

mchangir · 2023-10-13T05:39:54Z

test subvolgroup and subvol names and declare empty/whitespace strings as bad names

Fixes: https://tracker.ceph.com/issues/58645
Signed-off-by: Milind Changire mchangir@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

mchangir · 2023-10-13T05:41:57Z

this is a new PR to resurrect PR #50028

vshankar · 2023-10-16T07:40:50Z

This changes isn't really an mds change - the PR title is a bit misleading ;)

mchangir · 2023-10-16T11:09:20Z

This changes isn't really an mds change - the PR title is a bit misleading ;)

fixed the PR title

src/pybind/ceph_argparse.py

R-0-Y · 2023-11-23T11:55:38Z

Hi @mchangir , is there any update on this PR ? Is someone working on the make check (arm64) ?
Thanks in advance for your help on this.

mchangir · 2023-11-23T12:31:15Z

jenkins test make check arm64

mchangir · 2023-11-23T15:58:35Z

Hi @mchangir , is there any update on this PR ? Is someone working on the make check (arm64) ? Thanks in advance for your help on this.

@R-0-Y make check (arm64) is not a mandatory test that needs to pass

github-actions · 2023-12-15T14:31:39Z

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

doc/_ext/ceph_commands.py

src/pybind/ceph_argparse.py

qa/tasks/cephfs/test_volumes.py

vshankar · 2024-01-16T11:49:41Z

@mchangir Did we ever run this through tests? Also, I had requested a review from broader folks from other teams a while back since this was a generic change.

mchangir · 2024-01-16T11:52:54Z

@mchangir Did we ever run this through tests? Also, I had requested a review from broader folks from other teams a while back since this was a generic change.

Sorry about that. Probably slipped from my mind.
I'll tag people appropriately to solicit comments.

mchangir · 2024-01-16T12:01:17Z

rebased

mchangir · 2024-01-16T12:02:58Z

@ljflores could you review this PR ... especially the applicability of allowempty attribute to other parts of the codebase.

github-actions · 2024-03-16T13:01:30Z

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.

rishabh-d-dave · 2025-04-22T15:52:05Z

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

rishabh-d-dave · 2025-05-05T07:38:09Z

jenkins test make check arm64

rishabh-d-dave · 2025-05-05T07:38:18Z

jenkins test make check

rishabh-d-dave

QA run[1] had some failures in fs:volumes jobs, but they were unrelated to this PR. I re-ran fs:volumes suite with a branch that contained only this PR for extra confirmation, see [2][3]. All the jobs passed, therefore this PR has completely passed QA.

[1] https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20250426123842-debug
[2] https://pulpito.ceph.com/rishabh-2025-05-04_19:44:23-fs:volumes-pr-53989-testing-default-smithi/
[3] https://pulpito.ceph.com/rishabh-2025-05-05_07:34:59-fs:volumes-pr-53989-testing-default-smithi/

rishabh-d-dave · 2025-05-05T07:45:51Z

Waitng for this CI to pass now.

rishabh-d-dave · 2025-05-05T11:41:50Z

jenkins test make check

rishabh-d-dave · 2025-05-05T18:44:28Z

jenkins test make check

rishabh-d-dave · 2025-05-06T07:53:59Z

jenkins test make check

rishabh-d-dave

Forgot to approve while reporting about (successful) QA run.

Changes requested were incorporated.

VallariAg · 2025-05-12T14:09:11Z

Hi @rishabh-d-dave, this might have created a regression in ceph CLI for latest jenkins main branch builds:

After cephadm bootstrap, "ceph orch host add" command failed:

Traceback (most recent call last):
  File "/usr/bin/ceph", line 1327, in <module>
    retval = main()
  File "/usr/bin/ceph", line 1247, in main
    sigdict = parse_json_funcsigs(outbuf.decode('utf-8'), 'cli')
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 1004, in parse_json_funcsigs
    cmd['sig'] = parse_funcsig(cmd['sig'])
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 948, in parse_funcsig
    newsig.append(argdesc(t,
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 803, in __init__
    self.instance = self.t(**self.typeargs)
TypeError: __init__() got an unexpected keyword argument 'allowempty'

I think the reason we probably didn't catch this in teuthology tests is because teuthology install ceph rpm packages and cephadm binary of same build. So we didn't know that it was a breaking change that would make new builds including this PR incompatible with ceph builds that do not include this PR.

cc: @gbregman

vshankar · 2025-05-13T12:29:39Z

Hi @rishabh-d-dave, this might have created a regression in ceph CLI for latest jenkins main branch builds:

After cephadm bootstrap, "ceph orch host add" command failed:
Traceback (most recent call last):
  File "/usr/bin/ceph", line 1327, in <module>
    retval = main()
  File "/usr/bin/ceph", line 1247, in main
    sigdict = parse_json_funcsigs(outbuf.decode('utf-8'), 'cli')
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 1004, in parse_json_funcsigs
    cmd['sig'] = parse_funcsig(cmd['sig'])
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 948, in parse_funcsig
    newsig.append(argdesc(t,
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 803, in __init__
    self.instance = self.t(**self.typeargs)
TypeError: __init__() got an unexpected keyword argument 'allowempty'
I think the reason we probably didn't catch this in teuthology tests is because teuthology install ceph rpm packages and cephadm binary of same build. So we didn't know that it was a breaking change that would make new builds including this PR incompatible with ceph builds that do not include this PR.

cc: @gbregman

Maybe its not the package version but rather kwargs holding allowempty keyword argument, but some type-specific classes in ceph-argparse (CephInt, CephFloat, etc...), not accepting the argument.

mchangir · 2025-05-14T04:15:46Z

Hi @rishabh-d-dave, this might have created a regression in ceph CLI for latest jenkins main branch builds:
After cephadm bootstrap, "ceph orch host add" command failed:
Traceback (most recent call last):
  File "/usr/bin/ceph", line 1327, in <module>
    retval = main()
  File "/usr/bin/ceph", line 1247, in main
    sigdict = parse_json_funcsigs(outbuf.decode('utf-8'), 'cli')
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 1004, in parse_json_funcsigs
    cmd['sig'] = parse_funcsig(cmd['sig'])
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 948, in parse_funcsig
    newsig.append(argdesc(t,
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 803, in __init__
    self.instance = self.t(**self.typeargs)
TypeError: __init__() got an unexpected keyword argument 'allowempty'
I think the reason we probably didn't catch this in teuthology tests is because teuthology install ceph rpm packages and cephadm binary of same build. So we didn't know that it was a breaking change that would make new builds including this PR incompatible with ceph builds that do not include this PR.
cc: @gbregman
Maybe its not the package version but rather kwargs holding allowempty keyword argument, but some type-specific classes in ceph-argparse (CephInt, CephFloat, etc...), not accepting the argument.

I was also concerned about where does the allowempty keyword get injected for non-CephString data types ... since allowempty is in use only with the CephString data type.

vshankar · 2025-05-14T10:03:53Z

Hi @rishabh-d-dave, this might have created a regression in ceph CLI for latest jenkins main branch builds:

After cephadm bootstrap, "ceph orch host add" command failed:
Traceback (most recent call last):
  File "/usr/bin/ceph", line 1327, in <module>
    retval = main()
  File "/usr/bin/ceph", line 1247, in main
    sigdict = parse_json_funcsigs(outbuf.decode('utf-8'), 'cli')
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 1004, in parse_json_funcsigs
    cmd['sig'] = parse_funcsig(cmd['sig'])
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 948, in parse_funcsig
    newsig.append(argdesc(t,
  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 803, in __init__
    self.instance = self.t(**self.typeargs)
TypeError: __init__() got an unexpected keyword argument 'allowempty'
I think the reason we probably didn't catch this in teuthology tests is because teuthology install ceph rpm packages and cephadm binary of same build. So we didn't know that it was a breaking change that would make new builds including this PR incompatible with ceph builds that do not include this PR.

cc: @gbregman

This issues is seen with fs:upgrade. See: https://pulpito.ceph.com/vshankar-2025-05-12_08:22:09-fs-wip-vshankar-testing-20250508.200127-debug-testing-default-smithi/8281059/

vshankar · 2025-05-14T10:05:35Z

QA run[1] had some failures in fs:volumes jobs, but they were unrelated to this PR. I re-ran fs:volumes suite with a branch that contained only this PR for extra confirmation, see [2][3]. All the jobs passed, therefore this PR has completely passed QA.

[1] https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20250426123842-debug [2] https://pulpito.ceph.com/rishabh-2025-05-04_19:44:23-fs:volumes-pr-53989-testing-default-smithi/ [3] https://pulpito.ceph.com/rishabh-2025-05-05_07:34:59-fs:volumes-pr-53989-testing-default-smithi/

@rishabh-d-dave - I'm see the first link you shared above and that points to QA tracker: https://tracker.ceph.com/issues/71020. The QA run link there does not point to a teuthology run. What's going on?

vshankar · 2025-05-14T10:17:12Z

I am confused as in what tests this change went through since https://tracker.ceph.com/issues/71020 points to a non-existent teuthology run. It's unlikely that ptl-tool messed up the run link while updating the redmine ticket. @rishabh-d-dave please provide information on what happened.

This change is breaking fs:upgrade tests. See: https://pulpito.ceph.com/vshankar-2025-05-12_08:22:09-fs-wip-vshankar-testing-20250508.200127-debug-testing-default-smithi/8281059/

2025-05-12T08:56:36.912 INFO:teuthology.orchestra.run.smithi082.stderr:2025-05-12T08:56:36.911+0000 7f0d1971a640  1 --2- 172.21.15.82:0/4233805895 >> [v2:172.21.15.121:6828/1515519538,v1:172.21.15.121:6829/1515519538] conn(0x7f0cdc076a30 0x7f0
cdc078ef0 secure :-1 s=READY pgs=20 cs=0 l=1 rev1=1 crypto rx=0x7f0d140694d0 tx=0x7f0cfc031040 comp rx=0 tx=0).ready entity=mgr.24365 client_cookie=0 server_cookie=0 in_seq=0 out_seq=0
2025-05-12T08:56:36.917 INFO:teuthology.orchestra.run.smithi082.stderr:2025-05-12T08:56:36.916+0000 7f0d12ffd640  1 -- 172.21.15.82:0/4233805895 <== mon.1 v2:172.21.15.121:3300/0 6 ==== mon_command_ack([{"prefix": "get_command_descriptions"}]=
0  v0) v1 ==== 72+0+223317 (secure 0 0 0) 0x7f0d0005dc30 con 0x7f0d140ff440
2025-05-12T08:56:36.923 INFO:teuthology.orchestra.run.smithi082.stderr:Traceback (most recent call last):
2025-05-12T08:56:36.923 INFO:teuthology.orchestra.run.smithi082.stderr:  File "/usr/bin/ceph", line 1327, in <module>
2025-05-12T08:56:36.923 INFO:teuthology.orchestra.run.smithi082.stderr:    retval = main()
2025-05-12T08:56:36.923 INFO:teuthology.orchestra.run.smithi082.stderr:  File "/usr/bin/ceph", line 1247, in main
2025-05-12T08:56:36.923 INFO:teuthology.orchestra.run.smithi082.stderr:    sigdict = parse_json_funcsigs(outbuf.decode('utf-8'), 'cli')
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 1004, in parse_json_funcsigs
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:    cmd['sig'] = parse_funcsig(cmd['sig'])
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 948, in parse_funcsig
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:    newsig.append(argdesc(t,
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:  File "/usr/lib/python3.9/site-packages/ceph_argparse.py", line 803, in __init__
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:    self.instance = self.t(**self.typeargs)
2025-05-12T08:56:36.924 INFO:teuthology.orchestra.run.smithi082.stderr:TypeError: __init__() got an unexpected keyword argument 'allowempty'
2025-05-12T08:56:38.351 DEBUG:teuthology.orchestra.run.smithi082:> sudo /home/ubuntu/cephtest/cephadm --image quay.ceph.io/ceph-ci/ceph:reef shell -c /etc/ceph/ceph.conf -k /etc/ceph/ceph.client.admin.keyring --fsid 69096072-2f0d-11f0-a0c9-31e
8a22acf7e -e sha1=e0d253341dc86cddb888d7b0ea9782af334ba45a -- bash -c 'ceph versions | jq -e '"'"'.mgr | length == 1'"'"''

I proposed to revert this change as @VallariAg too mentioned that issues have been seen elsewhere.

@VallariAg

PR #53989 is causing failures in fs:upgrade. Also, @VallariAg reported an issue with something similar. I don't think adequate tests were run to qualify the PR as mergeable. Reverting the change for now. Signed-off-by: Venky Shankar <vshankar@redhat.com>

VallariAg · 2025-05-14T11:30:44Z

@vshankar, thank you for looking into this and reverting the change! Looks like it was also caught by rook team: rook/rook#15844

rishabh-d-dave · 2025-05-14T12:17:04Z

I am confused as in what tests this change went through since https://tracker.ceph.com/issues/71020 points to a non-existent teuthology run. It's unlikely that ptl-tool messed up the run link while updating the redmine ticket.

The tracker was very slow few days ago, and due to it I remember running into trouble while running backport scripts. Likely same thing happened with that QA ticket as well. Will look into it.

vshankar · 2025-05-14T12:28:29Z

I am confused as in what tests this change went through since https://tracker.ceph.com/issues/71020 points to a non-existent teuthology run. It's unlikely that ptl-tool messed up the run link while updating the redmine ticket.

The tracker was very slow few days ago, and due to it I remember running into trouble while running backport scripts. Likely same thing happened with that QA ticket as well. Will look into it.

I do not understand how a slowness in redmine can cause non-existent build and run.

rishabh-d-dave · 2025-05-14T14:17:29Z

I am confused as in what tests this change went through since https://tracker.ceph.com/issues/71020 points to a non-existent teuthology run. It's unlikely that ptl-tool messed up the run link while updating the redmine ticket.

The tracker was very slow few days ago, and due to it I remember running into trouble while running backport scripts. Likely same thing happened with that QA ticket as well. Will look into it.

I do not understand how a slowness in redmine can cause non-existent build and run.

Perhaps the part of the script that was trying to update the ticket timed out since redmine tracker was too slow.

vshankar · 2025-05-14T16:43:51Z

I am confused as in what tests this change went through since https://tracker.ceph.com/issues/71020 points to a non-existent teuthology run. It's unlikely that ptl-tool messed up the run link while updating the redmine ticket.

The tracker was very slow few days ago, and due to it I remember running into trouble while running backport scripts. Likely same thing happened with that QA ticket as well. Will look into it.

I do not understand how a slowness in redmine can cause non-existent build and run.

Perhaps the part of the script that was trying to update the ticket timed out since redmine tracker was too slow.

Even if it timed out, the branch didn't build and then how were you able to schedule the suite?

rishabh-d-dave · 2025-05-14T18:06:47Z

Even if it timed out, the branch didn't build and then how were you able to schedule the suite?

Tracker was down but Sepia lab/build infra is different and it wasn't down.

@VallariAg

PR ceph#53989 is causing failures in fs:upgrade. Also, @VallariAg reported an issue with something similar. I don't think adequate tests were run to qualify the PR as mergeable. Reverting the change for now. Signed-off-by: Venky Shankar <vshankar@redhat.com>

mchangir requested a review from a team as a code owner October 13, 2023 05:39

github-actions bot added cephfs Ceph File System documentation pybind tests labels Oct 13, 2023

mchangir self-assigned this Oct 16, 2023

mchangir changed the title ~~mds: handle bad arguments during subvolume create~~ mgr/volumes: handle bad arguments during subvolume create Oct 16, 2023

neesingh-rh reviewed Oct 19, 2023

View reviewed changes

src/pybind/ceph_argparse.py Show resolved Hide resolved

vshankar requested a review from a team October 20, 2023 10:42

github-actions bot added the needs-rebase label Dec 15, 2023

dparmar18 reviewed Jan 16, 2024

View reviewed changes

doc/_ext/ceph_commands.py Show resolved Hide resolved

dparmar18 reviewed Jan 16, 2024

View reviewed changes

src/pybind/ceph_argparse.py Show resolved Hide resolved

dparmar18 reviewed Jan 16, 2024

View reviewed changes

qa/tasks/cephfs/test_volumes.py Outdated Show resolved Hide resolved

mchangir requested a review from a team January 16, 2024 11:55

mchangir force-pushed the mds-handle-bad-arguments-during-subvolume-create-2 branch from 7e70864 to 33d8e27 Compare January 16, 2024 12:00

github-actions bot removed the needs-rebase label Jan 16, 2024

github-actions bot added the stale label Mar 16, 2024

mchangir force-pushed the mds-handle-bad-arguments-during-subvolume-create-2 branch from 33d8e27 to 7506ed2 Compare March 19, 2024 07:54

rishabh-d-dave added the wip-rishabh-testing2 label Apr 22, 2025

rishabh-d-dave reviewed May 5, 2025

View reviewed changes

rishabh-d-dave added the passed-qa for PRs that have passed QA but there's an on-going conversation, so merging would be premature label May 5, 2025

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

View reviewed changes

rishabh-d-dave merged commit a88c5ec into ceph:main May 6, 2025
12 checks passed

travisn mentioned this pull request May 9, 2025

Daily CI failing to call ceph config assimilate-conf against the latest ceph main since May 6th rook/rook#15844

Closed

vshankar mentioned this pull request May 14, 2025

Revert "mgr/volumes: handle bad arguments during subvolume create" #63281

Merged

mchangir mentioned this pull request Jun 17, 2025

tentacle: mgr/volumes: handle bad arguments during subvolume create #63967

Closed

Conversation

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

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

mchangir commented Oct 13, 2023

Uh oh!

vshankar commented Oct 16, 2023

Uh oh!

mchangir commented Oct 16, 2023

Uh oh!

Uh oh!

R-0-Y commented Nov 23, 2023

Uh oh!

mchangir commented Nov 23, 2023

Uh oh!

mchangir commented Nov 23, 2023

Uh oh!

github-actions bot commented Dec 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vshankar commented Jan 16, 2024

Uh oh!

mchangir commented Jan 16, 2024

Uh oh!

mchangir commented Jan 16, 2024

Uh oh!

mchangir commented Jan 16, 2024

Uh oh!

github-actions bot commented Mar 16, 2024

Uh oh!

rishabh-d-dave commented Apr 22, 2025

Uh oh!

rishabh-d-dave commented May 5, 2025

Uh oh!

rishabh-d-dave commented May 5, 2025

Uh oh!

rishabh-d-dave left a comment • 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!

rishabh-d-dave commented May 5, 2025

Uh oh!

rishabh-d-dave commented May 5, 2025

Uh oh!

rishabh-d-dave commented May 5, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VallariAg commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented May 13, 2025

Uh oh!

mchangir commented May 14, 2025

Uh oh!

vshankar commented May 14, 2025

Uh oh!

vshankar commented May 14, 2025

Uh oh!

vshankar commented May 14, 2025

Uh oh!

VallariAg commented May 14, 2025

Uh oh!

rishabh-d-dave commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented May 14, 2025

Uh oh!

rishabh-d-dave commented May 14, 2025

Uh oh!

vshankar commented May 14, 2025

Uh oh!

mchangir commented Oct 13, 2023 •

edited

Loading

rishabh-d-dave left a comment •

edited

Loading

VallariAg commented May 12, 2025 •

edited

Loading

rishabh-d-dave commented May 14, 2025 •

edited

Loading