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: Add support for RBD Trash #23351

Merged
merged 7 commits into from Sep 27, 2018

Conversation

Projects
None yet
8 participants
@tspmelo
Contributor

tspmelo commented Jul 31, 2018

This PR will bring all RBD Trash operations to the ceph dashboard.

Fixes: http://tracker.ceph.com/issues/24272

There is a known issue with the date picker component of ngx-bootstrap.
There is already a PR to fix this, so I will wait for the next release and then update this PR.

  • Update ngx-bootstrap
  • Add expiration date validation
  • Add i18n in all strings
@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Jul 31, 2018

Screenshots:
image
image
image
image
image
image
image
image

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch 3 times, most recently from 7cd1853 to 3de3b04 Jul 31, 2018

@ricardoasmarques

This comment has been minimized.

Member

ricardoasmarques commented Aug 1, 2018

This is the first page with top-level tabs, but in the future we will have more pages like this (grafana integration, etc...), so I suggest to reflect the selected tab in the URL and breadcrumb.

Some benefits: bookmark selected tab, browser back navigation, use router to navigate directly to a tab, etc...

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Aug 1, 2018

shall we use "Move an image to trash" instead of "Move RBD to trash"? it sounds like this button will help a user to disable RBD as a whole.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Aug 20, 2018

shall we use "Move an image to trash" instead of "Move RBD to trash"?

I agree - let's try to reduce our usage of terms like this.

@LenzGr LenzGr requested a review from dillaman Aug 20, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Aug 21, 2018

Should we use "RBD Image" or is "Image" enough?

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch from 04c36bb to 0ba997a Aug 21, 2018

result.append(trash)
return result
def _trash_list(self, pool_name=None):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

RbdTrash._trash_list is 100% identical to RBD._rbd_list. Can you refactor this?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

Although they were identical, the _rbd_pool_list call was referencing different methods.
I have renamed _rbd_pool_list to _trash_pool_list, so we can better differentiate them.

Refactoring this would require I added parameters to the method and conditional calls inside, do you think it is worth it?

result = []
for trash in images:
trash['pool_name'] = pool_name
trash['deletion_time'] = "{}Z".format(trash['deletion_time'].isoformat())

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

do we have a general problem with proper ISO 8601 in the dashboard?

find * -name '*.py' | grep -v frontend | xargs grep '{}Z' | wc -l
5

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

ceph cmds/api doesn't return dates in iso format, which can cause problems in the UI.

@@ -374,6 +375,17 @@ def default_features(self):
rbd_default_features = mgr.get('config')['rbd_default_features']
return _format_bitmask(int(rbd_default_features))
# Move an image to the trash. Images, even ones actively in-use by clones,

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

Method documentation should IMO be put into the docstring instead.

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

Done.

import six
import cherrypy

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

reason for moving this?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

I'm guessing it was during a rebase.

for pool in images:
for image in pool['value']:
if image['deferment_end_time'] < now:

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

is this UTC or in local timezone? See utcnow(). Also, why do we have to honor deferment ourselves?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

They are both in UTC.
rbd api doesn't support the purge command, so I implemented it.
When purging you can only delete images that have expired, and that is determined by the deferment_end_time

@RESTController.Collection('POST', query_params=['pool_name'])
def purge(self, pool_name=None):
now = "{}Z".format(datetime.now().isoformat())
images = self._trash_list(pool_name)

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

eh. If this actually contains a list of pool containing the images, I'd rename this to pools?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

done.

if image['deferment_end_time'] < now:
def _remove_trash(ioctx):
rbd_inst = rbd.RBD()
# pylint: disable=cell-var-from-loop

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

Don't ignore cell-var-from-loop here. Instead, I'd fix the code. The current implementation may be broken, if you try to delete more than one image at a time.

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

done

@handle_rados_error('pool')
@RbdTask('trash/purge', ['{pool_name}'], 2.0)
@RESTController.Collection('POST', query_params=['pool_name'])
def purge(self, pool_name=None):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

As the rbd.RBD() method is called trash_remove(), what do you think of staying consistent to the Python API can call this endpoint remove instead?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

purge is the correct name for this method, but I have renamed delete into remove.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 23, 2018

Member

Why do you think purge is better?

This comment has been minimized.

@tspmelo

tspmelo Aug 24, 2018

Contributor

It's the official command to execute this operation.
trash purge [pool-name]

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 24, 2018

Member

urgh. thus there is an inconsistency between the command line and the python API?

This comment has been minimized.

@tspmelo

tspmelo Aug 24, 2018

Contributor

Remove is also a valid rbd trash command, so the API is correct.
What happens is that the python API is lacking the purge method, so I implemented it in the dashboard.

return _rbd_call(pool_name, _restore_trash)
# Delete an image from trash. If image deferment time has not expired you

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Aug 21, 2018

Member

move this into the docstring?

This comment has been minimized.

@tspmelo

tspmelo Aug 22, 2018

Contributor

done

@dillaman

This comment has been minimized.

Contributor

dillaman commented Aug 21, 2018

Couple comments:
(1) I attempted to "Purge Trash" and left the default pool selection at "All" and then received a "Purge RBD Trash [errno 2] error opening pool 'false'" popup in the corner.
(2) When purging, I selected my 'RBD' pool and received a success message but my trashed image wasn't purged.
(3) In the 'move image to trash' popup, it would be nice if the 'expires at' field could be "Protection expires at" to be clearer. It would also be nice if the date/time could be defaulted to empty with perhaps a greyed out hint text of something along the lines of "not protected".
(4) It seems odd that deleting an item from the trash requires you to type in the full image name to verify, but yet the purge action doesn't have a similar protection.
(5) In the "Expires at" column, could it just say "expired" if the expiration date is in the past?

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Aug 21, 2018

Should we use "RBD Image" or is "Image" enough?

I think "Image" is sufficient.

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch from 0ba997a to cc9af11 Aug 22, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Aug 22, 2018

@dillaman

  1. Done.
  2. The message informs if the command was successfully executed. It doesn't know if there was any image deleted. Was your image expired? could you try again with the new code?
  3. Done.
  4. I have removed the need to type the image name when deleting an expired image. CLI doesn't require you to do it, so the dashboard should do the same.
  5. I have change the column from "expires at" to "status" and I'm using the same output the CLI returns. Now it will show "Expired at 8/22/18 4:16:29 PM" or "Protected until 8/31/18 4:16:11 PM"
@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Aug 22, 2018

I have updated the screenshots: #23351 (comment)

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch from cc9af11 to 34d25cf Aug 22, 2018

@tspmelo tspmelo removed the needs-rebase label Aug 22, 2018

novalidate>
<div class="modal-body">
<p>
<ng-container i18n>To move</ng-container>

This comment has been minimized.

@votdev

votdev Sep 20, 2018

Contributor

Please apply the following patch to fix the text issue.

auswahl_002

--- src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-move-modal/rbd-trash-move-modal.component.html	(date 1533458219000)
+++ src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-move-modal/rbd-trash-move-modal.component.html	(date 1537454024000)
@@ -10,9 +10,9 @@
           novalidate>
       <div class="modal-body">
         <p>
-          <ng-container i18n>To move</ng-container>
+          <ng-container i18n>To move</ng-container>&nbsp;
           <kbd>{{ poolName }}/{{ imageName }}</kbd>&nbsp;
-          <ng-container i18n>to trash, click</ng-container>
+          <ng-container i18n>to trash, click</ng-container>&nbsp;
           <kbd i18n>Move Image</kbd>.&nbsp;
           <ng-container i18n>Optionally, you can pick a different expiration date.</ng-container>
         </p>
novalidate>
<div class="modal-body">
<p>
<ng-container i18n>To restore</ng-container>

This comment has been minimized.

@votdev

votdev Sep 20, 2018

Contributor

Please apply the following patch to fix the text display issue.

auswahl_003

--- src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-restore-modal/rbd-trash-restore-modal.component.html	(date 1533458219000)
+++ src/pybind/mgr/dashboard/frontend/src/app/ceph/block/rbd-trash-restore-modal/rbd-trash-restore-modal.component.html	(date 1537454340000)
@@ -10,9 +10,9 @@
           novalidate>
       <div class="modal-body">
         <p>
-          <ng-container i18n>To restore</ng-container>
+          <ng-container i18n>To restore</ng-container>&nbsp;
           <kbd>{{ poolName }}/{{ imageName }}@{{ imageId }}</kbd>,&nbsp;
-          <ng-container i18n>type the image's new name and click</ng-container>
+          <ng-container i18n>type the image's new name and click</ng-container>&nbsp;
           <kbd i18n>Restore Image</kbd>.
         </p>
@votdev

This comment has been minimized.

Contributor

votdev commented Sep 20, 2018

The date chooser dialog is displayed erroneous (Chromium Version 69.0.3497.81 and Firefox 62.0).

peek 2018-09-20 16-43

@votdev

This comment has been minimized.

Contributor

votdev commented Sep 20, 2018

Is it somehow possible to adapt the generic deletion dialog behaviour? I know the dialog has some more features if the selected image has an expiration date, so you can not reuse the deletion modal dialog.

auswahl_005

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Sep 20, 2018

@votdev Did you restarted the ng serve? This PR modifies angular.json and that is not automatically read.

I will look into the deletion modal.

@votdev

This comment has been minimized.

Contributor

votdev commented Sep 21, 2018

@votdev Did you restarted the ng serve? This PR modifies angular.json and that is not automatically read.

I will look into the deletion modal.

After running npm install and npm start it works.

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch from 9b3b7ff to f7f7baa Sep 21, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Sep 21, 2018

Just pushed some changes and the PR is now ready to review and merge.

  • I added a temporary fix for the datepicker so we don't have to wait for ngx-bootstrap to release a new version.
  • I'm now using the DeletionModalComponent and have removed the TrashDeleteModalComponent
  • Using the new TableActionsComponent

I have also updated the screenshots.

@tspmelo tspmelo removed the DNM label Sep 21, 2018

@LenzGr

LenzGr approved these changes Sep 24, 2018

I tested this locally - LGTM! Thank you.

@votdev

votdev approved these changes Sep 24, 2018

@callithea

This comment has been minimized.

Member

callithea commented Sep 25, 2018

jenkins test dashboard

tspmelo added some commits Jun 19, 2018

mgr/dashboard: Add RBD Trash endpoints
Fixes: http://tracker.ceph.com/issues/24272

Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Add UI for RBD Trash List
Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Add UI for RBD Trash Delete
Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Add UI for RBD Trash Purge
Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Replace current delete icons with "fa-times"
This will allow us to better differentiate a actual delete from a move to trash.

Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Add UI for RBD Trash Move
Fixes: http://tracker.ceph.com/issues/24272

Signed-off-by: Tiago Melo <tmelo@suse.com>
mgr/dashboard: Add UI for RBD Trash Restore
Signed-off-by: Tiago Melo <tmelo@suse.com>

@tspmelo tspmelo force-pushed the tspmelo:wip-rbd-trash branch from f7f7baa to 1eeaa50 Sep 25, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Sep 25, 2018

jenkins retest this please

1 similar comment
@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Sep 25, 2018

jenkins retest this please

@LenzGr LenzGr merged commit 75c1d8e into ceph:master Sep 27, 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

@tspmelo tspmelo deleted the tspmelo:wip-rbd-trash branch Sep 27, 2018

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