template: lock task handle before trying script check #23917
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
tgross
added
type/bug
theme/crash
theme/template
backport/ent/1.6.x+ent
tgross
force-pushed
the
template-handle-lock
branch
from
4f0f72e to
e09048c
Compare
tgross
commented
Sep 4, 2024
Comment on lines
+1224
to
+1245
| // remove the reference to the task handle and update the template contents | ||
| // again | ||
| harness.manager.SetDriverHandle(nil) | ||
| harness.consul.SetKV(t, key1, []byte(content1_3)) | ||
| timeout = time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second) | ||
|
|
||
| OUTER2: | ||
| for { | ||
| select { | ||
| case <-harness.mockHooks.RestartCh: | ||
| t.Fatal(t, "restart not expected") | ||
| case ev := <-harness.mockHooks.EmitEventCh: | ||
| if strings.Contains( | ||
| ev.DisplayMessage, "task driver handle is not available") { | ||
| break OUTER2 | ||
| } | ||
| case <-harness.mockHooks.SignalCh: | ||
| t.Fatal(t, "signal not expected") | ||
| case <-timeout: | ||
| t.Fatal(t, "should have received an event that task driver handle is unavailable") | ||
| } | ||
| } |
There was a problem hiding this comment.
Note for reviewers: unfortunately this test doesn't really exercise the bug that must have happened in #23875, because we'd have to concurrently make the change between the nil check in processScript and the subsequent Exec call, which is a very short race. Locally this test doesn't add anything meaningful to the time it takes the test to complete (always around ~3.2s on my machine), but I simultaneously hate to have a test that doesn't really test anything and a bugfix without a new test. I could go either way on removing it.
pkazmierczak
approved these changes
Sep 11, 2024
tgross
added a commit
that referenced
this pull request
Sep 24, 2024
When we introduced change_mode=script to templates, we passed the driver handle down into the template manager so we could call its `Exec` method directly. But the lifecycle of the driver handle is managed by the taskrunner and isn't available when the template manager is first created. This has led to a series of patches trying to fixup the behavior (#15915, #15192, #23663, #23917). Part of the challenge in getting this right is using an interface to avoid the circular import of the driver handle. But the taskrunner already has a way to deal with this problem using a "lazy handle". The other template change modes already use this indirectly through the `Lifecycle` interface. Change the driver handle `Exec` call in the template manager to a new `Lifecycle.Exec` call that reuses the existing behavior. This eliminates the need for the template manager to know anything at all about the handle state. Fixes: #24051
tgross
added a commit
that referenced
this pull request
Sep 25, 2024
When we introduced change_mode=script to templates, we passed the driver handle down into the template manager so we could call its `Exec` method directly. But the lifecycle of the driver handle is managed by the taskrunner and isn't available when the template manager is first created. This has led to a series of patches trying to fixup the behavior (#15915, #15192, #23663, #23917). Part of the challenge in getting this right is using an interface to avoid the circular import of the driver handle. But the taskrunner already has a way to deal with this problem using a "lazy handle". The other template change modes already use this indirectly through the `Lifecycle` interface. Change the driver handle `Exec` call in the template manager to a new `Lifecycle.Exec` call that reuses the existing behavior. This eliminates the need for the template manager to know anything at all about the handle state. Fixes: #24051
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In #23663 we fixed the template hook so that
change_mode="script"didn't lose track of the task handle during restores. But this revealed a second bug which is that access to the handle is not locked while in use, which can allow it to be removed concurrently.Fixes: #23875
Note for reviewers: we're still waiting on we can get some reproduction hints from the reporter, but the missing lock is clearly a bug anyways.