OCPBUGS-35342: Add e2e test for connect timeout#1084
Conversation
|
@alebedev87: This pull request references Jira Issue OCPBUGS-28673, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
591119d to
1a836aa
Compare
|
/test e2e-azure-operator Different test failed: |
|
@alebedev87: This pull request references Jira Issue OCPBUGS-28673. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn 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. |
|
@alebedev87: This pull request references Jira Issue OCPBUGS-28673, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
Accidentally clicked on "Close with comment". |
|
@alebedev87: This pull request references Jira Issue OCPBUGS-35342, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
/jira refresh |
|
@alebedev87: This pull request references Jira Issue OCPBUGS-35342, 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: DetailsIn 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. |
|
/test e2e-azure-operator |
|
/assign @gcs278 |
gcs278
left a comment
There was a problem hiding this comment.
Looks good to me, pretty much all of my comments are just nit picks. Feel free to take or leave them.
| ic.Spec.TuningOptions = operatorv1.IngressControllerTuningOptions{ | ||
| ConnectTimeout: &metav1.Duration{Duration: 2 * time.Second}, | ||
| } | ||
| if err := kclient.Create(context.TODO(), ic); err != nil { |
There was a problem hiding this comment.
nit Super minor nit pick, but I think I've been told to use context.Background() instead of context.TODO() in the past. Though they do the same thing, TODO means that we will try to change it later, where Background indicates we aren't planning to change it.
| if err := kclient.Create(context.TODO(), ic); err != nil { | ||
| t.Fatalf("failed to create ingresscontroller: %v", err) | ||
| } | ||
| defer assertIngressControllerDeleted(t, kclient, ic) |
There was a problem hiding this comment.
nit I think we are trying to transition new E2E tests use t.Cleanup as the new standard.
| // getIngressOperatorDeploymentImage returns the image of the cluster ingress operator. | ||
| // This image can be used to spawn test servers implemented in the operator. | ||
| func getIngressOperatorDeploymentImage(t *testing.T, client client.Client, timeout time.Duration) (string, error) { | ||
| deployment, err := getDeployment(t, client, types.NamespacedName{Namespace: controller.DefaultOperatorNamespace, Name: "ingress-operator"}, timeout) |
There was a problem hiding this comment.
nit I don't believe it actually matters, but just as a completeness item: consider adding t.Helper(), as any will help log the correct line numbers for future t.Error or t.Fatal that may get added to this function here, or in downstream functions it calls.
| } | ||
|
|
||
| // getIptablesImage returns the image with the iptables tool installed in it. | ||
| func getIptablesImage(t *testing.T, client client.Client, timeout time.Duration) (string, error) { |
There was a problem hiding this comment.
nit I don't believe it actually matters, but just as a completeness item: consider adding t.Helper(), as any will help log the correct line numbers for future t.Error or t.Fatal that may get added to this function here, or in downstream functions it calls.
| defer assertIngressControllerDeleted(t, kclient, ic) | ||
|
|
||
| operatorImage, err := getIngressOperatorDeploymentImage(t, kclient, 1*time.Minute) | ||
| if err != nil { |
There was a problem hiding this comment.
should this also check for an operatorImage of ""? It won't throw an error, and the test will keep going, possibly giving a confusing error message later.
There was a problem hiding this comment.
getIngressOperatorDeploymentImage now errors if the image was not found.
| return false, nil | ||
| } | ||
| defer response.Body.Close() | ||
| // sC The "timeout connect" stroke before a connection to the server could |
There was a problem hiding this comment.
Typo? I also don't really understand this sentence.
There was a problem hiding this comment.
It's a copy paste from the reference link below. sC is the termination status code which we see in the haproxy access logs when the connection timeout kicks in. Here I tried to explain there reason I check for the codes 503, 504.
Let me rephrase it to be clearer.
| elbHostname := wildcardRecord.Spec.Targets[0] | ||
|
|
||
| // Wait until we can resolve the ELB's hostname | ||
| if err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) { |
There was a problem hiding this comment.
nit should we use non-deprecated function in new code?
| } | ||
|
|
||
| // Create a pod with an HTTP application that delays the connection and sends echo responses. | ||
| httpdPod := buildDelayConnectHTTPPod("connect-timeout-http", operatorcontroller.DefaultOperandNamespace, iptablesImage, operatorImage) |
There was a problem hiding this comment.
So I was scratching my head a bit. I forgot to update the updated Ingress Operator with the code here so it gets the server-delay-connect command, so the pod never starts and just crash loops.
But I had a successful run?
operator_test.go:3247: got expected response code after unexpected elapsed time 219.607859ms
operator_test.go:3247: got expected response code after unexpected elapsed time 200.351503ms
operator_test.go:3251: got expected response code after elapsed time 8.201132105s
operator_test.go:3256: deleted ingresscontroller test-connect-timeout
--- PASS: TestConnectTimeout (268.35s)
Must have been a freak false positive. Either way, how about ensuring the connect-timeout-http pod comes up and gets into the running state? At least, that will be helpful to debugging future E2E failure.
There was a problem hiding this comment.
That's no good. I tried to protect from the false positives by adding a range check for the elapsed time. Let me try to reproduce this and find a way to avoid.
There was a problem hiding this comment.
waitForPodReady function added.
| Command: []string{ | ||
| "/bin/sh", | ||
| "-c", | ||
| "iptables -I INPUT -p tcp --dport 8080 -m conntrack --ctstate NEW -j NFQUEUE --queue-num 100", |
There was a problem hiding this comment.
Could you add a comment here explaining why this is necessary?
I think it's creating a NFQUEUE for the serve-delay-connect-test-server command to work correctly no?
| defaultQueueNum = 100 | ||
| ) | ||
|
|
||
| func NewServeDelayConnectCommand() *cobra.Command { |
There was a problem hiding this comment.
I ran:
$ sudo ./ingress-operator serve-delay-connect-test-server
2024/06/12 16:32:46 Callback is registered with delay 40s
2024/06/12 16:32:46 Serving on 8080
Then I ran:
$ curl localhost:8080
HTTP/1.1
It didn't delay. Why is that? Do I need to run iptables commands for this function to work?
There was a problem hiding this comment.
Yes. A rule needs to send new tcp connections on 8080 port to the queue number used by the command (100 by default). Let me add the exact command as a comment.
There was a problem hiding this comment.
Hmm, I ran it again, this time running:
$ sudo iptables -I INPUT -p tcp --dport 8080 -m conntrack --ctstate NEW -j NFQUEUE --queue-num 100
$ sudo ./ingress-operator serve-delay-connect-test-server
[sudo] password for gspence:
2024/06/20 19:14:54 Callback is registered with delay 40s
2024/06/20 19:14:54 Serving on 8080
2024/06/20 19:14:56 Callback is triggered
2024/06/20 19:14:56 Delaying SYN packet for 40s
I curled in another terminal:
$ curl localhost:8080
HTTP/1.1
But immediately got an answer back. Seems like progress since I get log output for the serve-delay-connect-test-server command, but why would curl return immediately and not delay?
I suppose it could be some configuration with my laptop, because I did test with the route created by your E2E test, and it delays successfully. So I believe it works, but I was just curious why it wouldn't work on my local laptop.
EDIT: I figured it out. I need to use curl 127.0.0.1:8080, then it works. I wiresharked it and looks like curl switched to ipv6 with localhost, which didn't work correctly. Not sure if it's worth a comment somewhere that IPv6 doesn't work correctly.
There was a problem hiding this comment.
Right, it must be laptop specific because it works on my old RHEL. The golang's server is supposed to bind on ipv6 and ipv4 addresses for localhost, so the problem should be in the iptables. Would this work for you if the iptables command is run for ipv6?
sudo ip6tables -I INPUT -p tcp --dport 8080 -m conntrack --ctstate NEW -j NFQUEUE --queue-num 100
There was a problem hiding this comment.
Ah yea that worked. Whoops, I forgot that iptables is ipv4 ipv6 specific.
| sigs := make(chan os.Signal, 1) | ||
| signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Hmm, I can't actually ctrl-c out of this program when I run it on the command line:
sudo ./ingress-operator serve-delay-connect-test-server
[sudo] password for gspence:
2024/06/12 16:32:46 Callback is registered with delay 40s
2024/06/12 16:32:46 Serving on 8080
^[[1;5A^C2024/06/12 17:34:30 Received signal: interrupt
2024/06/12 17:34:30 Error reading from netlink socket: netlink receive: i/o timeout
^C^C^C^C
^C^C^C^C
I think it's not actually stopping the program. Does this do the same for you?
There was a problem hiding this comment.
Good catch! The problem was in the unnecessary signal handling, removed it.
|
@alebedev87: This pull request references Jira Issue OCPBUGS-35342, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
1a836aa to
5ce3eda
Compare
|
/retest |
gcs278
left a comment
There was a problem hiding this comment.
Thanks for the responses. Just a few more nits, but looks nearly good to go.
| } | ||
|
|
||
| // waitForPodReady waits for a pod to become ready. | ||
| func waitForPodReady(t *testing.T, cl client.Client, pod *corev1.Pod, timeout time.Duration) error { |
There was a problem hiding this comment.
Should this go in util_test.go?
There was a problem hiding this comment.
I was thinking about but I didn't see any system in the way waitFor* functions are placed so I decided to keep it closer to other waitFor from the same file. May need a dedicated PR to reorder the util functions.
| t.Helper() | ||
| name := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name} | ||
| p := &corev1.Pod{} | ||
| err := wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) { |
| if err := kclient.Get(context.Background(), wildcardRecordName, wildcardRecord); err != nil { | ||
| t.Fatalf("failed to get wildcard dnsrecord %s: %v", wildcardRecordName, err) | ||
| } | ||
| elbHostname := wildcardRecord.Spec.Targets[0] |
There was a problem hiding this comment.
nit You imply ELBs here, but this test is not restricted to just AWS. Looks like it works fine on GCP and Azure, but shall we use a more generic term, like just lb or load balancer? I don't think they are called ELBs on GCP and azure.
Also, your comments mention ELBs as well.
| defaultQueueNum = 100 | ||
| ) | ||
|
|
||
| func NewServeDelayConnectCommand() *cobra.Command { |
There was a problem hiding this comment.
Hmm, I ran it again, this time running:
$ sudo iptables -I INPUT -p tcp --dport 8080 -m conntrack --ctstate NEW -j NFQUEUE --queue-num 100
$ sudo ./ingress-operator serve-delay-connect-test-server
[sudo] password for gspence:
2024/06/20 19:14:54 Callback is registered with delay 40s
2024/06/20 19:14:54 Serving on 8080
2024/06/20 19:14:56 Callback is triggered
2024/06/20 19:14:56 Delaying SYN packet for 40s
I curled in another terminal:
$ curl localhost:8080
HTTP/1.1
But immediately got an answer back. Seems like progress since I get log output for the serve-delay-connect-test-server command, but why would curl return immediately and not delay?
I suppose it could be some configuration with my laptop, because I did test with the route created by your E2E test, and it delays successfully. So I believe it works, but I was just curious why it wouldn't work on my local laptop.
EDIT: I figured it out. I need to use curl 127.0.0.1:8080, then it works. I wiresharked it and looks like curl switched to ipv6 with localhost, which didn't work correctly. Not sure if it's worth a comment somewhere that IPv6 doesn't work correctly.
5ce3eda to
84921fd
Compare
gcs278
left a comment
There was a problem hiding this comment.
just one more nit, but LGTM. I'm fine to send it through either way.
| func getDaemonSet(t *testing.T, client client.Client, name types.NamespacedName, timeout time.Duration) (*appsv1.DaemonSet, error) { | ||
| t.Helper() | ||
| ds := appsv1.DaemonSet{} | ||
| if err := wait.PollImmediate(1*time.Second, timeout, func() (bool, error) { |
- Added a new command to run a test HTTP server that delays TCP SYN packets - Implemented an e2e scenario to test the connect timeout tuning option
84921fd to
dc4e7f9
Compare
|
/retest |
|
Looks good! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 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 |
|
Install failed: /test e2e-gcp-operator |
|
/test e2e-gcp-operator |
This is little worrying. Let's see whether it's reproducible. /test e2e-aws-ovn-single-node |
|
@alebedev87: Jira Issue OCPBUGS-35342: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-35342 has been moved to the MODIFIED state. DetailsIn 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. |
|
/cherrypick release-4.16 |
|
@alebedev87: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked. Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of lines (20000)","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"406"}DetailsIn 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. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-ingress-operator-container-v4.17.0-202406252042.p0.gb18a07c.assembly.stream.el9 for distgit ose-cluster-ingress-operator. |
In order to be able to backport openshift/api#1829, I have been asked (by David Eads) to implement an e2e test. The ingress operator's e2e seems to be well suited for it similar to #1013.