fix(e2e): reduce E2E test flakiness (sandbox events, duplicate CSE timing)#8480
Open
r2k1 wants to merge 1 commit into
Open
fix(e2e): reduce E2E test flakiness (sandbox events, duplicate CSE timing)#8480r2k1 wants to merge 1 commit into
r2k1 wants to merge 1 commit into
Conversation
r2k1
requested review from
AbelHu,
Devinwong,
SriHarsha001,
awesomenix,
calvin197,
cameronmeissner,
djsly,
ganeshkumarashok,
junjiezhang1997,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
sulixu,
surajssd,
timmy-wright and
zachary-bailey
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets major sources of E2E test flakiness by tightening readiness gating, reducing validator false positives, and stabilizing timing-based assertions. It also extends scenario mutator hooks to receive cluster context, enabling scenarios that depend on cluster-level derived values (e.g., a proxy URL).
Changes:
- Make node readiness waiting more accurate by requiring cloud-provider uninitialized taint removal before proceeding.
- Reduce false-positive failures by expanding the eBPF/iptables allowlist and deduplicating CSE timing events; relax overly tight CSE perf thresholds.
- Update E2E scenario mutator function signatures to accept
*Cluster, adjust specific scenarios (e.g., Flatcar AzureCNI), and skip a consistently failing Ubuntu 20.04 FIPS lane.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/kube.go | Improves node readiness gating; adds proxy ConfigMap/DaemonSet + proxy discovery; fixes DaemonSet CreateOrUpdate mutation. |
| e2e/cluster.go | Adds cluster-level ProxyURL plumbing, sets up private DNS for API server, and broadens retryable cluster-create errors. |
| e2e/aks_model.go | Adds 409-conflict handling/waits for private DNS zone and VNet link creation. |
| e2e/validation.go | Allows a standard DHCP INPUT rule in the eBPF-host-routing iptables compatibility allowlist. |
| e2e/cse_timing.go | Deduplicates extracted CSE event timings that can appear in multiple event directories. |
| e2e/scenario_cse_perf_test.go | Adjusts full-install timing thresholds (notably installDeps) and updates mutator signatures. |
| e2e/types.go | Updates BootstrapConfigMutator / AKSNodeConfigMutator signatures to include *Cluster. |
| e2e/test_helpers.go | Threads *Cluster through mutator invocations (including pre-provision flow). |
| e2e/scenario_test.go | Removes forced Azure CNI plugin settings for Flatcar AzureCNI, skips Ubuntu2004FIPS, adds HTTPS proxy + private DNS scenario, updates mutator signatures across scenarios. |
| e2e/scenario_win_test.go | Updates bootstrap mutator signatures for Windows scenarios. |
| e2e/scenario_localdns_hosts_test.go | Updates mutator signatures for LocalDNS hosts plugin scenarios. |
| e2e/scenario_gpu_managed_experience_test.go | Updates mutator signatures for GPU managed experience scenarios. |
| e2e/scenario_gpu_daemonset_test.go | Updates mutator signatures for GPU daemonset scenario. |
Comments suppressed due to low confidence (2)
e2e/kube.go:330
EnsureDebugDaemonsetsnow creates the proxy ConfigMap/DaemonSet for every non-network-isolated cluster. If the proxy is only needed for a subset of scenarios, consider decoupling it from the general "debug daemonsets" setup so failures in pulling/starting the proxy don’t break unrelated tests during cluster preparation.
return nil
})
if err != nil {
return err
}
return nil
}
func (k *Kubeclient) createKubernetesSecret(ctx context.Context, namespace, secretName, registryName, username, password string) error {
defer toolkit.LogStepCtxf(ctx, "creating kubernetes secret %s in namespace %s for registry %s", secretName, namespace, registryName)()
e2e/kube.go:606
- The proxy DaemonSet runs with
HostNetwork: trueand exposes a fixedHostPort(8888) while tolerating all taints. This effectively opens an unauthenticated forward proxy on every system-pool node IP, which is reachable from within the VNet and may be abused or interfere with other host processes that might already bind the port. Consider scoping this down (e.g., run on a single chosen node/instance, tighten tolerations/nodeSelectors, and/or add network-level restrictions) and document the intended threat model for this test-only proxy.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
e2e/kube.go:83
- PR description mentions tolerating transient FailedCreatePodSandBox events by aggregating event.Count and allowing retries (e.g., maxRetries=3), but WaitUntilPodRunning no longer has any sandbox-event handling and the prior retry-aware method was removed. Either implement the described sandbox-failure tolerance (including using event.Count) or adjust the PR description so reviewers/users don’t assume this mitigation exists.
func (k *Kubeclient) WaitUntilPodRunning(ctx context.Context, namespace string, labelSelector string, fieldSelector string) (*corev1.Pod, error) {
defer toolkit.LogStepCtxf(ctx, "waiting for pod %s %s in %q namespace", labelSelector, fieldSelector, namespace)()
var pod *corev1.Pod
err := wait.PollUntilContextTimeout(ctx, 3*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
pods, err := k.Typed.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
FieldSelector: fieldSelector,
Comment on lines
+295
to
+303
| for _, check := range checks { | ||
| var lastResult *podExecResult | ||
| err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| execResult, execErr := execOnUnprivilegedPod(ctx, s.Runtime.Cluster.Kube, nonHostPod.Namespace, nonHostPod.Name, check.cmd) | ||
| if execErr != nil { | ||
| s.T.Logf("wireserver check %q: exec error (retrying): %v", check.desc, execErr) | ||
| return false, nil | ||
| } | ||
| lastResult = execResult | ||
| if lastResult.exitCode == "28" { | ||
| return true, nil | ||
| } | ||
| s.T.Logf("wireserver check %q: expected exit code 28, got %s (retrying)", check.desc, lastResult.exitCode) | ||
| return false, nil | ||
| }) | ||
| if err != nil { | ||
| s.T.Logf("host IPTABLES: %s", execScriptOnVMForScenario(ctx, s, "sudo iptables -t filter -L FORWARD -v -n --line-numbers").String()) | ||
| if lastResult == nil { | ||
| require.NoErrorf(s.T, err, "curl to %s did not complete before polling stopped", check.desc) | ||
| } | ||
| s.T.Logf("last curl result for %s: %s", check.desc, lastResult.String()) | ||
| assert.Equal(s.T, "28", lastResult.exitCode, "curl to %s expected to fail with timeout, but it didn't after retries", check.desc) | ||
| s.T.FailNow() | ||
| execResult, execErr := execOnUnprivilegedPod(ctx, s.Runtime.Cluster.Kube, nonHostPod.Namespace, nonHostPod.Name, check.cmd) | ||
| require.NoError(s.T, execErr, "failed to exec wireserver check %q on debug pod", check.desc) | ||
| if execResult.exitCode != "28" { | ||
| iptablesFwd := execScriptOnVMForScenario(ctx, s, "sudo iptables -t filter -L FORWARD -v -n --line-numbers").String() | ||
| iptablesKubeFwd := execScriptOnVMForScenario(ctx, s, "sudo iptables -t filter -L KUBE-FORWARD -v -n --line-numbers 2>/dev/null || echo 'chain not found'").String() | ||
| iptablesSave := execScriptOnVMForScenario(ctx, s, "sudo iptables-save -t filter 2>/dev/null | head -80").String() | ||
| conntrack := execScriptOnVMForScenario(ctx, s, "sudo conntrack -L -d 168.63.129.16 2>/dev/null || echo 'conntrack not available'").String() | ||
| s.T.Fatalf("wireserver must not be reachable from pods: curl to %s returned exit code %s (expected 28/timeout)\n"+ |
Contributor
Author
There was a problem hiding this comment.
Every single request to wireserver is expected to fail. If doesn't fail ocasionally, then it highlights a potential issue that needs to be addressed
Comment on lines
269
to
273
| // validateWireServerBlocked checks that unprivileged pods cannot reach WireServer. | ||
| // The iptables FORWARD DROP rules blocking pod→WireServer traffic can be transiently | ||
| // absent when kube-proxy or CNI flush/recreate iptables chains during node setup. | ||
| // We resolve the debug pod once up front (outside the retry budget) so that pod | ||
| // scheduling latency doesn't eat into the iptables-check timeout. | ||
| // validateWireServerBlocked checks that unprivileged pods cannot reach WireServer. | ||
| // Wireserver must never be reachable from pods — any successful connection is a | ||
| // security issue, not a transient condition to retry through. | ||
| func validateWireServerBlocked(ctx context.Context, s *Scenario) { |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
e2e/validation.go:26
ValidatePodRunningWithRetrycurrently will not compile:for i := range maxRetriesattempts to range over anint. Use a standard counted loop (e.g.,for i := 0; i < maxRetries; i++) and ensure the function still performs at least one validation attempt whenmaxRetriesis 0 or 1 (depending on intended semantics).
func ValidatePodRunningWithRetry(ctx context.Context, s *Scenario, pod *corev1.Pod, maxRetries int) {
var err error
for i := range maxRetries {
err = validatePodRunning(ctx, s, pod)
if err != nil {
time.Sleep(1 * time.Second)
s.T.Logf("retrying pod %q validation (%d/%d)", pod.Name, i+1, maxRetries)
continue
e2e/kube.go:82
- PR description says
WaitUntilNodeReady()was updated to wait for removal of thenode.cloudprovider.kubernetes.io/uninitialized:NoScheduletaint before returning success, but the implementation still returns as soon asNodeReady=True(no taint check). Either add the taint-removal wait as described, or update the PR description to match the actual behavior.
func (k *Kubeclient) WaitUntilPodRunning(ctx context.Context, namespace string, labelSelector string, fieldSelector string) (*corev1.Pod, error) {
defer toolkit.LogStepCtxf(ctx, "waiting for pod %s %s in %q namespace", labelSelector, fieldSelector, namespace)()
var pod *corev1.Pod
err := wait.PollUntilContextTimeout(ctx, 3*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
pods, err := k.Typed.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
e2e/cse_timing.go:135
listCmdno longer referencescseEventsDir, but the file still declares thecseEventsDirconstant. That constant is now unused, which will causego test/go vetto fail for the e2e module. Either removecseEventsDiror reintroduce it into the command (without reintroducing duplicate file matches).
"sudo find /var/log/azure/Microsoft.Azure.Extensions.CustomScript/ -name '*.json' -path '*/events/*' -exec sh -c 'cat \"$1\"; echo' _ {} \\; 2>/dev/null",
)
result, err := execScriptOnVm(ctx, s, s.Runtime.VM, listCmd)
if err != nil {
return nil, fmt.Errorf("failed to read CSE events: %w", err)
}
e2e/kube.go:82
- PR description mentions raising the
installDepsCSE perf threshold inscenario_cse_perf_test.go, but that file is not part of the PR diff (only validation.go, kube.go, cse_timing.go are modified). Either include the intended threshold change in this PR or update the description to reflect the actual changes.
func (k *Kubeclient) WaitUntilPodRunning(ctx context.Context, namespace string, labelSelector string, fieldSelector string) (*corev1.Pod, error) {
defer toolkit.LogStepCtxf(ctx, "waiting for pod %s %s in %q namespace", labelSelector, fieldSelector, namespace)()
var pod *corev1.Pod
err := wait.PollUntilContextTimeout(ctx, 3*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
pods, err := k.Typed.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
Comment on lines
+295
to
+300
| execResult, execErr := execOnUnprivilegedPod(ctx, s.Runtime.Cluster.Kube, nonHostPod.Namespace, nonHostPod.Name, check.cmd) | ||
| require.NoError(s.T, execErr, "failed to exec wireserver check %q on debug pod", check.desc) | ||
| if execResult.exitCode == "0" { | ||
| iptablesFwd := execScriptOnVMForScenario(ctx, s, "sudo iptables -t filter -L FORWARD -v -n --line-numbers").String() | ||
| iptablesKubeFwd := execScriptOnVMForScenario(ctx, s, "sudo iptables -t filter -L KUBE-FORWARD -v -n --line-numbers 2>/dev/null || echo 'chain not found'").String() | ||
| iptablesSave := execScriptOnVMForScenario(ctx, s, "sudo iptables-save -t filter 2>/dev/null | head -80").String() |
Contributor
Author
There was a problem hiding this comment.
Good catch — addressed in 4f42248.
Now whitelists only 28 (FORWARD DROP timeout) and 7 (FORWARD REJECT — connection refused). Anything else (0 = reachable, 127 = curl missing, 2/3 = bad args, 6 = DNS, etc.) fails the test with the actual exit code logged plus full iptables/conntrack diagnostics.
This is strictly more defensive than the original exit == 28 check (now also accepts REJECT-style blocks) while closing the silent-bypass holes you flagged. command -v curl pre-check is unnecessary because exit 127 now fails explicitly with "unexpected curl exit code 127" instead of being lost.
r2k1
commented
May 11, 2026
Contributor
Author
r2k1
left a comment
r2k1
left a comment
There was a problem hiding this comment.
Addressed review comment:
validation.go:300 — curl exit code check: Fixed. Now only fails on exit code 0 (successful connection), not on any non-28 code. Exit codes like 7 (connection refused), 28 (timeout), or 127 (curl missing) all correctly indicate wireserver is not reachable.
Collaborator
|
not sure bout 3) I fear this will cause more unstable issue, I agree we need to better understand, but I did work on this for 3 days and the RCA was never found. |
djsly
requested changes
May 12, 2026
Collaborator
djsly
left a comment
djsly
left a comment
There was a problem hiding this comment.
please decouple the wireserver in another PR to have it run multiple run to help diagnose the failures this will reintroduce
| // We resolve the debug pod once up front (outside the retry budget) so that pod | ||
| // scheduling latency doesn't eat into the iptables-check timeout. | ||
| // Wireserver must never be reachable from pods — any successful connection is a | ||
| // security issue, not a transient condition to retry through. |
Collaborator
There was a problem hiding this comment.
dont commit and ensure stability before removing the retry. or else this will keep generating issues, this test run with every e2e
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Fixes two sources of false-positive failures observed across recent E2E builds on main. ### 1. Remove FailedCreatePodSandBox early exit (`kube.go`) The previous code listed pod events every 3s and killed the test on the first FailedCreatePodSandBox event. But kubelet retries sandbox creation automatically — transient errors like "loopback interrupted system call" resolve on their own. The early exit was killing tests before kubelet could recover. Removed the event check entirely. The 5-minute poll timeout already handles persistent failures, and CrashLoopBackOff still fails fast. Also removed the now-unused `WaitUntilPodRunningWithRetry` wrapper and its `maxRetries` parameter. ### 2. Fix duplicate CSE event extraction (`cse_timing.go`) The `find` command searched two overlapping paths: `cseEventsDir` (`.../events/`) AND its parent directory with `-path '*/events/*'`. The same files matched twice → duplicate tasks → spurious `Task_installDeps#01` subtests (observed in 11/46 recent builds). Fixed by using a single starting path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| Tasks []CSETaskTiming | ||
| Provision *CSEProvisionTiming | ||
| taskIndex map[string]*CSETaskTiming | ||
| Tasks []CSETaskTiming |
Contributor
There was a problem hiding this comment.
Not related to this PR, but we probably should have some formatting or lint rule to stop whitespace only changes in PRs.
timmy-wright
approved these changes
May 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Fixes two sources of E2E test false-positive failures found by analyzing recent builds on
main.1. Remove FailedCreatePodSandBox early exit (
kube.go)The old code listed pod events every 3s and killed the test on the first
FailedCreatePodSandBoxevent. But kubelet retries sandbox creation automatically — transient errors like "loopback interrupted system call" resolve on their own. The early exit was killing tests before kubelet could recover.Removed the event check entirely. The 5-minute poll timeout already handles persistent failures.
CrashLoopBackOffstill fails fast. Also removed the unusedWaitUntilPodRunningWithRetrywrapper andmaxRetriesparameter.2. Fix duplicate CSE event extraction (
cse_timing.go)The
findcommand searched two overlapping paths:cseEventsDir(.../events/) AND its parent directory with-path '*/events/*'. Same files matched twice → duplicate tasks → spuriousTask_installDeps#01subtests (observed in 11/46 builds).Fixed by using a single starting path.
The strict wireserver validation change that was previously bundled here has been extracted into #8580 for separate review.
Which issue(s) this PR fixes:
N/A — test-only hardening, no linked issue.