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

mgr/dashboard: RBD copy, RBD flatten and snapshot clone (frontend) #21526

Merged
merged 7 commits into from Apr 26, 2018

Conversation

ricardoasmarques
Copy link
Contributor

@ricardoasmarques ricardoasmarques commented Apr 19, 2018

This PR will add the frontend implementation for RBD copy, RBD flatten and snapshot clone functionalities.

screenshot from 2018-04-19 15-08-29

screenshot from 2018-04-19 15-11-14

Signed-off-by: Ricardo Marques rimarques@suse.com

@ricardoasmarques
Copy link
Contributor Author

FYI, I've now pushed a new commit with the "RBD copy" implementation.

screenshot from 2018-04-20 08-08-32

@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: RBD snapshot clone (frontend) mgr/dashboard: RBD snapshot clone and RBD copy (frontend) Apr 20, 2018
@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: RBD snapshot clone and RBD copy (frontend) mgr/dashboard: RBD copy and snapshot clone (frontend) Apr 20, 2018
@@ -33,6 +33,12 @@
<span *ngIf="selection.first()?.is_protected"><i class="fa fa-fw fa-unlock"></i><span i18n>Unprotect</span></span>
</a>
</li>
<li role="menuitem"
[ngClass]="{'disabled': !selection.hasSingleSelection || selection.first().executing || !selection.first().is_protected}">

Choose a reason for hiding this comment

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

Starting w/ Mimic, you do not need to use protected snapshots to clone an image if clone v2 is enabled. It would be based on the rbd_default_clone_format configuration setting:

1: require a protected snapshot
2: do not require a protected snapshot
auto: check the OSD maps' min_compat_client / require_min_compat_client values and if the minimum client is at least Mimic, do not require a protected snapshot.

<a class="dropdown-item" routerLink="/rbd/clone/{{ poolName }}/{{ rbdName }}/{{ selection.first()?.name }}">
<i class="fa fa-fw fa-clone"></i><span i18n>Clone</span>
</a>
</li>
<li role="menuitem"

Choose a reason for hiding this comment

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

Nit: add support for copying an image snapshot

request.child_pool_name = this.rbdForm.get('pool').value;
request.child_image_name = this.rbdForm.get('name').value;
request.obj_size = this.formatter.toBytes(this.rbdForm.get('obj_size').value);
if (!this.defaultFeaturesFormControl.value) {

Choose a reason for hiding this comment

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

Nit: when I cloned an image w/ default features, the clone model didn't have the default features checkbox selected but did have each individual feature selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now created a new REST API endpoint to get the default features from the "rbd_default_features" config, which allowed me to remove the "Default features" checkbox from the UI.

Default features will now be pre-selected on the creation form, which I believe is an usability improvement.

request.dest_pool_name = this.rbdForm.get('pool').value;
request.dest_image_name = this.rbdForm.get('name').value;
request.obj_size = this.formatter.toBytes(this.rbdForm.get('obj_size').value);
if (!this.defaultFeaturesFormControl.value) {

Choose a reason for hiding this comment

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

Nit: same comment here

<li role="menuitem"
[ngClass]="{'disabled': !selection.hasSingleSelection || selection.first().executing}">
<a class="dropdown-item" routerLink="/rbd/copy/{{ selection.first()?.pool_name }}/{{ selection.first()?.name }}"><i class="fa fa-fw fa-copy"></i><span i18n>Copy</span></a>
</li>
<li role="menuitem"
[ngClass]="{'disabled': !selection.hasSingleSelection || selection.first().executing}">
<a class="dropdown-item" (click)="deleteRbdModal()"><i class="fa fa-fw fa-trash-o"></i><span i18n>Delete</span></a>

Choose a reason for hiding this comment

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

Nit: not related to this PR, but when I attempted to delete a parent image, I correctly received an error alert but the delete modal did not automatically close (it just stuck spinning, forcing me to click cancel).

@ricardoasmarques
Copy link
Contributor Author

I've pushed a new commit with RBD flatten functionality.

@ricardoasmarques ricardoasmarques changed the title mgr/dashboard: RBD copy and snapshot clone (frontend) mgr/dashboard: RBD copy, RBD flatten and snapshot clone (frontend) Apr 20, 2018
@votdev
Copy link
Member

votdev commented Apr 23, 2018

You need to adapt your controller to the latest changes.
"Module 'dashboard' has failed dependency: type object 'RESTController' has no attribute 'args_from_json'"

@ricardoasmarques
Copy link
Contributor Author

@votdev I've now rebase this branch and removed the usage of args_from_json.

Note that ATM master branch is broken, so you have to apply #21606 to compile ceph with success.


def test_flatten(self):
self.create_snapshot('rbd', 'img1', 'snapf')
self._rbd_cmd(['snap', 'protect', 'rbd/img1@snapf'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to use our own REST API endpoints to do this, to get better coverage of our own code.
For instance, use: self.update_snapshot('rbd', 'img1', 'snapf', None, True)

def test_flatten(self):
self.create_snapshot('rbd', 'img1', 'snapf')
self._rbd_cmd(['snap', 'protect', 'rbd/img1@snapf'])
self._rbd_cmd(['clone', 'rbd/img1@snapf', 'rbd_iscsi/img1_snapf_clone'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use: self.clone_image('rbd', 'img1', 'snapf', 'rbd_iscsi', 'img1_snapf_clone')

self.assertStatus(200)
self.assertIsNone(img['parent'])

self._rbd_cmd(['snap', 'unprotect', 'rbd/img1@snapf'])
Copy link
Contributor

Choose a reason for hiding this comment

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

self.update_snapshot('rbd', 'img1', 'snapf', None, False)


def test_default_features(self):
default_features = self._task_post('/api/block/image/default_features')
self.assertEquals(default_features, ['deep-flatten', 'exclusive-lock',
Copy link
Contributor

Choose a reason for hiding this comment

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

assertEquals is deprecated, use assertEqual instead


return _rbd_image_call(pool_name, image_name, _flatten)

@RESTController.collection()
Copy link
Contributor

Choose a reason for hiding this comment

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

please specify the HTTP method allowed (I guess it should be GET): @RESTController.collection(['GET'])

self.assertStatus(204)

def test_default_features(self):
default_features = self._task_post('/api/block/image/default_features')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using a POST request to retrieve info? this should be a GET http request.

@rjfd
Copy link
Contributor

rjfd commented Apr 24, 2018

ricardoasmarques and others added 5 commits April 24, 2018 16:50
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Dias <rdias@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
@ricardoasmarques
Copy link
Contributor Author

@dillaman @rjfd I've addressed all your comment, thanks for reviewing.

@votdev
Copy link
Member

votdev commented Apr 25, 2018

Would like to see the feature that the snapshot name is auto-generated using the current date, e.g. <IMAGENAME>_2018-04-25_13:29:11.

@votdev
Copy link
Member

votdev commented Apr 25, 2018

fireshot capture 24 - ceph - http___localhost_4200_ _block_rbd

The Created column does not display the date in my local date/time format.

@votdev
Copy link
Member

votdev commented Apr 25, 2018

Would be great if the Name field in the 'Copy|Clone RBD' form will be pre-filled, e.g. using the origin image name and the prefix _(copy|clone) and or the current date to make it unique.

@votdev
Copy link
Member

votdev commented Apr 25, 2018

If i press the Back button in the clone form i would expect that the previous view showing me the snapshot table is shown again. Currently the image table is displayed without any selected image.

@rjfd
Copy link
Contributor

rjfd commented Apr 25, 2018

@votdev could you paste the stack trace of that 500 error?

@rjfd
Copy link
Contributor

rjfd commented Apr 25, 2018

Run QA tests again with the latest change, and every test passed: http://pulpito.ceph.com/rdias-2018-04-25_11:46:51-rados:mgr-wip-rdias-testing-distro-basic-smithi/

@ricardoasmarques
Copy link
Contributor Author

Would like to see the feature that the snapshot name is auto-generated using the current date, e.g. _2018-04-25_13:29:11.

Would be great if the Name field in the 'Copy|Clone RBD' form will be pre-filled, e.g. using the origin image name and the prefix _(copy|clone) and or the current date to make it unique.

I would like to avoid any kind of name auto-generation until we have feedback from someone that uses this features in a production environment. Otherwise we run the risk of suggesting names that do not make sense in real-world usage.

@ricardoasmarques
Copy link
Contributor Author

ricardoasmarques commented Apr 25, 2018

If i press the Back button in the clone form i would expect that the previous view showing me the snapshot table is shown again. Currently the image table is displayed without any selected image.

I agree that this behavior would be an improvement, but our datatable doesn't support that.

Maybe we should enhance our datatable to reflect the selection and pagination info in the URL, but this is clearly a separate PR IMO.

@ricardoasmarques
Copy link
Contributor Author

The Created column does not display the date in my local date/time format.

Thanks for reporting this, but this table is not part of this PR.

I would say that this is an issue on cdDatePipe that is widely used across the dashboard frontend.

What about reporting an issue on http://tracker.ceph.com/ ?

@votdev
Copy link
Member

votdev commented Apr 25, 2018

@ricardoasmarques I've created a bug report for the cdDatePipe.

@LenzGr LenzGr merged commit 893857f into ceph:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants