Skip to content
Merged
Changes from all commits
Commits
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
27 changes: 21 additions & 6 deletions test/e2e/ha/ha_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,18 +131,33 @@ func getDbSidsFromClusterStatus(f *framework.Framework, deploy *appsv1.Deploymen
framework.ExpectNoError(err)
framework.ExpectHaveLen(pods.Items, int(*deploy.Spec.Replicas))

expectedCount := len(pods.Items)
dbServers := make(map[string]map[string]string)
for _, db := range [...]string{"nb", "sb"} {
ginkgo.By("Getting ovn" + db + " db server ids on all ovn-central pods")
ginkgo.By("Waiting for ovn" + db + " db cluster to show all servers on every ovn-central pod")
for pod := range slices.Values(pods.Items) {
stdout, stderr, err := framework.ExecShellInPod(context.Background(), f, pod.Namespace, pod.Name, cmdClusterStatus(db))
framework.ExpectNoError(err, fmt.Sprintf("failed to get ovn%s db status in pod %s: stdout = %q, stderr = %q", db, pod.Name, stdout, stderr))
status := parseClusterStatus(stdout)
framework.ExpectHaveLen(status.Servers, len(pods.Items), "unexpected number of servers in ovn%s db status in pod %s: stdout = %q, stderr = %q", db, pod.Name, stdout, stderr)
var lastStdout, lastStderr string
framework.WaitUntil(2*time.Second, 30*time.Second, func(_ context.Context) (bool, error) {
stdout, stderr, err := framework.ExecShellInPod(context.Background(), f, pod.Namespace, pod.Name, cmdClusterStatus(db))
if err != nil {
return false, nil
}
lastStdout, lastStderr = stdout, stderr
var count int
for line := range strings.SplitSeq(stdout, "\n") {
if slices.Contains(strings.Fields(line), "at") {
count++
}
}
return count == expectedCount, nil
}, fmt.Sprintf("ovn%s db on pod %s to show %d servers", db, pod.Name, expectedCount))

status := parseClusterStatus(lastStdout)
framework.ExpectHaveLen(status.Servers, expectedCount, "unexpected number of servers in ovn%s db status in pod %s: stdout = %q, stderr = %q", db, pod.Name, lastStdout, lastStderr)
Comment on lines +140 to +156
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There are a couple of improvements that can be made here:

  1. The logic to check for cluster convergence inside WaitUntil duplicates the server counting logic from parseClusterStatus. You can simplify this by calling parseClusterStatus inside the WaitUntil condition. This also makes the ExpectHaveLen check on line 156 redundant.
  2. The framework.WaitUntil function ignores its first argument and uses a hardcoded 2-second polling interval. The current code works by coincidence because the interval you pass matches the hardcoded one, but this is fragile.

Here is a suggested refactoring that addresses both points. It uses parseClusterStatus inside the wait loop and also adds a comment to clarify the behavior of WaitUntil.

			var status *clusterStatus
			// The first argument to WaitUntil is ignored; the poll interval is hardcoded to 2s.
			framework.WaitUntil(2*time.Second, 30*time.Second, func(_ context.Context) (bool, error) {
				stdout, stderr, err := framework.ExecShellInPod(context.Background(), f, pod.Namespace, pod.Name, cmdClusterStatus(db))
				if err != nil {
					return false, nil
				}
				lastStdout, lastStderr = stdout, stderr
				status = parseClusterStatus(stdout)
				return len(status.Servers) == expectedCount, nil
			}, fmt.Sprintf("ovn%s db on pod %s to show %d servers", db, pod.Name, expectedCount))

if len(dbServers[db]) == 0 {
dbServers[db] = maps.Clone(status.Servers)
} else {
framework.ExpectEqual(status.Servers, dbServers[db], "inconsistent servers in ovn%s db status in pod %s: stdout = %q, stderr = %q", db, pod.Name, stdout, stderr)
framework.ExpectEqual(status.Servers, dbServers[db], "inconsistent servers in ovn%s db status in pod %s: stdout = %q, stderr = %q", db, pod.Name, lastStdout, lastStderr)
}
}
}
Expand Down
Loading