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

[Remote State] fix lock release before deletion is completed #10611

Conversation

linuxpi
Copy link
Collaborator

@linuxpi linuxpi commented Oct 13, 2023

Description

  • We have lock to ensure only one cleanup task runs to cleanup stale manifests within a cluster UUID.
  • But we were releasing this lock before the delete task completes
  • This PR fixes that and lock is only released after completion or failure of deletion task.

Related Issues

Resolves #10586

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT:
  • URL:
  • CommitID: 22e940c
    Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
    Is the failure a flaky test unrelated to your change?

@github-actions github-actions bot added the bug Something isn't working label Oct 13, 2023
@linuxpi linuxpi marked this pull request as ready for review October 13, 2023 10:02
@linuxpi linuxpi changed the title fix lock release before deletion is completed [Remote State] fix lock release before deletion is completed Oct 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2023

Compatibility status:

Checks if related components are compatible with change 52e6621

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git]

@dblock
Copy link
Member

dblock commented Oct 13, 2023

Needs a test, please.

Copy link
Collaborator

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

💯 on requiring test

@linuxpi
Copy link
Collaborator Author

linuxpi commented Oct 16, 2023

Needs a test, please.

💯 on requiring test

Thanks @dblock @Bukhtawar, will add test!

@linuxpi linuxpi force-pushed the fix-remote-cluster-state-manifest-deletion branch from 22e940c to c427c21 Compare October 20, 2023 09:05
Signed-off-by: bansvaru <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #10611 (be658ad) into main (69f6f4e) will increase coverage by 0.00%.
Report is 5 commits behind head on main.
The diff coverage is 89.03%.

❗ Current head be658ad differs from pull request most recent head 52e6621. Consider uploading reports for the commit 52e6621 to get more accurate results

@@            Coverage Diff             @@
##               main   #10611    +/-   ##
==========================================
  Coverage     71.11%   71.11%            
+ Complexity    58530    58514    -16     
==========================================
  Files          4854     4858     +4     
  Lines        276049   276181   +132     
  Branches      40168    40184    +16     
==========================================
+ Hits         196311   196408    +97     
+ Misses        63385    63366    -19     
- Partials      16353    16407    +54     
Files Coverage Δ
.../action/search/SearchQueryCategorizingVisitor.java 100.00% <100.00%> (ø)
.../opensearch/action/search/SearchQueryCounters.java 100.00% <100.00%> (ø)
...rg/opensearch/common/settings/ClusterSettings.java 92.85% <ø> (ø)
...search/index/shard/RemoteStoreRefreshListener.java 92.39% <100.00%> (+6.29%) ⬆️
...ensearch/action/search/SearchQueryCategorizer.java 96.77% <96.77%> (ø)
...in/java/org/opensearch/index/shard/IndexShard.java 70.23% <94.44%> (+0.81%) ⬆️
...arch/gateway/remote/RemoteClusterStateService.java 69.74% <33.33%> (+0.19%) ⬆️
.../org/opensearch/index/query/QueryShapeVisitor.java 94.73% <94.73%> (ø)
...ain/java/org/opensearch/search/DocValueFormat.java 59.17% <0.00%> (-5.33%) ⬇️
...pensearch/action/search/TransportSearchAction.java 66.60% <42.85%> (-2.25%) ⬇️

... and 458 files with indirect coverage changes

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=pit/10_basic/Delete all}
      1 org.opensearch.client.PitIT.testDeleteAllAndListAllPits

Signed-off-by: bansvaru <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@shwetathareja shwetathareja merged commit c400d84 into opensearch-project:main Oct 20, 2023
13 of 14 checks passed
@shwetathareja shwetathareja added the backport 2.x Backport to 2.x branch label Oct 20, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 20, 2023
* fix lock release before deletion is completed

Signed-off-by: bansvaru <[email protected]>
(cherry picked from commit c400d84)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@linuxpi linuxpi deleted the fix-remote-cluster-state-manifest-deletion branch October 20, 2023 12:35
shwetathareja pushed a commit that referenced this pull request Oct 22, 2023
…#10778)

* fix lock release before deletion is completed

(cherry picked from commit c400d84)

Signed-off-by: bansvaru <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Oct 23, 2023
…rch-project#10611)

* fix lock release before deletion is completed

Signed-off-by: bansvaru <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…rch-project#10611)

* fix lock release before deletion is completed

Signed-off-by: bansvaru <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch bug Something isn't working skip-changelog
Projects
None yet
5 participants