Update:
Replaced word group with subvolgroup in PR description and commit message.

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver?

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver

AFAICT, its the volumes driver that enforces all the rules on the subvolume and group naming, which, I think, is the right place do such validations

jenkins test make check

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver

AFAICT, its the volumes driver that enforces all the rules on the subvolume and group naming, which, I think, is the right place do such validations

Right. However, in this case its a empty string that getting passed in. I expected this to be caught before calling into the volumes driver.

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver

AFAICT, its the volumes driver that enforces all the rules on the subvolume and group naming, which, I think, is the right place do such validations

Right. However, in this case its a empty string that getting passed in. I expected this to be caught before calling into the volumes driver.

There's a way to mention what are the acceptable characters for a parameter via the goodchars spec.

Is there a way to mention in the command parameter spec that the argument also needs to be non-empty string ?

Where can I find the parameter spec attributes definitions ?
Maybe I could add a spec to specify non-empty string.

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver

AFAICT, its the volumes driver that enforces all the rules on the subvolume and group naming, which, I think, is the right place do such validations

Right. However, in this case its a empty string that getting passed in. I expected this to be caught before calling into the volumes driver.

There's a way to mention what are the acceptable characters for a parameter via the goodchars spec.

Is there a way to mention in the command parameter spec that the argument also needs to be non-empty string ?

Where can I find the parameter spec attributes definitions ? Maybe I could add a spec to specify non-empty string.

Something along those lines. I think other mgr modules just might have the similar issues with empty strings.

@mchangir Do we know why the string (subvolume, group) isn't caught before even getting called into the volumes driver

AFAICT, its the volumes driver that enforces all the rules on the subvolume and group naming, which, I think, is the right place do such validations

Right. However, in this case its a empty string that getting passed in. I expected this to be caught before calling into the volumes driver.

There's a way to mention what are the acceptable characters for a parameter via the goodchars spec.
Is there a way to mention in the command parameter spec that the argument also needs to be non-empty string ?
Where can I find the parameter spec attributes definitions ? Maybe I could add a spec to specify non-empty string.

Something along those lines. I think other mgr modules just might have the similar issues with empty strings.

I looked up other mgr/volumes create commands. The only ones found were:

• fs subvolumegroup create
• fs subvolumegroup snapshot create
• fs subvolume create
• fs subvolume snapshot create

The subvolume* snapshot create commands fail with various errors without explicit argument validation and hence haven't had this issue before.

e.g. fs snap-schedule add command never validates input arguments for fs and path, the effect of validation only takes place when the actual snapshot creation is attempted and then the snap-schedule is deactivated or when the INSERT into the DB happens via the DB field/column constraints

changes in src/pybind/ceph_argparse.py seems to have broken something:

FAIL: test_parse_json_funcsigs (__main__.ParseJsonFuncsigs)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/pybind/test_ceph_argparse.py", line 46, in test_parse_json_funcsigs
    self.assertRaises(TypeError, parse_json_funcsigs, commands, 'cli')
AssertionError: TypeError not raised by parse_json_funcsigs

----------------------------------------------------------------------
Ran 136 tests in 58.276s

FAILED (failures=1)
sys:1: ResourceWarning: unclosed file <_io.TextIOWrapper name=4 encoding='UTF-8'>

https://jenkins.ceph.com/job/ceph-pull-requests/110979/consoleText

@vshankar this is ready for review

@vshankar this is ready for review

Please add a test for the fix.

@ljflores This touches ceph_argparse python source, would you be the one to review that?

EDIT: Changes LGTM, but wanted someone from the relevant team to have a look.

@ljflores ping? Could you please have a look.

EDIT: relevant tests in fs suite run fine.

Test passes: https://pulpito.ceph.com/vshankar-2023-06-21_05:44:47-fs-wip-vshankar-testing-20230620.131636-testing-default-smithi/7309526/

I'll take that back. @mchangir Please have a look at - https://pulpito.ceph.com/vshankar-2023-06-21_05:44:47-fs-wip-vshankar-testing-20230620.131636-testing-default-smithi/7309397/

Seems related to this change.

Fixed a bug introduced by commit bf83eaa. The commit had introduced a spurious empty string as a command-line argument to fs subvolume create command if no subvol_options section was found in the job config.

A sanity job run mostly passes with the changes.

Fixed a bug introduced by commit bf83eaa. The commit had introduced a spurious empty string as a command-line argument to fs subvolume create command if no subvol_options section was found in the job config.

A sanity job run mostly passes with the changes.

Thx @mchangir. I think there is merit running this change through other subsystem suites.

ping @ljflores

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

rebased

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.

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@mchangir

ropening...