Fix flaky e2e tests

[alvaroaleman]

What this PR does / why we need it:

e2e tests are currently flaky due to #2254 introducting tests that redeploy the ingress controller but only sometimes wait afterwards which may cause subsequent tests to fail.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

[codecov-io]

Codecov Report

Merging #2283 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2283   +/-   ##
======================================
  Coverage    37.3%   37.3%           
======================================
  Files          71      71           
  Lines        5042    5042           
======================================
  Hits         1881    1881           
  Misses       2876    2876           
  Partials      285     285

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee46f48...c6f1763. Read the comment docs.

[ElvinEfendi]

why is this necessary considering we wait for the pods to become "ready" at

ingress-nginx/test/e2e/framework/framework.go

Line 273 in ee46f48

err = WaitForPodsReady(kubeClientSet, 60*time.Second, replicas, namespace, metav1.ListOptions{

after this function executed?

[alvaroaleman]

Because it matches on the old generation of pods and instantly returns

[ElvinEfendi]

Good catch, thanks!

--

I wonder if it would be better to use WaitForNginxServer here to assert for a dynamic feature specific Nginx configuration(i.e the presence of upstream upstream_balancer)? Similarly we can assert absence of upstream upstream_balancer in disableDynamicConfiguration. That would save us some seconds.

[alvaroaleman]

Yeah, that's indeed a good idea.

I toyed a little around with checking if the deployments Status.UpdatedReplicas matches Spec.Replicas but that resulted in the instant return as well, apparently the controller sets updatedReplicas as soon as the new pod is in state creating and not when its ready.

I wonder what the best approach to check if a new generation of a deployment was fully rolled out is.

[ElvinEfendi]

Maybe as a more generic solution we could get the list of currently running pod names at

ingress-nginx/test/e2e/framework/framework.go

Line 264 in ee46f48

deployment, err := kubeClientSet.AppsV1beta1().Deployments(namespace).Get(name, metav1.GetOptions{})

and then path them as a list to WaitForPodsReady and in that function on top of requiring given number of running pods also require that the names of pods are not in the list of old pods(if any given). That way before returning in WaitForPodsReady we would also guarantee that it is the new pods that are ready.

[ElvinEfendi]

I totally agree this is really not great to sleep here but the reason I added it here was that I sometimes was getting timeout in the below WaitForNginxServer function where it was not able to find ready ingress-nginx-controller pod.

[aledbf]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, alvaroaleman

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:

• OWNERS [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aledbf]

@alvaroaleman thanks!