[17.06] orchestrator/update: Only shut down old tasks on success#2433
Conversation
|
@nishanttotla is the test failure related ? |
|
@vieux I'm not sure, it looks like the test is flaky. I'll test it locally, we may have to restart the CI. |
|
Test fails locally as well, it might be related to this change. I'll need to investigate that, because the test doesn't fail on master. |
|
So I ran tests again locally and they don't fail. It might just be a flaky test 🤔 . Checked on the master branch too, the test file is identical there. |
56e0834 to
1f6c479
Compare
Codecov Report
@@ Coverage Diff @@
## bump_v17.06 #2433 +/- ##
===============================================
+ Coverage 59.66% 65.65% +5.98%
===============================================
Files 120 88 -32
Lines 25128 17133 -7995
===============================================
- Hits 14993 11248 -3745
+ Misses 8796 4956 -3840
+ Partials 1339 929 -410 |
In start-then-stop mode, the updater waits for a verdict on the startup of a new task, and then shuts down the tasks that this new task was meant to replace. It should really only shut down the old task if the new task successfully reached running. The thinking here was probably that a task which fails on startup would briefly enter the running state before failing, so there isn't much point in distinguishing between running and some terminal state. However, if a health check is being used, the task may never enter the running state. This can also happen when a task is rejected by a node. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com> (cherry picked from commit 6728c52) Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
1f6c479 to
ca31e9f
Compare
|
I don't know what test failed originally. The one failing now doesn't seem like it can be related. |
|
i pulled down this PR and ran |
|
LGTM |
|
Same as @dperny, tests passed locally for me, so the failing test is likely flaky on the CI. |
Cherry-pick #2308.
git cherry-pick -s -x 6728c52055ba639312a293306cf58ce4be5d2aa6Cherry-pick was clean.