pkg/controller: start informers before populating store in tests #457
Conversation
Signed-off-by: Antonio Murdaca <[email protected]>
openshift-ci-robot
added
approved
|
CI glitch... /retest |
|
still glitch #457 (comment) |
|
/retest |
|
CI errors apart (not unit), this is fixing the flakes as my tests never failed in more than 5 hours |
|
the glitch is a CI issue |
|
/retest |
|
aws limit /retest |
|
LGTM, giving for another final review/merge: /assign kikisdeliveryservice |
|
lgtm (deferring to @kikisdeliveryservice for review). Nice find |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LorbusChris, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
HAProxy and other known flakes. /test e2e-aws |
|
known flake. fail [github.com/openshift/origin/test/extended/bootstrap_user/bootstrap_user_login.go:52]: Expected error:
<*util.ExitError | 0xc422561740>: {
Cmd: "oc login --config=/tmp/configfile916071508 --namespace=e2e-test-bootstrap-login-cpctm -u kubeadmin -p 5LpcuRoMeuNntNkAusTRa269NuAbQoU9ptn4aloHbp4",
StdErr: "Error from server (InternalError): Internal error occurred: unexpected response: 400",
ExitError: {
ProcessState: {
pid: 11753,
status: 256,
rusage: {
Utime: {Sec: 0, Usec: 268025},
Stime: {Sec: 0, Usec: 66749},
Maxrss: 94440,
Ixrss: 0,
Idrss: 0,
Isrss: 0,
Minflt: 15028,
Majflt: 0,
Nswap: 0,
Inblock: 0,
Oublock: 0,
Msgsnd: 0,
Msgrcv: 0,
Nsignals: 0,
Nvcsw: 1198,
Nivcsw: 27,
},
},
Stderr: nil,
},
}
exit status 1
not to have occurredopenshift/origin#22088 is moving it to flake |
|
Thanks for the update on that test, @abhinavdahiya ! |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
1 similar comment
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
CI errors tracked in slack /retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
This patch does various things, all related: - start the informers for controllers before running them to follow what https://github.com/kubernetes/sample-controller/blob/master/main.go#L64-L71 does and to avoid races already spotted in unit tests (openshift#457) - move the clients builder code from cmd/common to a new package just for that under the new internal/ folder so nobody but us can use that - move common controllers code under pkg/controller/common to be reused - create an e2e only clientset, avoiding us to type client version everytime (eg client.CoreV1()..). This is a need cause if in the future the api change, we don't play grep for a week replacing old apis... Signed-off-by: Antonio Murdaca <[email protected]>
9 participants
Signed-off-by: Antonio Murdaca [email protected]
- What I did
The indexer in the informer uses a store which manages object using a read-write lock, the flakes come from the fact that if we don't sync up the store before adding objects, the listing of objects can race against an add (verified this by adding a bunch of debug logs during investigation and I could spot that
lister.Listcan return empty even if we added objects to the indexer before starting the informers themselves).Fixes #417
Fixes #444
Fixes #451
Fixes #449
Cannot reproduce anymore with this patch (running in a loop since 3 hours now), I'm able to consistently reproduce by running
go test -racew/o this patch.- How to verify it
w/o this patch the command above fails as soon as the flake makes the test error out. With the patch, the above continues perpetually.
- Description for the changelog