Fix egress gateway pod cleanup for remote zone pods. #4744
Conversation
github-actions
bot
added
area/e2e-testing
feature/egress-gateway
| @@ -802,7 +802,7 @@ var _ = ginkgo.Describe("External Gateway", func() { | |||
| ginkgo.Entry("IPV6 udp", &addressesv6, "udp"), | |||
| ginkgo.Entry("IPV6 tcp", &addressesv6, "tcp")) | |||
|
|
|||
| ginkgo.DescribeTable("ExternalGWPod annotation: Should validate conntrack entry deletion for TCP/UDP traffic via multiple external gateways a.k.a ECMP routes", func(addresses *gatewayTestIPs, protocol string) { | |||
| ginkgo.DescribeTable("ExternalGWPod annotation: Should validate conntrack entry deletion for TCP/UDP traffic via multiple external gateways a.k.a ECMP routes", func(addresses *gatewayTestIPs, protocol string, deletePod bool) { | |||
There was a problem hiding this comment.
Wouldn't it be better to split this into its own table/use case? one for annotation deletion and another one for pod deletion. The flow is mostly the same, but the breakdown can help to write a more specific description of what the test does in case the test fails (it failed while deleting annotation or it failed while deleting the pod). WDYT?
There was a problem hiding this comment.
You can distinguish the tests even now:
- test names will be different
- ginkgo.By will log different steps
I also find it much easier to understand the difference between test sequences when it shows which step exactly differs, e.g. vs having a full copy of this test with 3 different lines
There was a problem hiding this comment.
Honestly it's a personal preference between readability vs reusability. I tend to prefer readability in tests even if that means replicating code that can't be extracted as functions (perhaps in this case it could be, but that's another story).
On the other hand, true or false is not clear what they mean as arguments without understanding the logic inside the test. I'd recommend to use constants whose names define their behavior rather than booleans as arguments for what they mean, e.g. BY_DELETE_ANNOTATION, BY_DELETE_GW_POD.
It's a cosmetic comment since this logic will go away, eventually I hope 😄 .
re-labeling gateway pod. It uses different handlers for update and delete pod internally. Remove external.gateway from the dual-stack exclusion, as it is supported for ipv6. Signed-off-by: Nadia Pinaeva <[email protected]>
For local zone pods, deleteLogicalPort cleans this up, but before IC this function was called for all non-host-network pods, hence this logic. After IC, deleteLogicalPort won't be called for all remote zone pods, so condition is not needed. Signed-off-by: Nadia Pinaeva <[email protected]>
npinaeva
force-pushed
the
egressgw-deletepod-
branch
from
36f688e
to
839842b
Compare
|
@npinaeva shouldn't the title be updated to include the bug reference OCPBUGS-38753? |
Cleanup gateway pod for remote zone.
For local zone pods, deleteLogicalPort cleans this up, but before IC
this function was called for all non-host-network pods, hence this
logic. After IC, deleteLogicalPort won't be called for all remote zone
pods, so condition is not needed.
See test result without the fix here #4743