test/e2e: More refactoring #765
Conversation
openshift-ci-robot
added
size/L
test/e2e/mcd_test.go
Outdated
There was a problem hiding this comment.
isn't thi racy if we jump here before the pool starts rolling? I believe that's why I went node by node confirming current is there and of the new pool
There was a problem hiding this comment.
ok, you added a check below but this may flake then?
There was a problem hiding this comment.
Just above that we do waitForRenderedConfig right?
There was a problem hiding this comment.
For the record though I didn't test this much locally, created a PR to have CI do it.
There was a problem hiding this comment.
oh yeah, I had an old master on my laptop and didn't notice that, cool yeah, no flake no race 🎉
|
Hum, so that run failed; added some more debug logs to help me figure it out. Offhand...I think there is a race here because the tests were relying on checking for config=X and status=Updated, but we don't update those atomically. Really...it feels like we should have |
|
would checking if state is Done before checking that they match clarify what's happening (ie switch the 2 asserts?)
|
|
OK this test passes "locally" now - I added a |
|
Hooray, e2e-aws-op passed! #773 should help us avoid the race. |
|
ive seen these weird e2e-aws-upgrade failures before, checking them out elsewhere. let's try again: |
Rather than poll all of the daemons, add a helper that waits for a pool to complete a config. One of our tests walks over the MCDs, change it to just assert on all of the nodes. The SSH test can also just wait for a pool and then `rsh` to each node.
|
Rebased 🏄♂️ and CI is good, can I get a lgtm? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, 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 |
See openshift#765 (comment) MachineConfigPool needs a `Spec.Configuration` and `Status.Configuration` [just like other objects][1] so that we can properly detect state. Currently there's a race because the render controller may set `Status.Configuration` while the pool's `Status` still has `Updated`, so one can't reliably check whether the pool is at a given config. With this, ownership is clear: the render controller sets the spec, and the node controller updates the status. [1] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status)
See openshift#765 (comment) MachineConfigPool needs a `Spec.Configuration` and `Status.Configuration` [just like other objects][1] so that we can properly detect state. Currently there's a race because the render controller may set `Status.Configuration` while the pool's `Status` still has `Updated`, so one can't reliably check whether the pool is at a given config. With this, ownership is clear: the render controller sets the spec, and the node controller updates the status. [1] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status)
5 participants
Rather than poll all of the daemons, add a helper that waits
for a pool to complete a config.
One of our tests walks over the MCDs, change it to just assert
on all of the nodes.
The SSH test can also just wait for a pool and then
rshtoeach node.