Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ceph: Allowing ceph filesystem argument to be optional #5931

Merged
merged 2 commits into from Aug 3, 2020

Conversation

aruniiird
Copy link
Contributor

@aruniiird aruniiird commented Jul 30, 2020

If we are unable to get a default ceph data pool name,
script won't be throwing an error.

Signed-off-by: Arun Kumar Mohan amohan@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

[skip ci]

@aruniiird
Copy link
Contributor Author

@leseb , can you please take a look?

@aruniiird aruniiird force-pushed the mds_pools_optional_in_external_script branch from cf6e4df to e047834 Compare July 30, 2020 14:42
@aruniiird aruniiird changed the title ceph: Removing any error while trying to find 'data_pool' ceph: Allowing ceph filesystem argument to be optional Jul 30, 2020
if len(json_out) > 1 and not self._arg_parser.cephfs_filesystem_name:
# if the arguments are not provided, generate an error message
# if there is an unsuccessful attempt, report an error
if ret_val != 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the command fails I wouldn't report an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed...

Copy link
Contributor Author

@aruniiird aruniiird Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But @leseb , don't you think when the underlying command itself fails, the user has to be notified.
Otherwise user will provide the arguments (through command line) and when the command fails we will silently reset the args and continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a proof of concept changed this part, please take a look. If you think this is not the correct behaviour, will revert the changes (to the non-error / silent return one)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the latter looks good, thanks.

# if there is no matching fs exists, that means provided data_pool name is invalid
if not matching_json_out:
raise ExecutionFailureException(
"Provided data_pool name, {}, doesn't exists".format(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Provided data_pool name, {}, doesn't exists".format(
"Provided data_pool name, {}, does not exist".format(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@aruniiird aruniiird force-pushed the mds_pools_optional_in_external_script branch from e047834 to 65fc7c1 Compare July 30, 2020 15:55
@aruniiird aruniiird requested a review from leseb July 30, 2020 16:13
@aruniiird aruniiird force-pushed the mds_pools_optional_in_external_script branch from 65fc7c1 to a2c3604 Compare July 30, 2020 16:30
If we are unable to get or infer a default value for ceph filesystem
script won't be throwing an error.

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
'ceph-rgw' storageclass will be provided only if 'rgw_endpoint' exists
'cephfs' storageclass will be provided only if ceph-fs and/or data-pool
parameters are valid.

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
@aruniiird aruniiird force-pushed the mds_pools_optional_in_external_script branch from a2c3604 to cba605a Compare July 31, 2020 05:46
if len(json_out) > 1 and not self._arg_parser.cephfs_filesystem_name:
# if the arguments are not provided, generate an error message
# if there is an unsuccessful attempt, report an error
if ret_val != 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the latter looks good, thanks.

@leseb
Copy link
Member

leseb commented Aug 3, 2020

@aruniiird please rebase to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants