[SR] Snapshot and Restore UI by jen-huang · Pull Request #39193 · elastic/kibana

jen-huang · 2019-06-18T17:59:06Z

Snapshot and Restore UI

This PR adds phase 2 of Snapshot and Restore app (phase 1 PR). Previously this app was named Snapshot Repositories, now that this phase is complete, this app can finally be named properly 😄 This phase adds the following functionality:

Delete snapshots, one at a time or bulk, in Snapshots tab
Restore a snapshot using a form wizard, in Snapshots tab
Track index recovery progress, in new Recovery status tab

Outstanding items

A few items are currently outstanding for this feature and will be handled in separate PRs after this one is merged:

Client integration tests
API integration tests
Functional smoke test

Screenshots

I took too many screenshots so for your convenience they are grouped in collapsible sections below!

Delete snapshots

.

Single delete, either by selecting one row, clicking trash can icon, or Delete button from details panel

Bulk delete by selecting multiple rows

Restore snapshot - entry

.

Restore icon in row

If the snapshot is still in-progress, or otherwise invalid, restore icon is disabled

Restore button in details

Disabled restore button in details

Restore snapshot - wizard

.

Step 1: Logistics

Include global state toggle is disabled when the snapshot does not have global state available

If all indices is toggled off, selectable list appears. Same pattern for rename indices toggle. All fields have real time validation

Step 2: Index settings

This step is optional

Modified index settings accepts a JSON input and checks for certain un-modifiable setting keys

Reset index settings is a combobox field that allows custom input for additional setting keys not listed in suggestions:

Step 3: Review

Executing restore loading state, after a few seconds, user is redirected to Recovery status tab

Recovery status

.

Table with indices that have been recovered, or are being recovered, from a snapshot. Each row is expandable to drill down into per-shard recovery details

This table polls every 30 seconds by default, and the user can change the frequency of the polling

Every data update from a poll request will not interfere with the table UX - aka, there will be no flash of loading screen, current page will not reset, and expanded rows will stay expanded

…restore

elasticmachine · 2019-06-18T17:59:08Z

Pinging @elastic/es-ui

… all

…bal state

cjcenizal

I really like seeing the de/serialization logic on the client again, in order to enable the JSON view of the request! I've looked through the code and left some comments. Things look great overall. I really enjoyed reading the code. I still need to test locally.

cjcenizal · 2019-06-25T01:34:23Z

x-pack/legacy/plugins/snapshot_restore/common/lib/restore_serialization.test.ts

+import { serializeRestoreSettings } from './restore_serialization';
+
+describe('restore_serialization', () => {
+  describe('restore_serialization()', () => {


Nit: this second describe is so similar to the first that it might just end up being noise in the terminal.

cjcenizal · 2019-06-25T01:36:05Z

x-pack/legacy/plugins/snapshot_restore/common/lib/restore_serialization.test.ts

+  describe('restore_serialization()', () => {
+    it('should serialize restore settings', () => {
+      expect(serializeRestoreSettings({})).toEqual({});
+      expect(


This assertion seems like a subset of the next assertion. Is it testing something different? If so, maybe it could use its own it description to make the difference clear, and if not then could we remove it?

cjcenizal · 2019-06-25T01:38:15Z

x-pack/legacy/plugins/snapshot_restore/common/lib/restore_serialization.ts

+    try {
+      parsedIndexSettings = JSON.parse(indexSettings);
+    } catch (e) {
+      // Silently swallow parsing errors


Would throwing the error hurt? If so, can we add a comment for the rationale behind silently swallowing the error? If not, can we throw it to be a bit more defensive and transparent?

cjcenizal · 2019-06-25T01:40:08Z

x-pack/legacy/plugins/snapshot_restore/common/lib/restore_serialization.ts

+    ignore_index_settings: ignoreIndexSettings,
+  };
+
+  return Object.entries(settings).reduce((sts: RestoreSettingsEs, [key, value]) => {


I can read this just fine, but maybe a comment along the lines of // Remove undefined settings. would reduce grok time from 5 seconds to 0. Or even making this a local function called removeUndefinedSettings at the top of the file.

cjcenizal · 2019-06-25T01:44:38Z

x-pack/legacy/plugins/snapshot_restore/common/types/recovery.ts

+  shards: Array<Partial<SnapshotRecoveryShard>>;
+}
+
+export interface SnapshotRecoveryShard {


Nice types! I find these really informative (which is an unusual development in my relationship with types). 😄

cjcenizal · 2019-06-26T12:30:46Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/http/use_request.ts

-    setError(response.error);
-    setData(response.data);
-    setLoading(false);
+    // Only set data if we are doing polling


Nit: it looks like data is set in both conditions, so maybe this comment could be clarified.

EDIT: I left another comment below which may make this moot.

cjcenizal · 2019-06-26T12:32:08Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/http/use_request.ts

-    setLoading(true);
+    const isPollRequest = currentInterval && !isFirstRequest.current;
+
+    // Don't reset main error/loading states if we are doing polling


Can we also explain the "why" from a UX perspective?

// Don't reset main error/loading states if we are doing polling because we want // the reload to be transparent to the user (e.g. no spinner).

EDIT: I left another comment below which may make this moot.

cjcenizal · 2019-06-26T12:37:02Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/http/use_request.ts

+  const [polling, setPolling] = useState<boolean>(false);
+  const [currentInterval, setCurrentInterval] = useState<UseRequest['interval']>(interval);
+  const intervalRequest = useRef<any>(null);
+  const isFirstRequest = useRef<boolean>(true);


Do you think we could return isFirstRequest.current as part of the return object, and the UI could just use that to determine whether or not to show the loading spinner? That would mean we wouldn't need the polling state or isPollRequest logic in this hook.

If that's possible then I think that'd be an improvement because then the UX would be clearer when reading the consuming code and this hook would become more reusable. I'd love to extract this hook so we can reuse it in all of our apps and I can think of situations where we may want to show a subtle spinner when polling (e.g. when refreshing Console autocomplete definitions).

I will defer any changes to this hook for when we do re-use it outside this app. as it currently is, the recovery status table does show a subtle spinner (next to interval dropdown) when a poll request is in progress by using the current returned polling state

cjcenizal · 2019-06-26T12:42:38Z

x-pack/legacy/plugins/snapshot_restore/public/app/services/validation/validate_restore.ts

+  return str ? !Boolean(str.trim()) : true;
+};
+
+export const validateRestore = (restoreSettings: RestoreSettings): RestoreValidation => {


I think this file is a great candidate for some unit tests.

will add in a follow up

cjcenizal · 2019-06-26T12:50:28Z

x-pack/legacy/plugins/snapshot_restore/server/routes/api/recovery.test.ts

+        },
+        {
+          index: 'barIndex',
+          shards: [{}, {}],


This seems strange... from the deserialization and endpoint logic, it looks like these shards should contain some of the ES data, e.g. index: { size: {}, files: {} }. Do you know why we expect them to be empty objects?

the deserialization logic strips out undefined settings after destructuring (aka index: { size: {}, files: {} } is essentially flattened, then stripped out)

yaronp68 · 2019-06-26T13:04:52Z

@jen-huang Great job! this UI is awesome
suggest to consider the following changes (all related to the screenshot):

make the snapshot name linked to the snapshots tab with the specific snapshot
have the ability to get a flyout with the restore information (mainly the list of indices and if includes the global state)

jen-huang · 2019-06-26T16:38:15Z

@yaronp68 Thanks for looking! Both of your suggestions were things I thought about. The recovery status UI uses the Indices Recovery API, which shows recovery status keyed by index name. If a snapshot or repository that was used to recover an index is deleted, this API will still return both of their names, so if we link to snapshot or repository from this table, it's possible that it will be a 404. A future enhancement may be flagging the link so that a nicer 404 message is returned instead of the default one, like "This repository is no longer registered."

There is no way to get recovery information by "recovery ID", the above API returns every index that has had any recovery operation performed, so the list of indices in this UI includes ones that have snapshot recovery in progress, and ones from the past that were recovered from a snapshot. When the user gets redirected to this tab, it's not just the indices they have just selected to recover from that snapshot because we can't retrieve the recovery information for just that recovery request, so that means other recovery information like global state isn't available.

alisonelizabeth

@jen-huang This looks really awesome! Nice job!

A couple minor things I noticed while testing:

Breadcrumb is still Snapshot Repositories. I did a quick search in the code, and it looks like there is another reference in app.tsx and also in one of the tests that needs to be fixed.
The Logistics doc link points to the main snapshot restore docs. Maybe it should go here instead? https://www.elastic.co/guide/en/elasticsearch/reference/master//modules-snapshots.html#restore-snapshot
What do you think about clearing the values when you toggle a field back off in the restore wizard? For example, if I choose to modify index settings, then toggle it off and turn it back on again, the values I initially entered remain.
If you go back to a previous step, should the EuiSteps reflect that?
Should Includes global state have a value here?

alisonelizabeth · 2019-06-26T19:03:56Z

...napshot_restore/public/app/sections/home/snapshot_list/snapshot_details/tabs/tab_summary.tsx

+      {hiddenIndicesCount ? (
+        <li key="hiddenIndicesCount">
+          <EuiTitle size="xs">
+            <EuiLink onClick={() => setIsShowingFullIndicesList(true)}>


Should we allow the user to toggle this (hide indices again)?

alisonelizabeth · 2019-06-26T19:07:43Z

...gins/snapshot_restore/public/app/sections/home/recovery_list/recovery_table/shards_table.tsx

+              }
+            )}
+          >
+            <EuiText size="xs" textAlign="center" style={{ width: '100%' }}>


Can this inline style be added to a .scss file?

this was actually no longer needed 😄

alisonelizabeth · 2019-06-26T19:13:27Z

x-pack/legacy/plugins/snapshot_restore/public/app/sections/home/recovery_list/recovery_list.tsx

+                      setIsIntervalMenuOpen(false);
+                    }}
+                  >
+                    {interval >= 60 * 1000 ? (


if you follow @cjcenizal suggestion, maybe you can also update this to use the const?

alisonelizabeth · 2019-06-26T19:13:47Z

x-pack/legacy/plugins/snapshot_restore/public/app/sections/home/recovery_list/recovery_list.tsx

+                      <FormattedMessage
+                        id="xpack.snapshotRestore.recoveryList.intervalMenu.intervalValue"
+                        defaultMessage="{minutes} {minutes, plural, one {minute} other {minutes}}"
+                        values={{ minutes: Math.ceil(interval / (60 * 1000)) }}


…hot-restore

jen-huang · 2019-06-27T23:55:51Z

@alisonelizabeth

Breadcrumb fixed! I'll address the tests in a follow up PR (these test are currently skipped).
Thanks for spotting the doc link. The anchor path changed some time between 7.2 and current/master :D
The caching of previous selections when toggling field on/off in the restore wizard was intentional so that the user doesn't have to enter their previous choice (especially annoying on the Indices selection list). However the cached values do clear when moving between steps.
Fixed various issues with the EuiSteps in wizard.
Global state value fixed.

jen-huang · 2019-06-27T23:57:50Z

@cjcenizal @alisonelizabeth Thanks for the reviews! I've addressed or replied to all of them. There are a few UI enhancement ideas collected from design and stakeholder reviews that I'm currently working on, but as to not block this PR I'd like to submit them in a follow up PR so that this one can get merged soon. Please let me know if you have anything else you'd like me to adjust before approval 😄

elasticmachine · 2019-06-28T00:50:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…on format

elasticmachine · 2019-06-28T06:04:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

@jen-huang changes LGTM, thanks!

I did happen to notice this error in the console when I was registering a repository, not sure if it existed before not.

…restore

elasticmachine · 2019-06-28T18:31:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Change app name and change default tab to snapshots * Fix i18n issues * UI placeholder for deleting snapshots * Add bulk snapshot delete endpoint and test, adjust UI delete components * Add restore action buttons * Set up restore snapshot form entry * Add RestoreSettings type * Restore step general * Combobox for ignore settings * Code editor for modifying index settings * Truncate list of indices in snapshot details and provide link to show all * Disable include global state option for snapshots that don't have global state * Add step titles, rename General to Logistics * Committing deleted file * Change repository detail settings to reverse list style * Review summary tab and placeholder json tab * Add restore wizard validation * Add restore serialization * Create restore endpoint and integration * Move new files to /legacy * Fix bugs, add search filter bar to indices list * Allow de/select all of indices * Create new recovery status tab * Prefix hook methods with `use` * Remove unnecessary RouteComponentProps * Add get all snapshot recoveries endpoint, deserialization, types, and tests * Remove unused timeout variable; enhance to allow polling (interval'd requests) without resetting state like loading * Add recovery table * Use shim'd i18n * Adjust disabled restore state * Fix invariant error * Fix partial restore label * Fix misc bugs * Address copywriting feedback * Address i18n feedback * Address PR feedback * Rename recovery to restore * Add toggle show/hide link to restore wizard summary * Fix snapshot tests due to changes in includeGlobalState deserialization format * Fix EuiCard warning

jen-huang added 13 commits June 5, 2019 14:48

Change app name and change default tab to snapshots

6ed1c54

Fix i18n issues

de3c2c2

UI placeholder for deleting snapshots

6dda580

Merge remote-tracking branch 'upstream/master' into feature/snapshot-…

a1d8c8d

…restore

Add bulk snapshot delete endpoint and test, adjust UI delete components

21f8e5d

Merge remote-tracking branch 'upstream/master' into feature/snapshot-…

098cf76

…restore

Add restore action buttons

259cfbe

Set up restore snapshot form entry

bb48ae0

Add RestoreSettings type

46be0da

Restore step general

67d5476

Merge remote-tracking branch 'upstream/master' into feature/snapshot-…

e0e82a8

…restore

Combobox for ignore settings

10b380b

Code editor for modifying index settings

3a452ef

jen-huang added release_note:enhancement WIP Work in progress v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.3.0 labels Jun 18, 2019

jen-huang changed the title ~~[SR] Snapshot restore UI (WIP)~~ [SR] [skip ci] Snapshot restore UI (WIP) Jun 18, 2019

jen-huang added 2 commits June 18, 2019 12:23

Truncate list of indices in snapshot details and provide link to show…

d44ec39

… all

Disable include global state option for snapshots that don't have glo…

53fdeb8

…bal state

elastic deleted a comment from elasticmachine Jun 18, 2019

jen-huang added 4 commits June 18, 2019 13:31

Add step titles, rename General to Logistics

ef57466

Committing deleted file

be1172d

Change repository detail settings to reverse list style

7ac5a52

Review summary tab and placeholder json tab

df0dd7c

cjcenizal mentioned this pull request Jun 19, 2019

Add smoke test for Snapshot Repositories #39286

Closed

Add restore wizard validation

395772e

cjcenizal reviewed Jun 26, 2019

View reviewed changes

Fix misc bugs

a701055

alisonelizabeth reviewed Jun 26, 2019

View reviewed changes

jen-huang added 6 commits June 26, 2019 20:39

Address copywriting feedback

950f0ff

Address i18n feedback

ed52296

Address PR feedback

59785b6

Rename recovery to restore

a6bebbd

Add toggle show/hide link to restore wizard summary

eb3f03d

Merge branch 'master' of github.com:elastic/kibana into feature/snaps…

a223042

…hot-restore

Fix snapshot tests due to changes in includeGlobalState deserializati…

b437a19

…on format

alisonelizabeth approved these changes Jun 28, 2019

View reviewed changes

jen-huang added 2 commits June 28, 2019 10:19

Fix EuiCard warning

5bf2e94

Merge remote-tracking branch 'upstream/master' into feature/snapshot-…

7b7a1f1

…restore

jen-huang merged commit 8a35056 into elastic:master Jun 28, 2019

jen-huang deleted the feature/snapshot-restore branch June 28, 2019 18:58

jen-huang mentioned this pull request Jun 28, 2019

[7.x] [SR] Snapshot and Restore UI (#39193) #39938

Merged

jen-huang mentioned this pull request Jun 28, 2019

[SR] Adjust app permissions #39966

Merged

cjcenizal added the release highlight label Jun 30, 2019

This was referenced Jul 1, 2019

Added snapshot restore smoke test to ensure the app has loaded. #39812

Merged

[SR] Add link to index settings docs #40083

Merged

cjcenizal mentioned this pull request Jul 10, 2019

Extract useRequest and sendRequest from SR into elasticsearch-ui-shared plugin #40777

Merged

jen-huang mentioned this pull request Jul 18, 2019

[SR] Allow custom index pattern to be used instead of selectable list when choosing indices to restore #41534

Merged

Conversation

jen-huang commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Snapshot and Restore UI

Outstanding items

Screenshots

Uh oh!

elasticmachine commented Jun 18, 2019

Uh oh!

cjcenizal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jen-huang Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaronp68 commented Jun 26, 2019

Uh oh!

jen-huang commented Jun 26, 2019

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jen-huang commented Jun 27, 2019

Uh oh!

jen-huang commented Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 28, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 28, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 28, 2019

💚 Build Succeeded

jen-huang commented Jun 18, 2019 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cjcenizal Jun 25, 2019 •

edited

Loading

jen-huang Jun 27, 2019 •

edited

Loading

jen-huang commented Jun 27, 2019 •

edited

Loading