fix(test): Fix flaky VPC e2e tests for SG rule deletion#331
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sapphirew The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| assert len(resource["spec"]["tags"]) == 1 | ||
| assert resource["spec"]["tags"][0]["key"] == "updatedtagkey" | ||
| assert resource["spec"]["tags"][0]["value"] == "updatedtagvalue" | ||
| user_spec_tags = [t for t in resource["spec"]["tags"] if not t["key"].startswith("services.k8s.aws/")] |
There was a problem hiding this comment.
Can we update this to use shared util functions for asserting without ack tags? Heres an example from emr-serverless
updated_tags = {"environment": "production", "team": "data-platform"}
cr = k8s.get_resource(ref)
cr["spec"]["tags"] = updated_tags
k8s.replace_custom_resource(ref, cr)
time.sleep(MODIFY_WAIT_AFTER_SECONDS)
assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=10)
response = emrserverless_client.list_tags_for_resource(resourceArn=application_arn)
latest_tags = response["tags"]
tags.assert_ack_system_tags(tags=latest_tags)
tags.assert_equal_without_ack_tags(expected=updated_tags, actual=latest_tags)
There was a problem hiding this comment.
Good call, updated to use the shared lib for tags assertion
There was a problem hiding this comment.
Looks like the tests already make tags assertion above. Going to drop this change until have a clear root cause.
| assert resource["spec"]["tags"][0]["key"] == "initialtagkey" | ||
| assert resource["spec"]["tags"][0]["value"] == "initialtagvalue" | ||
| # Only user tags should be present in Spec (filter out ACK system tags) | ||
| user_spec_tags = [t for t in resource["spec"]["tags"] if not t["key"].startswith("services.k8s.aws/")] |
There was a problem hiding this comment.
Same thing here, use shared utility functions for asserting without ack tags
| ) | ||
|
|
||
| # Check user tags are removed from Spec | ||
| # Check user tags are removed from Spec (filter out ACK system tags) |
There was a problem hiding this comment.
Same thing here, use shared utility functions for asserting without ack tags
|
@sapphirew I think this is actually a bug in the ec2-controller. The system tags should be filtered before patching the spec.tags field. |
sapphirew
force-pushed
the
fix/vpc-e2e-test-flakiness
branch
from
06122f9 to
431767b
Compare
…oup_rule
Replace time.sleep(10) with wait_on_condition("ACK.ResourceSynced",
"True") so the test waits for the controller to signal reconciliation
is complete rather than relying on a fixed delay that may be
insufficient under CI load.
sapphirew
force-pushed
the
fix/vpc-e2e-test-flakiness
branch
from
431767b to
daf90bb
Compare
sapphirew
changed the title
@knottnt I couldn't root cause the bug for tags in the ec2-controller yet. Going to reduce the scope of the fix to |
|
/retest |
|
@sapphirew |
ack-prow
Bot
added
the
do-not-merge/hold
|
This PR should fix system tags issues: aws-controllers-k8s/code-generator#702 |
| # Wait for the controller to finish reconciling (SG rule deletion | ||
| # happens inline during creation but the controller may be busy) |
There was a problem hiding this comment.
nit: I don't think this is true anymore after you previous PR.
| # Wait for the controller to finish reconciling (SG rule deletion | |
| # happens inline during creation but the controller may be busy) | |
| # Wait for the controller to finish reconciling (SG rule deletion | |
| # happens in update path after initial creation) |
4 participants
Issue #, if available:
Description of changes:
The test_crud_tags test failed because the ACK runtime's EnsureTags persist ACK system tags (services.k8s.aws/controller-version, services.k8s.aws/namespace) into spec.tags during reconciliation. Filter out tags with the
services.k8s.aws/prefix before asserting user tag counts and values.The
test_disallow_default_security_group_ruletest failed because the controller was too busy processing other resources to complete the reconcile within the fixed 10-second sleep. Replacetime.sleepwithwait_on_condition("ACK.ResourceSynced", "True")which polls until the controller signals reconciliation is complete.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.