OCPBUGS-62930: Wait for egressfirewall rules to be applied successfully before sending traffic #30433
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@arkadeepsen: This pull request references Jira Issue OCPBUGS-62930, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
requested review from
anuragthehatter,
pliurh and
pperiyasamy
|
/retest-required |
|
/test ? |
|
@arkadeepsen: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test e2e-gcp-ovn-techpreview |
|
/retest-required |
| o.Expect(err).NotTo(o.HaveOccurred(), "created egress-firewall object") | ||
|
|
||
| g.By("waiting for egressfirewall rules to be applied successfully") | ||
| egfwName, err := oc.AsAdmin().Run("get").Args("egressfirewall", "-o", "jsonpath={.items[0].metadata.name}").Output() |
There was a problem hiding this comment.
shouldn't egfwName refers to egress firewall name created from manifest ? this command just retrieves first egFw object from the namespace, right ?
or is it going to be only one egFw object in the namespace which is created from manifest file ? may be adding a comment here would clarify it.
There was a problem hiding this comment.
There can be only one egress firewall object in a namespace: https://ovn-kubernetes.io/features/network-security-controls/egress-firewall/#introduction
I'll add a comment to clarify this.
There was a problem hiding this comment.
Added a comment for the clarification.
arkadeepsen
force-pushed
the
egressfirewall-race
branch
from
d427137 to
cc6479d
Compare
arkadeepsen
changed the title
|
/lgtm Thanks @arkadeepsen ! |
|
/retest-required |
1 similar comment
|
/retest-required |
|
|
||
| err = wait.PollUntilContextTimeout(context.TODO(), 100*time.Millisecond, 1*time.Second, true, func(ctx context.Context) (bool, error) { | ||
| out, err := oc.AsAdmin().Run("get").Args("egressfirewall", egfwName, "-o", "jsonpath={.status.status}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get egressfirewall object") |
There was a problem hiding this comment.
I think we want to keep retrying on failure until success or timeout. So we'd better just return false here.
There was a problem hiding this comment.
Thanks for pointing that out. I have made the changes.
arkadeepsen
force-pushed
the
egressfirewall-race
branch
from
cc6479d to
470da32
Compare
arkadeepsen
force-pushed
the
egressfirewall-race
branch
from
470da32 to
04ab760
Compare
| egfwName, err := oc.AsAdmin().Run("get").Args("egressfirewall", "-o", "jsonpath={.items[0].metadata.name}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to get egressfirewall object") | ||
|
|
||
| err = wait.PollUntilContextTimeout(context.TODO(), 100*time.Millisecond, 1*time.Second, true, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
The 1-second timeout is pretty short. Are you sure it's enough to wait for an EFW to be ready?
There was a problem hiding this comment.
I added the 1-second timeout as this scenario was flaking very rarely and when it was not, it worked fine without the wait. I didn't want to add too much delay to the test.
But now on having a second look, the flake happens on the ping to 1.1.1.1 which is performed after a 1 second ping to 8.8.8.8. So we need to wait at least more than that. Do you think a 3 second timeout would be fine or should we go with 5 second timeout?
There was a problem hiding this comment.
I would give a 1s interval and 30s timeout to reduce flakiness.
There was a problem hiding this comment.
Updated the interval to 1s and timeout to 30s.
arkadeepsen
force-pushed
the
egressfirewall-race
branch
from
04ab760 to
0e925c9
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, pliurh, pperiyasamy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required |
1 similar comment
|
/retest-required |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 0e925c9
New tests seen in this PR at sha: 0e925c9
|
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
@arkadeepsen: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
This PR adds a wait for checking if the egressfirewall rules are applied successully before sending egress traffic. This will avoid any racy behavior where the traffic is sent out before the rules are applied on all the nodes successfully.