fix monitor renew lock issue #1623

sbcd90 · 2024-07-26T19:13:07Z

Issue #, if available:

If a particular monitor run fails to release the lock, the lock can never be renewed and every monitor run is skipped.
Description of changes:

If a particular monitor run fails to release the lock, the lock can be renewed and acquired after a fixed timeout of 5 mins..

CheckList:

Commits are signed per the DCO using --signoff

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.

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 · 2024-07-26T19:38:00Z

hi @eirsep , plz review.

eirsep · 2024-07-29T21:23:01Z

core/src/main/kotlin/org/opensearch/alerting/core/lock/LockService.kt

                                            if (isLockReleased(existingLock)) {
                                                log.debug("lock is released or expired: {}", existingLock)
                                                val updateLock = LockModel(existingLock, getNow(), false)
                                                updateLock(updateLock, listener)
                                            } else {
-                                                log.debug("Lock is NOT released or expired. {}", existingLock)
-                                                listener.onResponse(null)
+                                                log.debug("Lock is NOT released. {}", existingLock)


plz add a test to verify expired locks are retrieved and deleted/re-used instead. create in now()- 10 mins

added a test. https://github.com/opensearch-project/alerting/pull/1623/files#diff-9b08d53e8cff3d739beb6ba304e4eaf466f08412762c3cd55886a52b722a77f1R2753

eirsep · 2024-07-29T21:23:35Z

core/src/main/kotlin/org/opensearch/alerting/core/lock/LockService.kt

@@ -37,6 +39,7 @@ class LockService(private val client: Client, private val clusterService: Cluste

    companion object {
        const val LOCK_INDEX_NAME = ".opensearch-alerting-config-lock"
+        val LOCK_EXPIRED_SECONDS = TimeValue(5, TimeUnit.MINUTES)


this is not holding seconds?

changed to mins.

eirsep · 2024-07-29T23:30:59Z

core/src/main/kotlin/org/opensearch/alerting/core/lock/LockService.kt

+                                                    val updateLock = LockModel(existingLock, getNow(), false)
+                                                    updateLock(updateLock, listener)
+                                                } else {
+                                                    log.debug("Lock is NOT expired. {}", existingLock)


plz explicitly mention we don't run Lock is NOT expired. Not running monitor

eirsep · 2024-07-29T23:45:41Z

alerting/src/main/kotlin/org/opensearch/alerting/MonitorRunnerService.kt

@@ -331,12 +331,12 @@ object MonitorRunnerService : JobRunner, CoroutineScope, AbstractLifecycleCompon
        when (job) {
            is Workflow -> {
                launch {
-                    var lock: LockModel? = null
+                    var workflowLock: LockModel? = null


change this to error log : https://github.com/opensearch-project/alerting/pull/1623/files#diff-7a7546a20937ab41ffd48d77f0d0ba9b9b6c5031289f2e2eb7543167b99701cbR236

and catch between try and finally for lock acquire logic

added catch between try & finally block.

engechas · 2024-07-30T03:27:50Z

core/src/main/kotlin/org/opensearch/alerting/core/lock/LockService.kt

+                                                ) {
+                                                    log.debug("Lock is expired. Renewing Lock {}", existingLock)
+                                                    val updateLock = LockModel(existingLock, getNow(), false)
+                                                    updateLock(updateLock, listener)


What happens if the previous node that owned the lock resumes its execution? i.e. node holding lock was unresponsive due to high traffic and temporarily left the cluster. After traffic subsides it rejoins the cluster

Related question - are there any race conditions with multiple nodes attempting to claim an expired lock?

hi @engechas , each lock is specific to a monitor. & if the node running the old job goes down, the job is cancelled. the monitor job is then reassigned to a new node & it can then try to renew the lock with this fix now.

Signed-off-by: Subhobrata Dey <[email protected]>

opensearch-trigger-bot · 2024-07-31T00:32:18Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.x
# Create a new branch
git switch --create backport-1623-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25eec30dbf0b327b8d7e9b684e7e14c6dec74ca4
# Push it to GitHub
git push --set-upstream origin backport-1623-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport-1623-to-2.x.

opensearch-trigger-bot · 2024-07-31T00:32:19Z

The backport to 2.15 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.15 2.15
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.15
# Create a new branch
git switch --create backport-1623-to-2.15
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25eec30dbf0b327b8d7e9b684e7e14c6dec74ca4
# Push it to GitHub
git push --set-upstream origin backport-1623-to-2.15
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.15

Then, create a pull request where the base branch is 2.15 and the compare/head branch is backport-1623-to-2.15.

opensearch-trigger-bot · 2024-07-31T00:32:21Z

The backport to 2.16 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/alerting/backport-2.16 2.16
# Navigate to the new working tree
pushd ../.worktrees/alerting/backport-2.16
# Create a new branch
git switch --create backport-1623-to-2.16
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 25eec30dbf0b327b8d7e9b684e7e14c6dec74ca4
# Push it to GitHub
git push --set-upstream origin backport-1623-to-2.16
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/alerting/backport-2.16

Then, create a pull request where the base branch is 2.16 and the compare/head branch is backport-1623-to-2.16.

eirsep · 2024-07-31T09:48:24Z

@sbcd90 plz backport to 2.15, 2.13,2.11, 2.16

fix monitor renew lock issue

93267b3

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 requested review from lezzago, AWSHurneyt, eirsep, getsaurabh02, praveensameneni, bowenlan-amzn, rishabhmaurya, engechas, riysaxen-amzn and jowg-amazon as code owners July 26, 2024 19:13

eirsep reviewed Jul 29, 2024

View reviewed changes

engechas reviewed Jul 30, 2024

View reviewed changes

sbcd90 added 2 commits July 30, 2024 23:32

address review comments

f84ef1c

Signed-off-by: Subhobrata Dey <[email protected]>

update security workflow ci

743ee1b

Signed-off-by: Subhobrata Dey <[email protected]>

eirsep approved these changes Jul 31, 2024

View reviewed changes

engechas approved these changes Jul 31, 2024

View reviewed changes

sbcd90 added backport 2.x backport 2.15 backport 2.16 labels Jul 31, 2024

sbcd90 merged commit 25eec30 into opensearch-project:main Jul 31, 2024
18 of 21 checks passed

opensearch-trigger-bot bot added the failed backport label Jul 31, 2024

sbcd90 mentioned this pull request Jul 31, 2024

[Backport 2.x] fix monitor renew lock issue #1627

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix monitor renew lock issue #1623

fix monitor renew lock issue #1623

sbcd90 commented Jul 26, 2024

sbcd90 commented Jul 26, 2024

eirsep Jul 29, 2024

sbcd90 Jul 31, 2024

eirsep Jul 29, 2024

sbcd90 Jul 31, 2024

eirsep Jul 29, 2024

sbcd90 Jul 31, 2024

eirsep Jul 29, 2024

eirsep Jul 29, 2024

sbcd90 Jul 31, 2024

engechas Jul 30, 2024

engechas Jul 30, 2024

sbcd90 Jul 30, 2024

opensearch-trigger-bot bot commented Jul 31, 2024

opensearch-trigger-bot bot commented Jul 31, 2024

opensearch-trigger-bot bot commented Jul 31, 2024

eirsep commented Jul 31, 2024 •

edited

Loading

fix monitor renew lock issue #1623

fix monitor renew lock issue #1623

Conversation

sbcd90 commented Jul 26, 2024

sbcd90 commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 31, 2024

opensearch-trigger-bot bot commented Jul 31, 2024

opensearch-trigger-bot bot commented Jul 31, 2024

eirsep commented Jul 31, 2024 • edited Loading

eirsep commented Jul 31, 2024 •

edited

Loading