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: Auto-create a name for RBD image snapshots #23735

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

votdev
Copy link
Member

@votdev votdev commented Aug 24, 2018

If the user creates an snapshot of a RBD image, then a name according to ISO 8601 (<image_name>_<timestamp_ISO_8601>, e.g. test01-20180824T170532Z) will be auto-created. This can still be modified by the user.

Note, the timestamp encoded in the snapshot name is NOT the real creation date (that's because the snapshot is created after you pressed the Create Snapshot button and the timestamp is the one when the dialog appears in the UI). To workaround this issue we can drop the HHMMSS part from the snapshot name if necessary.

Signed-off-by: Volker Theile vtheile@suse.com

Copy link
Contributor

@LenzGr LenzGr left a comment

Choose a reason for hiding this comment

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

LGTM. This is useful as it makes sure that snapshot names are unique by default.

} else {
// Auto-create a name for the snapshot: <image_name>_<timestamp_ISO_8601>
// https://en.wikipedia.org/wiki/ISO_8601
snapName = `${this.rbdName}-${moment().format('YYYYMMDD[T]HHmmss[Z]')}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Z indicates you are using the Zulu timezone, you need to convert the date to UTC before doing the format.

-      snapName = `${this.rbdName}-${moment().format('YYYYMMDD[T]HHmmss[Z]')}`;
+      snapName = `${this.rbdName}-${moment().utc().format('YYYYMMDD[T]HHmmss[Z]')}`;

Copy link
Member Author

@votdev votdev Aug 29, 2018

Choose a reason for hiding this comment

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

Thx, seems i misunderstood the docu. I thought moment() is just UTC.

@LenzGr LenzGr changed the title Auto-create a name for RBD image snapshots mgr/dashboard: Auto-create a name for RBD image snapshots Aug 29, 2018
Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

Except for the missing tests everything LGTM - Good work :)

@votdev votdev force-pushed the suggest_snapshot_name branch 2 times, most recently from 67e99b2 to 4503091 Compare August 30, 2018 15:12
@votdev votdev force-pushed the suggest_snapshot_name branch 2 times, most recently from 9a7cfa8 to d9efce2 Compare August 31, 2018 09:19
@votdev votdev dismissed Devp00l’s stale review August 31, 2018 09:24

All comments have been addressed

@LenzGr
Copy link
Contributor

LenzGr commented Aug 31, 2018

retest this please

Signed-off-by: Volker Theile <vtheile@suse.com>
Copy link
Contributor

@ricardoasmarques ricardoasmarques left a comment

Choose a reason for hiding this comment

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

Lgtm

@LenzGr LenzGr merged commit df317c8 into ceph:master Sep 4, 2018
@votdev votdev deleted the suggest_snapshot_name branch September 4, 2018 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants