-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix egress gateway pod cleanup for remote zone pods. #4744
base: master
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
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