Fix watchdog usage in controller unit tests #250
Conversation
Do not exit the Start method with an error in case of a peers update problem, it - potentially breaks watchdog related unit tests, because the k8s manager will stop all components, which disarms the watchdog - will cause a pod restart in production for no reason Retry with a quicker interval instead. Signed-off-by: Marc Sluiter <[email protected]>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slintes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test 4.17-openshift-e2e |
|
Looking at this briefly on my phone and it looks very familiar to what I was working on trying to accomplish on my local branch, and was having issues getting fully correct! Thanks for putting this in, I should be able to rebase off this and I think it will do the trick. Regards, |
| quickUpdateWorkerPeersError := p.updateWorkerPeers(ctx) | ||
| quickUpdateControlPlanePeersError := p.updateControlPlanePeers(ctx) | ||
| if quickUpdateWorkerPeersError == nil && quickUpdateControlPlanePeersError == nil { | ||
| quickCancel() |
There was a problem hiding this comment.
hm, does this cancel the parent context as well? That would be bad!
There was a problem hiding this comment.
It doesn't, we're good here. Would be terrifying if a child can cancel the parent's and with that all sibling's contexts, right? 🙈
pkg/watchdog/watchdog_test.go
Outdated
| wd.Stop() | ||
| }) | ||
|
|
||
| It("should be triggered and and not be fed anymore", func() { |
There was a problem hiding this comment.
typo: "and" is repeated twice.
pkg/watchdog/watchdog_test.go
Outdated
| }) | ||
|
|
||
| Context("Watchdog started", func() { | ||
| It("should be feeded", func() { |
There was a problem hiding this comment.
Maybe it's old code, but I think it's a typo: s/feeded/fed. Same below
pkg/watchdog/watchdog_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| Context("Triggered watchdog resetted", func() { |
pkg/watchdog/watchdog_test.go
Outdated
| wd.Reset() | ||
| }) | ||
|
|
||
| It("should be armed and and be feeded", func() { |
There was a problem hiding this comment.
typo: "add" repeated twice
This is the right place for testing feeding Signed-off-by: Marc Sluiter <[email protected]>
slintes
force-pushed
the
fix-unit-test
branch
from
98bd586 to
6a76bd9
Compare
|
/lgtm |
|
/hold @mshitrit in case you want to review |
openshift-ci
bot
added
do-not-merge/hold
and removed
do-not-merge/work-in-progress
labels
- Do not test watchdog feeding (is in new watchdog unit tests now) - Test watchdog status instead - Reset watchdog before each test! Otherwise we get false positives, because earlier tests triggered the watchdog already! - Fix "Unhealthy node without api-server access" test: the watchdog should NOT be triggered! Signed-off-by: Marc Sluiter <[email protected]>
slintes
force-pushed
the
fix-unit-test
branch
from
6a76bd9 to
c674511
Compare
|
/hold cancel |
Why we need this PR
Controller unit tests are broken. They have false positives because they all use the same watchdog.
Also, one unit test is testing for the wrong result.
Changes made
For details see commit messages.
Which issue(s) this PR fixes
Fixes issues with #238
Test plan
There are new and fixed tests