SR: SLM retention UI #45193

alisonelizabeth · 2019-09-09T20:23:17Z

This PR adds the ability to configure snapshot retention as part of the create policy wizard.

I considered integrating the form lib into this, but ultimately decided it would be better addressed in a separate PR (if we decide to do it), as it would involve refactoring the existing code.

I am also still working with Gail on the copy, so there will likely be some text changes to come.

Release notes

This feature adds the ability to configure snapshot retention when creating a Snapshot Lifecycle Management policy within the Snapshot and Restore app. If a policy has retention configured, users have the ability to update the slm.retention_schedule cluster setting through the UI. Policy statistics were also added to the policy details panel.

Testing locally

You will need to create a repository first, before creating a policy. For a shared filesystem repo, you need to specify a path.repo in ES. For example:

yarn es snapshot -E path.repo=/tmp/es-backups
For more information, see the SR docs.

Screenshots

Policy tab

If a policy with retention exists, but the slm.retention_schedule cluster setting has not be set, a callout will display.

A user has the ability to edit the slm.retention_schedule cluster setting if they have policies with retention.

The slm.retention_schedule cluster setting panel will not render, if there are no policies configured with retention.

Retention schedule modal (supports simple and advanced cron editor)

Snapshot retention step

Validation

Review step

Added retention to summary and request tabs

Details panel

Added policy stats and retention details (if configured)

elasticmachine · 2019-09-09T20:23:19Z

Pinging @elastic/es-ui

elasticmachine · 2019-09-09T21:39:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-13T18:00:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-13T20:20:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-16T21:01:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…etention

elasticmachine · 2019-09-17T22:14:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-20T18:08:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…etention

elasticmachine · 2019-09-23T17:48:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-23T22:02:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-09-30T18:31:39Z

@elasticmachine merge upstream

elasticmachine · 2019-09-30T19:43:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cuff-links · 2019-10-01T02:08:12Z

Looking at the tests now.

cuff-links

I ran the tests locally. They work. I had some comments/nits but they aren't enough to block the PR on. Good Job.

Bamieh

I've left a few comments for i18n related things. Thank you

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

...public/app/sections/home/policy_list/policy_retention_schedule/policy_retention_schedule.tsx

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx

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

alisonelizabeth · 2019-10-01T18:21:33Z

@Bamieh thanks for the review! I believe I addressed all of the issues you found with ids, and responded to your other comments.

elasticmachine · 2019-10-01T20:16:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Bamieh

i18n LGTM!

jloleysens

This looks really great @alisonelizabeth ! I read through the code and will do a manual test before submitting my final review, but wanted to submit these comments so long. There is nothing I could identify as a major blocker so these are mostly suggestions and questions.

jloleysens · 2019-10-02T10:21:48Z

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

+
+export const deserializeTime = (time: string) => {
+  const timeUnits = Object.values(TIME_UNITS);
+  const timeChars = time.split('');


Don't think this .split is necessary. String.prototype has a an indexOf method:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/indexOf

jloleysens · 2019-10-02T10:33:53Z

...k/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_retention.tsx

+
+      {/** Expiration field */}
+      {renderExpireAfterField()}
+      {/** Retention count fields */}


Nit: I'd say the code is self-documented in this case 😃

jloleysens · 2019-10-02T10:35:31Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_review.tsx

        </EuiFlexItem>
      </EuiFlexGroup>
+
+      {/* Retention summary */}


Thanks for this comment! 👍

jloleysens · 2019-10-02T10:53:49Z

...k/legacy/plugins/snapshot_restore/public/app/components/policy_update_retention_provider.tsx

+
+type OnSuccessCallback = () => void;
+
+export const PolicyUpdateRetentionProvider: React.FunctionComponent<Props> = ({ children }) => {


Suggestion: Maybe this could be PolicyRetentionModalProvider? Just think having ModalProvider in the name would good. WDYT?

good point, changed to UpdateRetentionModalProvider

...public/app/sections/home/policy_list/policy_retention_schedule/policy_retention_schedule.tsx

jloleysens · 2019-10-02T11:09:59Z

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

    });
  }
+
+  public getTimeUnitLabel(timeUnit = TIME_UNITS.SECOND, timeValue = '0') {


Will we ever call just getTimeUnitLabel() and have a label 0 seconds? Just wondering whether the defaults are meaningful here and if we shouldn't require these values to be provided by consumers of this service.

good point, fixed

jloleysens · 2019-10-02T11:11:38Z

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


+  if (
+    retention &&
+    retention.minCount &&


Can minCount or maxCount ever be 0?

no, it must be at least 1

jloleysens · 2019-10-02T11:19:22Z

x-pack/legacy/plugins/snapshot_restore/__jest__/client_integration/policy_add.test.ts

+
+// We need to skip the tests until react 16.9.0 is released
+// which supports asynchronous code inside act()
+describe.skip('<PolicyAdd />', () => {


Just to clarify, none of the tests are currently running in CI - are we manually testing for now?

that's correct. we can remove the skip once we upgrade to react 16.9.0.

…etention

alisonelizabeth · 2019-10-02T14:37:56Z

@jloleysens thanks for the initial review! I believe I addressed all of your comments.

jloleysens

@alisonelizabeth

Great work, looks good! Summary of manual testing. Once again now major issues encountered:

Small note: The linked cron schedule docs go here: https://www.elastic.co/guide/en/elastic-stack-overview/master/trigger-schedule.html#schedule-cron which looks like it's been moved :c but you can still get there.

elasticmachine · 2019-10-02T15:42:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-10-02T16:09:26Z

Thanks @jloleysens! Great catch on the docs link. I updated it to the correct link.

elasticmachine · 2019-10-02T17:18:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-02T18:02:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mvenukadasula · 2020-05-15T18:02:34Z

@alisonelizabeth Is this the right place to add this comment or you want me to create new defect?

Snapshot retention policy defined from Kibana UI is not working as expected. Snapshots are not getting purged though retention policy expired. Tested for 1 hour and 1 day.

alisonelizabeth · 2020-05-15T18:46:45Z

Hi @uvenukadasula! Can you please open a "Bug report" issue in the Kibana repo here and provide steps to reproduce? Thanks!

alisonelizabeth added 2 commits September 9, 2019 13:56

adjust policy typings and serialization to support retention

03891ab

add snapshot retention step to policy wizard

33962bb

alisonelizabeth added release_note:enhancement 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.5.0 labels Sep 9, 2019

alisonelizabeth added 2 commits September 13, 2019 09:54

add retention cluster settings endpoint

4f1b75a

update expire_after field to use number field + select

468541c

alisonelizabeth added 2 commits September 13, 2019 15:01

add retention to policy table and policy details panel

c9a5001

add validation for min count field

283bc73

add policy stats to detail panel; move retention details to summary tab

e5b9f41

Merge branch 'master' of github.com:elastic/kibana into feature/slm-r…

612c8b2

…etention

add ability to update slm.retention_schedule cluster setting

b2b9fc1

alisonelizabeth force-pushed the feature/slm-retention branch from 70261e5 to b2b9fc1 Compare September 20, 2019 16:32

Merge branch 'master' of github.com:elastic/kibana into feature/slm-r…

a81b8f4

…etention

alisonelizabeth added 3 commits September 23, 2019 13:51

add slm stats endpoint

9ffa24f

remove retention tab on review step

cb41214

add policy stats to list view

972d57d

alisonelizabeth added 2 commits September 24, 2019 10:25

move retention schedule functionality to policy tab

3bef06a

fix existing tests

9c7427f

Merge branch 'master' into feature/slm-retention

87b033c

cuff-links self-requested a review September 30, 2019 19:33

cuff-links approved these changes Oct 1, 2019

View reviewed changes

Bamieh reviewed Oct 1, 2019

View reviewed changes

fix i18n ids

9ee698d

remove unused translations

beb70e7

Bamieh approved these changes Oct 2, 2019

View reviewed changes

jloleysens reviewed Oct 2, 2019

View reviewed changes

alisonelizabeth added 2 commits October 2, 2019 10:30

address review feedback

169afc0

Merge branch 'master' of github.com:elastic/kibana into feature/slm-r…

920addc

…etention

jloleysens approved these changes Oct 2, 2019

View reviewed changes

fix docs url and ts error

f879fef

fix ts linting error

a8c7c58

alisonelizabeth merged commit e81494f into elastic:master Oct 2, 2019

alisonelizabeth deleted the feature/slm-retention branch October 2, 2019 18:06

This was referenced Oct 2, 2019

[7.x] SR: SLM retention UI (#45193) #47145

Merged

[SR] add ability to execute snapshot retention manually #47150

Merged

alisonelizabeth added a commit that referenced this pull request Oct 2, 2019

[SR] SLM retention UI (#45193) (#47145)

0323980


		type OnSuccessCallback = () => void;

		export const PolicyUpdateRetentionProvider: React.FunctionComponent<Props> = ({ children }) => {

SR: SLM retention UI #45193

SR: SLM retention UI #45193

Uh oh!

Conversation

alisonelizabeth commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

Testing locally

Screenshots

Uh oh!

elasticmachine commented Sep 9, 2019

Uh oh!

elasticmachine commented Sep 9, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 13, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Sep 13, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Sep 16, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Sep 17, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 20, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Sep 23, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 23, 2019

💔 Build Failed

Uh oh!

alisonelizabeth commented Sep 30, 2019

Uh oh!

elasticmachine commented Sep 30, 2019

💚 Build Succeeded

Uh oh!

cuff-links commented Oct 1, 2019

Uh oh!

cuff-links left a comment

Choose a reason for hiding this comment

Uh oh!

Bamieh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alisonelizabeth commented Oct 1, 2019

Uh oh!

elasticmachine commented Oct 1, 2019

💚 Build Succeeded

Uh oh!

Bamieh left a comment

Choose a reason for hiding this comment

Uh oh!

jloleysens 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Sep 9, 2019 •

edited

Loading

alisonelizabeth commented May 15, 2020 •

edited

Loading