Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky e2e tests #2283

Merged
merged 1 commit into from
Apr 1, 2018
Merged

Fix flaky e2e tests #2283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions test/e2e/lua/dynamic_configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
Expect(err).NotTo(HaveOccurred())
Expect(ing).NotTo(BeNil())

time.Sleep(5 * time.Second)

Copy link
Member

@ElvinEfendi ElvinEfendi Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

err = f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, "proxy_pass http://upstream_balancer;")
Expand Down Expand Up @@ -195,7 +193,11 @@ func enableDynamicConfiguration(kubeClientSet kubernetes.Interface) error {
args = append(args, "--enable-dynamic-configuration")
deployment.Spec.Template.Spec.Containers[0].Args = args
_, err := kubeClientSet.AppsV1beta1().Deployments("ingress-nginx").Update(deployment)
return err
if err != nil {
return err
}
time.Sleep(15 * time.Second)
Copy link
Member

@ElvinEfendi ElvinEfendi Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil
})
}

Expand All @@ -211,7 +213,11 @@ func disableDynamicConfiguration(kubeClientSet kubernetes.Interface) error {
}
deployment.Spec.Template.Spec.Containers[0].Args = newArgs
_, err := kubeClientSet.AppsV1beta1().Deployments("ingress-nginx").Update(deployment)
return err
if err != nil {
return err
}
time.Sleep(15 * time.Second)
return nil
})
}

Expand Down