search

opensearch-project / index-management

🗃 Automate periodic data operations, such as deleting indices at a certain age or performing a rollover at a certain size
https://opensearch.org/docs/latest/im-plugin/index/
Apache License 2.0
53 stars 108 forks source link

GET SM policies return empty list when ism config index does not exist #1072

Closed aggarwalShivani closed 5 months ago

aggarwalShivani commented 6 months ago

Fix for GET request on SM policies to return empty list when ism config index does not exist

Issue #, if available: #1055

Description of changes: When a GET request is issued to snapshot mananagement API endpoint, to list all available sm policies, it throws an IndexNotFound exception if the ism config index is not yet created. (Note - This index gets created only after atleast one ism/sm policy gets created). To align the behaviour to ism policies endpoint (that responds with an empty list for missing config index cases), the change is done here for SM policies. In success case, searchResponse is returned like done before. In the failure case (for missing config index), instead of throwing OpenSearchStatusException, an empty list of 0 Length is returned from the getAllPolicies function. If any other exception is caught, then that exception is thrown as-is.

CheckList:

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.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6270b2c) 74.86% compared to head (7a63a9d) 74.90%.

Files Patch % Lines
.../api/transport/get/TransportGetSMPoliciesAction.kt 50.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1072 +/- ## ============================================ + Coverage 74.86% 74.90% +0.04% - Complexity 2808 2814 +6 ============================================ Files 367 367 Lines 16518 16522 +4 Branches 2362 2363 +1 ============================================ + Hits 12366 12376 +10 + Misses 2853 2843 -10 - Partials 1299 1303 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Hailong-am commented 5 months ago

@bowenlan-amzn would like to confirm with you, return empty list meet your expectation as well

bowenlan-amzn commented 5 months ago

@Hailong-am Yes, this is the behavior of ISM. Though rollup and transform doesn't catch IndexNotFoundException

aggarwalShivani commented 5 months ago

Hi reviewers, Thanks for the feedback so far. I'm facing difficulty in understanding some failures in checks running on the github CI for my PR. Its my first PR to this project and would highly appreciate any pointers and guidance. 4 out of 30 checks have failed. I've put some details below -

i. Docker Security Test Workflow

WARNING: Please consider reporting this to the maintainers of org.opensearch.bootstrap.Security
uncaught exception in thread [main]
java.lang.IllegalStateException: failed to load plugin class [org.opensearch.security.OpenSearchSecurityPlugin]
Likely root cause: OpenSearchException[plugins.security.ssl.transport.keystore_filepath or plugins.security.ssl.transport.server.pemcert_filepath and plugins.security.ssl.transport.client.pemcert_filepath must be set if transport ssl is requested.]

Analysis - This looks like an issue with security plugin, unrelated to the changes made in this PR. Any thoughts on this?

ii. In both these cases, Test and Build Workflow / test-and-build-linux (21, ism) and Test and Build Workflow / test-and-build-linux (21, non-ism), all failures have the same exception ->

   Caused by:
        java.net.SocketTimeoutException: 60000 MILLISECONDS
java.lang.AssertionError: Fri Jan 19 15:23:33 ART 2024: There are still tasks running after this test that might break subsequent tests:

I have run some of these testcases individually on my local setup, and they passed. Any pointers on the root cause here? Are they expected?

iii. codecov/patch failed with warning - Added lines #L78 - L79 were not covered by tests. But the test-case added, does cover the mentioned case. I'll perhaps recheck this after changes get finalized for TransportGetSMPoliciesAction.kt

Hailong-am commented 5 months ago

ii. In both these cases, Test and Build Workflow / test-and-build-linux (21, ism) and Test and Build Workflow / test-and-build-linux (21, non-ism), all failures have the same exception ->

we can ignore jdk 21 failure, it's known issue. Reference https://github.com/opensearch-project/flow-framework/issues/426

Docker Security Test Workflow

Related to https://github.com/opensearch-project/index-management/issues/1064, will have another PR https://github.com/opensearch-project/index-management/pull/1076 to handle it.

aggarwalShivani commented 5 months ago

Thanks @Hailong-am and @bowenlan-amzn for the inputs and help here :)