Abort HA Realization Logic After Timeout #800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianbrost
reviewed
Sep 9, 2024
oxzi
force-pushed
the
ha-desperate-timing-fixes
branch
from
46a9b12 to
a03246f
Compare
oxzi
force-pushed
the
ha-desperate-timing-fixes
branch
from
a03246f to
c13752a
Compare
|
After going over this with @lippserd, I reworked this PR. The changes are, in a nutshell, that the potentially blocking |
lippserd
requested changes
Sep 24, 2024
oxzi
force-pushed
the
ha-desperate-timing-fixes
branch
from
c13752a to
3e78b92
Compare
oxzi
added a commit
to Icinga/icinga-go-library
that referenced
this pull request
Sep 24, 2024
A key finding of Icinga/icingadb#800 was that committing a transaction does not necessarily have to respect the context of the transaction, depending on the database driver. As @lippserd suggested there, I have added notes to the documentation of all relevant database functions.
lippserd
previously approved these changes
Sep 30, 2024
oxzi
added a commit
to Icinga/icinga-go-library
that referenced
this pull request
Oct 8, 2024
A key finding of Icinga/icingadb#800 was that committing a transaction does not necessarily have to respect the context of the transaction, depending on the database driver. As @lippserd suggested there, I have added notes to the documentation of all relevant database functions.
yhabteab
reviewed
Oct 22, 2024
This was referenced Oct 23, 2024
oxzi
force-pushed
the
ha-desperate-timing-fixes
branch
from
3e78b92 to
15f7be6
Compare
yhabteab
reviewed
Oct 24, 2024
lippserd
requested changes
Oct 24, 2024
There was a problem hiding this comment.
Although I approved earlier, I now have something that I had previously forgotten. It's not about the code itself, but about the commit(s). The changes here are subject to multiple commits:
- Changes in main
- Increasing the "heartbeat from future" log to warning
- Explicit rollback
- Inserting the environment inside the transaction
- Introduction and use of LastMessageTime() (plus the corresponding godoc paragraph of realize)
- Executing commit in a separate Go routine (plus its godoc paragraph of realize)
The main loop select cases for hactx.Done() and ctx.Done() were unified, as hactx is a derived ctx. A closed ctx case may be lost as the hactx case could have been chosen.
Timing issues may be the root of future failures. Thus, it is important to be aware if the timing seems to be out of sync.
Each transaction is created within the retryable function, but this function may be exited prematurely before committing. A deferred rollback ensures that the transaction will be rolled back and cleaned up in this case, or will be a noop when performed after the commit.
The HA.insertEnvironment() method was inlined into the retryable function to use the deadlined context. Otherwise, this might block afterwards, as it was used within HA.realize(), but without the passed context.
Since the retryable HA function may be executed a few times before succeeding, the inserted heartbeat value will be directly outdated. The heartbeat logic was slightly altered to always use the latest heartbeat time value.
A strange HA behavior was reported in #787, resulting in both instances being active. The logs contained an entry of the previous active instance exiting the HA.realize() method successfully after 1m9s. This, however, should not be possible as the method's context is deadlined to a minute after the heartbeat was received. However, as it turns out, executing COMMIT on a database transaction is not bound to the transaction's context, allowing to survive longer. To mitigate this, another context watch was introduced. Doing so allows directly handing over, while the other instance can now take over due to the expired heartbeat in the database.
oxzi
force-pushed
the
ha-desperate-timing-fixes
branch
from
15f7be6 to
8b95d25
Compare
yhabteab
approved these changes
Oct 25, 2024
|
Shouldn't this pull request close issue #787? |
This was the initial motivation, yes. However, as I was unable to reproduce the reported behavior, I cannot verify this. My intention was to make the observed bug impossible with this change, but maybe there is still another bug lurking around the corner. However, with a bit more trust in this change, I have linked this PR to the issue. Otherwise, we can reopen it. |
oxzi
added a commit
that referenced
this pull request
Dec 17, 2024
During #800, the commit dd0ca8f inlined the HA.insertEnvironment method into the retryable realization function. However, while doing so, I forgot to change the query execution context from h.db to tx. This resulted in an error when being used together with a single database connection, as introduced in #828. Co-Authored-By: Yonas Habteab <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main loop select cases for hactx.Done() and ctx.Done() were unified, as hactx is a derived ctx. A closed ctx case may be lost as the hactx case could have been chosen.
Timing issues may be the root of future failures. Thus, it is important to be aware if the timing seems to be out of sync.
Each transaction is created within the retryable function, but this function may be exited prematurely before committing. A deferred rollback ensures that the transaction will be rolled back and cleaned up in this case, or will be a noop when performed after the commit.
The HA.insertEnvironment() method was inlined into the retryable function to use the deadlined context. Otherwise, this might block afterwards, as it was used within HA.realize(), but without the passed context.
Since the retryable HA function may be executed a few times before succeeding, the inserted heartbeat value will be directly outdated. The heartbeat logic was slightly altered to always use the latest heartbeat time value.
A strange HA behavior was reported in Competing HA takeover results in both instances becoming active #787, resulting in both instances being active.
The logs contained an entry of the previous active instance exiting the HA.realize() method successfully after 1m9s. This, however, should not be possible as the method's context is deadlined to a minute after the heartbeat was received.
However, as it turns out, executing COMMIT on a database transaction is not bound to the transaction's context, allowing to survive longer. To mitigate this, another context watch was introduced. Doing so allows directly handing over, while the other instance can now take over due to the expired heartbeat in the database.
Closes #787.