e2e tests for timeout and draining timeout #521
Conversation
|
Thanks! Will take a look soon. /ok-to-test |
k8s-ci-robot
added
needs-ok-to-test
MrHohn
removed
the
needs-ok-to-test
k8s-ci-robot
added
the
cncf-cla: yes
pkg/fuzz/helpers.go
Outdated
| } | ||
|
|
||
| // SetDrainTimeout defines the BackendConfig's draining timeout | ||
| func (b *BackendConfigBuilder) SetDrainTimeout(timeout int64) *BackendConfigBuilder { |
There was a problem hiding this comment.
SetConnectionDrainingTimeout
cmd/e2e-test/draining_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func verifyDrainingTimeouts(t *testing.T, gclb *fuzz.GCLB, svcNamespace, svcName string, expectedTimeout int64) error { |
There was a problem hiding this comment.
verifyConnectionDrainingTimeout
cmd/e2e-test/draining_test.go
Outdated
| t.Errorf("Delete(%q) = %v, want nil", ing.Name, err) | ||
| } | ||
| t.Logf("Waiting for GCLB resources to be deleted (%s/%s)", s.Namespace, ing.Name) | ||
| if err := e2e.WaitForGCLBDeletion(ctx, Framework.Cloud, gclb, nil); err != nil { |
There was a problem hiding this comment.
Add SkipDefaultBackend option here as well.
cmd/e2e-test/timeout_test.go
Outdated
| t.Errorf("Delete(%q) = %v, want nil", ing.Name, err) | ||
| } | ||
| t.Logf("Waiting for GCLB resources to be deleted (%s/%s)", s.Namespace, ing.Name) | ||
| if err := e2e.WaitForGCLBDeletion(ctx, Framework.Cloud, gclb, nil); err != nil { |
There was a problem hiding this comment.
Can we add SkipDefaultBackend option here similar to #523? We are hitting some test flakiness lately because the default backend service may not be cleaned up if there is any other ingress that needs it.
cmd/e2e-test/draining_test.go
Outdated
| } | ||
|
|
||
| if err := verifyConnectionDrainingTimeout(t, gclb, s.Namespace, "service-1", timeout); err != nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
Can we make an error message with verifyConnectionDrainingTimeout(...)= format? Similar to
Otherwise it may be confusing in the logs as we run many tests in parallel.
| beConfig: fuzz.NewBackendConfigBuilder("", "backendconfig-1"). | ||
| Build(), | ||
| }, | ||
| } { |
There was a problem hiding this comment.
Ideally would be great to have a transition test as well (e.g. 60s -> 30s), but these should be good for the initial test.
There was a problem hiding this comment.
Will look into that next
cmd/e2e-test/timeout_test.go
Outdated
| } | ||
|
|
||
| if err := verifyTimeout(t, gclb, s.Namespace, "service-1", timeout); err != nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
log with format: verifyTimeout(...)=
|
/hold We can only merge this PR once we cut a new Ingress-GCE version. Thanks for the work! |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
lgtm
|
@bpineau Can you fix the unit test? |
Those are simple tests checking that the settings from BackendConfig propagates well to the live BackendService. Let me know if you'd rather me to write fuzz http reqs testing real timeouts expirations (that's much more involved -esp. the connection draining part- so it'll take more time though).
Reflect changes to ConnectionDraining.DrainingTimeoutSec in e2e/fuzz tests.
k8s-ci-robot
removed
the
lgtm
Test draining timeouts changes/transitions.
|
Rebased / fixed the test, and added transitions tests (as was suggested by @MrHohn a few weeks ago). |
| t.Errorf("verifyConnectionDrainingTimeout(..., %q, %q, %d) = %v, want nil", s.Namespace, "service-1", timeout, err) | ||
| } | ||
|
|
||
| // Test modifications/transitions |
There was a problem hiding this comment.
Ideally, the transition test should be another test func.
|
@bpineau Also can you please squash commits? |
|
@bpineau friendly ping |
|
/lgtm Let's just get this in, we can clean up the internals of the test later. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpineau, rramkumar1 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 |
As suggested by @MrHohn
Those are simple tests checking that the settings from BackendConfig propagates well
to the live BackendService.
Let me know if you'd rather me to write fuzz http reqs testing real timeouts expirations
(that's much more involved -esp. the connection draining part- so it'll take more time though).