test(listener): add ignore-field-drift e2e coverage for weight drift - #90
Conversation
Add end-to-end coverage for the services.k8s.aws/ignore-field-drift annotation (aws-controllers-k8s/runtime#256) on an ELBv2 Listener's forward-action target-group weights, mirroring "Scenario 2" of aws-controllers-k8s#85: a blue/green deploy tool shifts traffic weights across target groups on the live listener, and without ignore-field-drift the controller reconciles the weights back to the declared spec, breaking the deployment. TestListenerIgnoreFieldDrift.test_weight_drift_ignored declares a weighted forward action (90/10) annotated to ignore spec.defaultActions, shifts the weights out-of-band to 50/50 via ModifyListener, and asserts that the external distribution survives, the resource stays Synced, and a subsequent spec edit is retained in the CR but not pushed to AWS. The IgnoreFieldDrift feature gate is Alpha and disabled by default, so the test enables it on the deployed controller for the module and restores the prior value afterwards (mirrors the ec2-controller VPC coverage for the same runtime feature). The gate is only present once the controller's runtime dependency includes runtime#256, so this coverage runs green after the corresponding runtime bump. Two ip-type target groups back the weighted action so the fixture does not depend on registered targets. Signed-off-by: Hao Wang <rhaowang@amazon.com>
The suite runs 16 pytest-xdist workers with LoadScheduling, which spreads
individual tests across worker PROCESSES. A fixture is therefore created
once in every worker that picks up a test from this file, at any scope, so
the module-scoped enable/restore pair rolls the shared ack-elbv2-controller
Deployment twice per such worker.
Each rollout stops reconciliation cluster-wide for a few seconds, which is
enough to fail an unrelated worker mid-assertion -- in ec2-controller the
same pattern lost three tests to KeyError on a status field that was never
populated and to an ACK.ResourceSynced wait that expired. elbv2 has fewer
drift tests so fewer rollouts, but the listener, load balancer and target
group tests running in parallel are exposed the same way.
Turn the gate on once and leave it on:
- Check-then-set, so a worker that finds it already on skips the patch.
Concurrent workers observing it off compute the same FEATURE_GATES value
from the same starting point, so the second patch leaves the pod template
identical, does not bump the Deployment generation and does not roll a
second time. No cross-process lock needed.
- No restore, which would cost a second rollout and could disable the gate
underneath a drift test still running in another worker. Safe to leave
on: the gate is inert unless a resource carries the ignore-field-drift
annotation, and only this file's drift resources do.
Session scope also makes the ordering explicit -- the gate is now guaranteed
to be on before the module-scoped listener fixture creates its resources,
rather than relying on fixture argument order.
Still one rollout per run. Removing it entirely means setting the gate at
controller setup time (FEATURE_GATES in test-infra's controller-setup.sh,
alongside the existing IAMRoleSelector case), after which this fixture
degrades to a no-op check.
Ports ec2-controller b2cb92a to elbv2.
Signed-off-by: Hao Wang <rhaowang@amazon.com>
1cf24d1 to
c21c503
Compare
1. The "drift survives" and "stays Synced" assertions cannot fail — verified
Issue. The out-of-band Proven two independent ways. First, a control listener identical to the fixture's except for carrying no
Second, the controller's log for the annotated listener contains exactly two entries Nothing between them. The controller never examined the resource during the window in The Synced assertion fails the same way, and worse: Fix. Capture |
2. Fixtures read
|
Addresses both review findings from @gustavodiaz7722. 1. The drift-survives and stays-Synced assertions could not fail. An out-of-band ModifyListener produces no watch event, and this controller's resync period is the runtime default of 10 hours: config/controller/deployment.yaml (what controller-setup.sh deploys via kustomize) passes no --reconcile-default-resync-seconds or --reconcile-resource-resync-seconds, and listener's RequeueOnSuccessSeconds() returns 0, so getResyncPeriod falls through to defaultResyncPeriod. No reconcile occurred during MODIFY_WAIT_AFTER_SECONDS, so "the weights are still 50/50" observed that nobody had looked, not that the controller declined to revert -- it held identically with the feature absent. The Synced check was worse: wait_on_condition returned on its first poll off the ACK.ResourceSynced condition written back at create time. Now the test captures condition.get_synced_last_transition_time before the shift, forces a reconcile by patching an inert annotation, and requires via wait_on_condition_after that a reconcile which started after the drift completed with ACK.ResourceSynced=True. That single gate carries both original claims, so the separate (vacuous) Synced wait is gone. Only past it is the weight assertion meaningful. An annotation is enough to trigger the reconcile because the runtime adds AnnotationChangedPredicate to the event filter whenever the IgnoreFieldDrift gate is on; the default filter is GenerationChangedPredicate alone. Keeping the probe off the spec means the only delta the reconcile sees is the external drift itself. A precondition assert also confirms AWS reports the shifted weights before the reconcile is forced, so a race with ModifyListener cannot make the test pass for the wrong reason. The same treatment is applied to the final spec-edit step. That patch does bump generation, so a reconcile was guaranteed to be queued -- but not to have finished within a fixed sleep. 2. The fixtures read status.ackResourceMetadata.arn before it existed. wait_resource_consumed_by_controller returns as soon as the resource has any .status, which is the first status write and predates the ARN. Neither two_target_groups nor ignore_field_drift_listener waited on ACK.ResourceSynced, and the listener resolves two target group references created 10s earlier, so the ARN read could KeyError. The listener fixture compounded it by yielding the CR snapshot taken at that moment, leaving the test no way to recover. Both fixtures now wait for ACK.ResourceSynced, and the listener fixture re-reads and asserts the ARN is present before yielding. Signed-off-by: Hao Wang <rhaowang@amazon.com>
|
Both findings confirmed and fixed in acb154e. I verified each independently against the code rather than taking the reproductions at face value; the chains hold. 1. Vacuous drift / Synced assertionsConfirmed end to end. One refinement: the test's window is ~30s after create, not the ~hour in your experiment, so a post-create status-write reconcile could occasionally land inside it. That makes the assertion unreliable rather than strictly unfailable — which argues for the same fix, since the test never verified a reconcile had happened either way. The fix follows your first option. The test now captures Two details worth recording:
I applied the same treatment to the final spec-edit step. That patch does bump 2. ARN read before it existsConfirmed. Both fixtures now wait for Evidence the fix is not just greenThe controller log for the passing run now contains two suppression events where the old test produced one: The |
|
/LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gustavodiaz7722, sapphirew The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
Adds end-to-end coverage for the
services.k8s.aws/ignore-field-driftannotation (aws-controllers-k8s/runtime#256) on an ELBv2Listener's forward-action target-group weights.This mirrors "Scenario 2" of #85: a blue/green deploy tool shifts traffic weights across target groups on the live listener, and without ignore-field-drift the controller reconciles the weights back to the declared spec, breaking the deployment. The generic runtime feature lets the resource opt
spec.defaultActionsout of drift reconciliation via the annotation — no controller-specific code required.Test-only change:
test/e2e/tests/test_listener.py,test/e2e/resources/listener_ignore_field_drift.yaml,test/e2e/resources/target_group_ip.yaml.What the test does
TestListenerIgnoreFieldDrift.test_weight_drift_ignored:Listenerwith a weighted forward action across two target groups (declared 90/10), annotatedservices.k8s.aws/ignore-field-drift: "spec.defaultActions".Synced.ModifyListener(simulating the deploy tool).Synced, and a subsequent spec edit (→70/30) is retained in the CR but not pushed to AWS.Two ip-type target groups back the weighted action so the fixture does not depend on registered targets.
Feature gate handling
The
IgnoreFieldDriftgate is Alpha and disabled by default, so the test enables it on the deployed controller by patchingFEATURE_GATESon the Deployment. The gate landed in runtime v0.62.0;mainis now on v0.63.0, so it is available.The
ignore_field_drift_enabledfixture is session-scoped, check-then-set, and never restores — this matters and is not just style. pytest-xdist spreads individual tests across 16 worker processes underLoadScheduling, so a fixture at any scope is instantiated once in every worker that picks up a test from this file. An enable/restore pair therefore rolls the shared controller Deployment twice per such worker, and each restart stops reconciliation cluster-wide long enough to fail an unrelated listener / load balancer / target group test waiting onACK.ResourceSynced. This is the same fix as ec2-controller#361b2cb92a, where the module-scoped version cost three unrelated tests in one run.Check-then-set needs no cross-process lock: concurrent workers that both observe the gate off compute the same
FEATURE_GATESstring from the same starting value, so the second patch leaves the pod template byte-identical and does not bump the Deployment generation. Leaving the gate on is safe — it is inert unless a resource carries the annotation, and only this file's drift resources do; the kind cluster is torn down at the end of the run.Follow-up worth doing: set the gate at controller setup time (
FEATURE_GATESin test-infra'scontroller-setup.sh, asIAMRoleSelectoralready does), after which this fixture degrades to a no-op check and the mid-run rollout disappears entirely.Testing
Verified locally via a full
kinde2e run with the gate enabled:1 passed. The external 50/50 shift survived, the resource stayedSynced, and the 70/30 spec edit was retained but not pushed.Note for anyone hitting this on an older base: against a runtime without the gate, the controller treats the unknown gate as fatal and exits, which surfaces only as
controller deployment ack-elbv2-controller did not roll out within 120s. The crashlooping pod's logs are not in the prowcontroller_logsartifact (it captures the surviving pod), so that artifact looks misleadingly clean.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.