bump defaultReconciliationTimeout to 60 sec#36788
bump defaultReconciliationTimeout to 60 sec#36788andrewhsu wants to merge 1 commit intomoby:masterfrom andrewhsu:l
Conversation
To accomodate for increase in leader election from 3 seconds to 10 seconds. This will give more cycles to find another leader. Signed-off-by: Andrew Hsu <[email protected]>
cyli
left a comment
cyli
left a comment
There was a problem hiding this comment.
LGTM - I'm not sure if this bumps the tests up enough - it was 30 seconds before, which means that if the leader election tick were 3 seconds, it's possible to get up to 10 leader elections. With 60 seconds and a 10 second election tick, it's possible to get up to 6.
I think this will be sufficient, but just wanted to mention that it doesn't give us the exact number of possible elections we might have had before.
| ) | ||
|
|
||
| var defaultReconciliationTimeout = 30 * time.Second | ||
| var defaultReconciliationTimeout = 60 * time.Second |
There was a problem hiding this comment.
Please add a comment for how this timeout is used.
|
@anshulpundir for example: docker-archive/docker-ce#492 (comment) |
Codecov Report
@@ Coverage Diff @@
## master #36788 +/- ##
=========================================
- Coverage 35.22% 34.93% -0.3%
=========================================
Files 614 614
Lines 45645 45645
=========================================
- Hits 16080 15944 -136
- Misses 27435 27607 +172
+ Partials 2130 2094 -36 |
|
If this is for a specific test then there is no need to raise it for everything. We already raise it in couple of places where needed, eg. |
|
I agree with @tonistiigi's suggestion to only raise it for the test in question. @andrewhsu |
|
I am fine with bumping it up only for this test - but there might be some other tests that do not specifically test leader election, but may trigger leader elections (because we are taking down nodes), which could become more flaky as well. Update: Actually I think I may be confusing the swarmkit integration tests with these, the two in particular that I was thinking of do seem to try to preserve the leader. So possibly not an issue, but something to keep in mind. |
|
Agree with @cyli. We can always change increase the timeout for tests as and when necessary. BTW, there are concerns that we might be masking bugs. And so maybe try with a smaller timeout increase ? |
8 participants
- What I did
bump
defaultReconciliationTimeoutfrom 30 sec to 60 secTo accommodate for increase in leader election from 3 seconds to 10
seconds. This will give more cycles to find another leader.
- How I did it
in the tests
- How to verify it
run
DockerSwarmSuite.TestAPISwarmLeaderElectiontest- Description for the changelog
N/A
- A picture of a cute animal (not mandatory but encouraged)
🐋