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

mgr/dashboard_v2: Add RBD create functionality #20751

Merged
merged 1 commit into from Mar 14, 2018

Conversation

Projects
None yet
7 participants
@s0nea
Copy link
Member

s0nea commented Mar 6, 2018

This pull request adds a basic RBD create functionality to the REST API. It enables
the POST request of the http://< host >:< port >/api/rbd API endpoint.

The commit adds also related tests.

Signed-off-by: Tatjana Dehler tdehler@suse.com

self.rbd = rbd.RBD()

# Get input values
size = data.get('size', 4 * 1024 ** 3)

This comment has been minimized.

@jcsp

jcsp Mar 6, 2018

Contributor

I'm not sure it makes sense to have a default size for RBD images on the backend: shouldn't the creator always have an idea of how big they want their image?

This comment has been minimized.

@s0nea

s0nea Mar 6, 2018

Author Member

This code is inspired by the related openATTIC RBD model: https://github.com/openattic/openattic/blob/master/backend/ceph/models.py#L806
There we defined a default value. But I agree, we can remove it.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 6, 2018

Member

I'd also vote for making size mandatory here.

This comment has been minimized.

@s0nea

s0nea Mar 7, 2018

Author Member

Okay


# Set features
feature_bitmask = self._format_features(features) if len(features) > 0 \
else 61 # FIXME: hardcoded int

This comment has been minimized.

@rjfd

rjfd Mar 6, 2018

Contributor

please remove this hardcoded value. If the user does not choose any feature then pass feature_bitmask = None, and librbd will take care of setting the default image features.

This comment has been minimized.

@s0nea

s0nea Mar 7, 2018

Author Member

Yes, will do


# Get input values
size = data.get('size', 4 * 1024 ** 3)
obj_size = data.get('obj_size', 2 ** 22)

This comment has been minimized.

@rjfd

rjfd Mar 6, 2018

Contributor

please don't set a default value to obj_size and let librbd python binding take care of that. If obj_size is not specified then order = None.

This comment has been minimized.

@s0nea

s0nea Mar 7, 2018

Author Member

Okay

@ricardoasmarques
Copy link
Member

ricardoasmarques left a comment

Define a response format that can be used by the frontend.

else 61 # FIXME: hardcoded int

ioctx = mgr.rados.open_ioctx(data['pool_name'])
self.rbd.create(ioctx, data['name'], size, order=order, old_format=False,

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Mar 7, 2018

Member

IMO, instead of returning empty responses, we should return responses that contains useful information that can be used to increase the front-end usability.

What about this?
{ success: true }
and
{ success: false, message: "Image myrbd already exists in pool mypool", }

Note: This example can be implemented by catching the ImageExists exception.

If the backend does not catch this kind of exceptions, the best that frontend can do is displaying a generic error message that results from the "500 - Internal Server Error", without useful information to the user.

This comment has been minimized.

@s0nea

s0nea Mar 7, 2018

Author Member

Yes, I agree with you.

This is a kind of input validation right? - Validating if the submitted image name is usable. I talked to @sebastian-philipp about that and he told me that we will possible do the input validation on another layer and not in the Python code. That's why I left it out in this pull request. If we all agree on doing it on the API layer, I will be happy to add it.
Or is it not that kind of input validation you meant @sebastian-philipp?

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member

If rbd creation fails, it will raise an exception which generates a standard JSON error with a status code != 201, thus { success: true } or success: false is not necessary here.

But I agree that we should return something useful. What about serializing the new RBD?

This comment has been minimized.

@rjfd

rjfd Mar 7, 2018

Contributor

I think we should differentiate between general python exceptions, that may occur due to a bug, and RBD exceptions that are thrown by the RBD.create function.

For python exceptions, those are already handled by our infrastructure that outputs a JSON with the traceback and status code 500.
For RBD, we should always return status code 200, and a JSON message similar to what @ricardoasmarques is proposing, where we specify a success flag, which will be true if the image was created, and false otherwise. In case of an RBD exception there are two approaches to communicate the error to the frontend:

a) return a string message with information about the error;
b) return an integer code (available in all RBD exceptions) that describes the error

I'm in favor of the second approach (b) because that way the frontend can actually generate a string message that can be translated into several languages.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Mar 7, 2018

Member

Great, using an integer code (approach b) looks good.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member

We should make use of HTTP status codes:

Code Meaning
201 Created
400 Validation Error
500 Internal Server Error

there is no need for any additional status Boolean in the response.

Our JSON exception handler already provides a standard way to generate error responses and we should make use of it as much as possible. Instead of defining new ways to return non-successful replies. This way, we only have only one JSON structure for errors.

This comment has been minimized.

@rjfd

rjfd Mar 7, 2018

Contributor

@sebastian-philipp I don't think an RBD create error fits in one of those three status code.
If we get an error because we are trying to create an image that already exists, it's not a validation error, nor an internal server error.

This comment has been minimized.

@s0nea

s0nea Mar 7, 2018

Author Member

@rjfd I thought this would be a validation error, because the given image name is wrong (already in use), so status 400 - Bad Request? And yes, I agree, we should provide more information there, like "Can't create image 'xy' because it already exists."

This comment has been minimized.

@jecluis

jecluis Mar 8, 2018

Member

@rjfd @sebastian-philipp while http error codes can be useful to relay what is happening on the backend side, only returning those codes results in the loss of much contextual knowledge those errors are unable to properly relay to the frontend.

Given the rbd-related exceptions are documented, it makes sense to catch them, handle them and relay their meaning in a meaningful way as a result. Return them with an http error code, sure, but don't simply ignore this semantic knowledge that will make it way easier for the front-end to properly relay the problem encountered to the user.

And if we presume this is the way, it makes sense to me to ensure that all operations follow a base format when returning to the user. E.g., a Json with a "success" field, being set to true or false, makes sense. Something down the lines of
{ status: { success: true, msg: "great success!", error: 0 }, wtv: "else", ... }

And similarly for an error message. Then use the appropriate http code to convey what's happening.

@s0nea s0nea force-pushed the s0nea:wip-mgr-dashboard_rbd_add branch 2 times, most recently from 21bcf89 to 4f9be36 Mar 7, 2018

features=feature_bitmask, stripe_unit=stripe_unit,
stripe_count=stripe_count, data_pool=data_pool)
return {'success': True, 'detail': 'Image {!s} created successfully'.format(name)}
except (InvalidArgument, ImageExists) as e:

This comment has been minimized.

@rjfd

rjfd Mar 7, 2018

Contributor

Why are you catching just these two exceptions?
I would catch rbd.OSError, which is the super class of rbd specific exceptions. Then I would also return to the frontend the rbd.OSError.errno integer code, so that we can cope with translation of error messages in the frontend.

This comment has been minimized.

@s0nea

s0nea Mar 8, 2018

Author Member

Yes, I agree.

self.rbd.create(ioctx, name, size, order=order, old_format=False,
features=feature_bitmask, stripe_unit=stripe_unit,
stripe_count=stripe_count, data_pool=data_pool)
return {'success': True, 'detail': 'Image {!s} created successfully'.format(name)}

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. Why do you return a Boolean that is equal to cherrypy.response.status == 201?
  2. That message should be generated in the frontend, cause translation is easier done there.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. I've already been thru the mistake of adding such a superfluous success-boolean to an API with later regretting it again. We really shouldn't repeat this mistake.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. Trust the HTTP status code. It's already well established. There is no need to invent something new here.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. If we need to return the name here, then why not instead return the whole newly generated RBD instead?

This comment has been minimized.

@rjfd

rjfd Mar 8, 2018

Contributor

I'm fine with using HTTP status codes if we can have a different status code for each of the following cases:

  • success (e.g., image created)
  • validation error (e.g., image_name was not provided)
  • Ceph library operation exception (e.g., every rbd.OSError)
  • unexpected exception (e.g., bug in the code)

But I still have a concern about using HTTP status codes to communicate the result, or error, of a Ceph library operation, when we start executing these operations asynchronously.

I'm preparing a PR where the above RBD create operation is executed asynchronously, and then the result of the operation will be communicated within a wrapper object that describes the async operation.
At the time that the rbd.create function returns, it is no longer possible to set the HTTP status code, and the asynchronous execution infrastructure will just return the return value of the async operation, or in case of an exception is raised, it stores the exception object.

Therefore, AFAICT we will not be able to use HTTP status codes for communicating operations results. So do you think it's worth the effort of using HTTP status codes in this PR since we will be changing that in a near future?

This comment has been minimized.

@rjfd

rjfd Mar 8, 2018

Contributor

If we need to return the name here, then why not instead return the whole newly generated RBD instead?

What do you mean by "newly generated RBD"? Are you suggesting that we perform another rbd library call to fetch the image details and return it to the frontend? or just re-use the parameter values that we received from the frontend?

This comment has been minimized.

@s0nea

s0nea Mar 8, 2018

Author Member

I would suggest the following status codes:

success (e.g., image created)
-> status 201?
validation error (e.g., image_name was not provided)
-> status 400? - because it's an error initiated by the user
Ceph library operation exception (e.g., every rbd.OSError)
-> status 500? - because it's an error caused by the server
unexpected exception (e.g., bug in the code)
-> status 500? - because it's an error caused by the server as well

@rjfd:

I'm preparing a PR where the above RBD create operation is executed asynchronously, and
then the result of the operation will be communicated within a wrapper object that describes the
async operation.

Does that mean this pull request will become unnecessary anyway?

This comment has been minimized.

@rjfd

rjfd Mar 8, 2018

Contributor

Does that mean this pull request will become unnecessary anyway?

No, I'm using the code you've written for the RBD creation to test the asynchronous execution infrastructure.

This comment has been minimized.

@s0nea

s0nea Mar 8, 2018

Author Member

Okay :)

stripe_count=stripe_count, data_pool=data_pool)
return {'success': True, 'detail': 'Image {!s} created successfully'.format(name)}
except (InvalidArgument, ImageExists) as e:
return {'success': False, 'detail': str(e)}

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. You're returning here with a status code 201 Created
  2. just re-raising the exception would (at least for now) provide the same information to the frontend.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member

(at least for now)

Because, our dashboard_v2.tools.json_error_page handler is not getting the Exception. Instead, it's getting str(e) and the traceback. IMO, some interestingly bad API design.

  1. So, we should add an exception handler to RESTController.default instead, that knows what to do with Ceph exceptions.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 7, 2018

Member
  1. doing something like this:
...
except SomeException as e:
    return {'detail': str(e)}

is so generic, it shouldn't be implemented here. And in fact, it's duplicating our json_error_page.

This comment has been minimized.

@rjfd

rjfd Mar 8, 2018

Contributor

I don't think it's good to start generalize at the first time we see this code pattern. Let's wait for more code to be in to understand how we should refactor the error handling.

@s0nea s0nea force-pushed the s0nea:wip-mgr-dashboard_rbd_add branch from 4f9be36 to fcf3d9c Mar 8, 2018

@ricardoasmarques
Copy link
Member

ricardoasmarques left a comment

lgtm

@rjfd
Copy link
Contributor

rjfd left a comment

Please add a test for the case where you are trying to create an RBD image with the same name as an existing image.


# TODO: change to GET the specific RBD instead of the list as soon as it is available?
ret = self._get('/api/rbd/rbd')
rbd_names = [rbd['name'] for rbd in ret['value'] if 'name' in rbd]

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

why is the if 'name' in rbd test necessary? are you expecting to received an dictionary without the name key in the list of rbd images result?

This comment has been minimized.

@s0nea

s0nea Mar 9, 2018

Author Member

I just wanted to be sure that the key I want to access is really there. But yes, it makes sense that the test will fail if the field has been renamed for example.

self._rbd_cmd(['rm', 'rbd/test_rbd'])

@authenticate
def test_create_data_pool(self):

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

please rename the test function to test_create_rbd_in_data_pool


# TODO: possibly change to GET the specific RBD (see above)
ret = self._get('/api/rbd/rbd')
rbd_names = [rbd['name'] for rbd in ret['value'] if 'name' in rbd]

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

same comment here as above

self.assertStatus(200)
self.assertIn('test_rbd', rbd_names)

self._rbd_cmd(['rm', 'rbd/test_rbd'])

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

Why are you removing the image inside the test? the images will be removed when you delete the pools in tearDownClass method.

This comment has been minimized.

@s0nea

s0nea Mar 9, 2018

Author Member

Just to clean it up where it was created. I will remove this line.

'name': 'test_rbd_data_pool',
'size': 10240,
'data_pool': 'data_pool'}
self._post('/api/rbd', data)

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

you should also validate the result JSON

data = {'pool_name': 'rbd',
'name': 'test_rbd',
'size': 10240}
self._post('/api/rbd', data)

This comment has been minimized.

@rjfd

rjfd Mar 9, 2018

Contributor

validate the result JSON

True
"""
if not features or not isinstance(features, list):
logger.warning("Given RBD features list is empty or None.")

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 9, 2018

Member

Either you expect _format_features to get None as features, then you should not log this at all, as it is already tested.

Or, I would not expect this funciton to silently returns None, and instead raises an exception.

This comment has been minimized.

@s0nea

s0nea Mar 9, 2018

Author Member

Okay, will do.

@rjfd

This comment has been minimized.

Copy link
Contributor

rjfd commented Mar 14, 2018

QA tests look good:
http://pulpito.ceph.com/rdias-2018-03-13_16:52:27-rados:mgr-wip-rdias-testing-distro-basic-mira/

The failed job is due to the impossibility of create an EC pool with overwrittes flag in non-bluestore cluster. We should fix the RBD test to avoid running that particular case when in the presence of a non-bluestore cluster.

In order to merge this PR we need at least to skip the test that uses the EC pool for now. @s0nea could you add @unittest.skip("requires bluestore cluster") to the test_create_rbd_in_data_pool function, and move the EC pool creation/deletion code from setUpClass / tearDownClass to the test_create_rbd_in_data_pool?

@s0nea

This comment has been minimized.

Copy link
Member Author

s0nea commented Mar 14, 2018

@rjfd thank you for testing :) I'm going to adapt it.

mgr/dashboard: Add RBD create functionality
This commit adds a basic RBD create functionality to the REST API. It enables
the POST request of the http://<host>:<port>/api/rbd API endpoint.

The commit adds also related tests.

Signed-off-by: Tatjana Dehler <tdehler@suse.com>

@s0nea s0nea force-pushed the s0nea:wip-mgr-dashboard_rbd_add branch from 8e9570f to 5056cba Mar 14, 2018

@rjfd

rjfd approved these changes Mar 14, 2018

Copy link
Contributor

rjfd left a comment

lgtm

@rjfd rjfd removed the wip-rdias-testing label Mar 14, 2018

@LenzGr LenzGr merged commit 78022e1 into ceph:master Mar 14, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

epuertat added a commit to rhcs-dashboard/ceph that referenced this pull request Jul 17, 2018

Merge pull request ceph#20751 from s0nea/wip-mgr-dashboard_rbd_add
mgr/dashboard_v2: Add RBD create functionality to the Python backend

Reviewed-by: Ricardo Dias <rdias@suse.com>
Reviewed-by: Sebastian Wagner <sebastian.wagner@suse.com>
Reviewed-by: John Spray <john.spray@redhat.com>
Reviewed-by: Ricardo Marques <rimarques@suse.com>
(cherry picked from commit 78022e1)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>

@s0nea s0nea deleted the s0nea:wip-mgr-dashboard_rbd_add branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment