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

fix snapshot restore and recovery endpoint #611

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

hogesako
Copy link
Contributor

Description

Fixed snapshot restore endpoint when wait_for_completion is false.
Also fixed _recovery endpoint needed for testing when wait_for_completion is false.

Issues Resolved

None

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

DCO is failing + some nits

@@ -427,10 +427,11 @@ components:
schema:
type: object
properties:
accepted:
description: Equals `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Equals `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`
description: Returns `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unified with existing comments.
Should the existing comments also be amended?
https:/opensearch-project/opensearch-api-specification/blob/main/spec/namespaces/snapshot.yaml#L386

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

spec/schemas/indices.recovery.yaml Show resolved Hide resolved
Copy link

github-actions bot commented Oct 12, 2024

Changes Analysis

Commit SHA: c483acb
Comparing To SHA: 5dc65a5

API Changes

Summary

├─┬Paths
│ ├─┬/_snapshot/{repository}/{snapshot}/_restore
│ │ └─┬POST
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └─┬Schema
│ │           ├──[➖] required (29753:17)❌ 
│ │           └──[➕] properties (29750:15)
│ └─┬/_snapshot/{repository}/{snapshot}
│   ├─┬PUT
│   │ └─┬Responses
│   │   └─┬200
│   │     └─┬application/json
│   │       └─┬Schema
│   │         └─┬accepted
│   │           └──[🔀] description (29711:30)
│   └─┬POST
│     └─┬Responses
│       └─┬200
│         └─┬application/json
│           └─┬Schema
│             └─┬accepted
│               └──[🔀] description (29711:30)
└─┬Components
  └─┬indices.recovery:RecoveryOrigin
    ├──[➕] properties (48612:9)
    ├──[➕] properties (48614:9)
    ├──[➕] properties (48616:9)
    └──[➕] properties (48620:9)

Document Element Total Changes Breaking Changes
paths 4 1
components 4 0
  • BREAKING Changes: 1 out of 8
  • Modifications: 2
  • Removals: 1
  • Additions: 5
  • Breaking Removals: 1

Report

The full API changes report is available at: https:/opensearch-project/opensearch-api-specification/actions/runs/11366888933/artifacts/2063666302

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 29 29 0

Copy link

Spec Test Coverage Analysis

Total Tested
504 317 (62.9 %)

Signed-off-by: Tatsuya Kawakami <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Some more small stuff, LMK when it's ready! Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
tests/snapshot/snapshot/restore.yaml Outdated Show resolved Hide resolved
tests/snapshot/snapshot/restore.yaml Outdated Show resolved Hide resolved
@hogesako
Copy link
Contributor Author

@dblock
Thank you for your review.
I think the work is complete.
If there is any work that needs to be done, please let me know.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Sorry one more nit. It should say "Returns" in the description and let's split the test file.

Thanks for opening opensearch-project/OpenSearch#16334.

@@ -383,7 +383,7 @@ components:
type: object
properties:
accepted:
description: Equals `true` if the snapshot was accepted. Present when the request had `wait_for_completion` set to `false`
description: Return `true` if the snapshot was accepted. Present when the request had `wait_for_completion` set to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Return `true` if the snapshot was accepted. Present when the request had `wait_for_completion` set to `false`.
description: Returns `true` if the snapshot was accepted. Present when the request had `wait_for_completion` set to `false`.

@@ -427,10 +427,11 @@ components:
schema:
type: object
properties:
accepted:
description: Return `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Return `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`.
description: Returns `true` if the restore was accepted. Present when the request had `wait_for_completion` set to `false`.

status: 200
payload:
accepted: true
- synopsis: Wait finish async restore.
Copy link
Member

@dblock dblock Oct 16, 2024

Choose a reason for hiding this comment

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

This should live in its own file, so restore.yaml and recovery.yaml where the snapshot would be a prologue. This way you don't need multiple-paths-detected warning suppression and the sample can be used in a separate documentation page for _recovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following error occurs when a snapshot is deleted in the epilogue immediately after an asynchronous restore.

ERROR   EPILOGUES
        ERROR   DELETE /_snapshot/{repository}/{snapshot} ([my-fs-repository:my-test-snapshot/KIVGLRXyR168qOKeAS1sIw] cannot delete snapshot during a restore in progress in [RestoreInProgress[{9gz8AVvDRPm9e2ZU8Gdw9A}{my-fs-repository:my-test-snapshot/KIVGLRXyR168qOKeAS1sIw}]])

Therefore, I placed the test for the _recovery endpoint in the chapters to wait for the restore to complete(response cannot be specified in epilogue).
I would be grateful if you could let me know if there is a better method.

Copy link
Member

Choose a reason for hiding this comment

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

We can leave it like this for now, no big deal.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please open an issue describing this? I think epilogues should be able to deal with the response, too.

@dblock dblock merged commit 99bc701 into opensearch-project:main Oct 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants