fix(e2e): reduce E2E test flakiness#8480
Open
r2k1 wants to merge 4 commits into
Open
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,
| // 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. |
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.
Revert a1bebdc (feat(e2e): add HTTP_PROXY + private DNS test scenario) which had issues on the e2e-flakiness-fixes branch. Analysis of 55 E2E builds on main (3 weeks) showed 84% failure rate. Root causes identified and fixed: 1. Node readiness race (kube.go): WaitUntilNodeReady() returned success on NodeReady=True even when node still had the cloud-provider uninitialized taint, preventing test pod scheduling. Now waits for taint removal before declaring node ready. 2. IPtables false positives (validation.go): iptables eBPF-host-routing validator rejected a normal host DHCP INPUT rule (UDP/68) not in its allowlist. Added to allowlist. 3. CSE timing threshold (scenario_cse_perf_test.go): installDeps 90s threshold was set with 'no direct prod data' and consistently exceeded by the network-heavy apt workflow. Raised to 120s. 4. Duplicate CSE events (cse_timing.go): events appearing in both GA events directory and handler subdirectories created spurious Task_installDeps#01 subtests. Added deduplication. 5. Broken Ubuntu2004FIPS lane (scenario_test.go): Test added on 2026-04-22 without VMSS FIPS capability setup, never green. Skipped until properly fixed. Dropped from earlier version: Flatcar AzureCNI networkPlugin removal. Rubber duck review found removing networkPlugin=azure defaults to kubenet (not none), which would break tests differently. Proper fix requires PR #7463 (set to none instead). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
…retry
Address reviewer feedback that the previous `exit != 0` check silently
accepted exit 127 (curl missing), 2 (bad args), 6 (DNS), etc., which
could disable this security validation without surfacing the cause.
- Whitelist exit codes 28 (FORWARD DROP timeout) and 7 (FORWARD REJECT
connection refused) as the only valid 'wireserver blocked' signals.
- Anything else now fails loudly with full iptables/conntrack diagnostics
and the actual exit code in the error message.
- Retry the curl ONLY on exec-layer flakes (transient kube-apiserver
hiccups), never on the result itself — a single observed unexpected
exit code is enough to fail the security check.
This is strictly more defensive than the original ('exit == 28' only)
because it now accepts REJECT-based blocks as well, while continuing
to fail on every other class of regression.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
124
to
130
| // Read all event JSON files from the CSE events directory, explicitly | ||
| // appending a newline after each file so each JSON document is separated. | ||
| // Search both the primary events directory and any handler-version subdirectories, | ||
| // as the Guest Agent may move events between these locations. | ||
| // Search the CustomScript directory tree for any events/ subdirectories, | ||
| // as the Guest Agent may store events in handler-version subdirectories. | ||
| listCmd := fmt.Sprintf( | ||
| "sudo find %s /var/log/azure/Microsoft.Azure.Extensions.CustomScript/ -name '*.json' -path '*/events/*' -exec sh -c 'cat \"$1\"; echo' _ {} \\; 2>/dev/null", | ||
| cseEventsDir, | ||
| "sudo find /var/log/azure/Microsoft.Azure.Extensions.CustomScript/ -name '*.json' -path '*/events/*' -exec sh -c 'cat \"$1\"; echo' _ {} \\; 2>/dev/null", | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Contributor
There was a problem hiding this comment.
Applied in 91203e3. listCmd now uses a plain string literal instead of fmt.Sprintf(...) since there are no format args.
Agent-Logs-Url: https://github.com/Azure/AgentBaker/sessions/eda1011a-68fd-49a2-9d8b-fd6a93d727f8 Co-authored-by: r2k1 <2599261+r2k1@users.noreply.github.com>
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:
|
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.
Summary
Fixes several E2E test issues found by analyzing 55 builds on
mainover 3 weeks.Changes
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 handles persistent failures.
CrashLoopBackOffstill fails fast.Also removed the unused
WaitUntilPodRunningWithRetrywrapper 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.
3. Strict wireserver validation (
validation.go)Previously retried for 1 minute, passing if curl eventually timed out. But any successful curl means wireserver was reachable from pods — a security issue, not a transient condition to retry through.
Changed to single curl per check, fail immediately if reachable. Added diagnostics on failure: FORWARD chain, KUBE-FORWARD chain, iptables-save, conntrack entries.