Add timeout config for node_e2e tests #83268

odinuge · 2019-09-28T09:43:18Z

What type of PR is this?
/kind feature
/sig testing

What this PR does / why we need it:

Some test suits use more than the default 45m, resulting in the test to
crash. Without invoking the go runner manually, it is currently impossible to set the timeout when running the tests.

Here is an example of a test suite taking ~2 hours.

The current 45m limit comes from here: https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/remote/remote.go#L34

Which issue(s) this PR fixes:

Fixes # N/A

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2019-09-28T09:43:20Z

@odinuge: The label(s) area/testing cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

What type of PR is this?
/kind feature
/area testing

What this PR does / why we need it:

Some test suits use more than the default 45m, resulting in the test to
crash.

The current 45m limit comes from here: https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/remote/remote.go#L34

Which issue(s) this PR fixes:

Fixes # N/A

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

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/test-infra repository.

odinuge · 2019-09-28T09:51:32Z

hack/make-rules/test-e2e-node.sh

@@ -95,6 +95,7 @@ if [ "${remote}" = true ] ; then
  cleanup=${CLEANUP:-"true"}
  delete_instances=${DELETE_INSTANCES:-"false"}
  preemptible_instances=${PREEMPTIBLE_INSTANCES:-"false"}
+  test_timeout=${TIMEOUT:-"45m"}


Should we avoid hardcoding "45m" here (ref. https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/remote/remote.go#L34), and only use the test-timeout flag when the timeout is given? And if so, should we then still write Defaults to 45m in the Makefile?

Yeah, I'm plus one on avoiding hardcoding 45m - lets keep remote.go as the source of truth for the default. I think the Makefile should also indicate that remote.go contains the source of truth.

odinuge · 2019-09-28T09:54:02Z

/cc BenTheElder
/priority backlog

fejta · 2019-09-29T22:33:41Z

@kubernetes/sig-node-pr-reviews
/assign @dchen1107
/uncc @fejta @BenTheElder

odinuge · 2019-10-22T08:54:27Z

/cc @mattjmcnaughton @derekwaynecarr

mattjmcnaughton

I can only see benefits from adding the ability to configure the timeout :) None of the default behaviors are changed, nor is the behavior when running via CI. We are just giving the individual dev more flexibility.

LGTM modulo I agree with your comment around not hard coding "45m" in two places. Please ping me when you've updated the diff and I'll mark as "lgtm".

mattjmcnaughton · 2019-10-22T13:47:43Z

hack/make-rules/test-e2e-node.sh

@@ -95,6 +95,7 @@ if [ "${remote}" = true ] ; then
  cleanup=${CLEANUP:-"true"}
  delete_instances=${DELETE_INSTANCES:-"false"}
  preemptible_instances=${PREEMPTIBLE_INSTANCES:-"false"}
+  test_timeout=${TIMEOUT:-"45m"}


Yeah, I'm plus one on avoiding hardcoding 45m - lets keep remote.go as the source of truth for the default. I think the Makefile should also indicate that remote.go contains the source of truth.

odinuge · 2019-10-23T17:47:28Z

/test pull-kubernetes-node-e2e-containerd

fejta-bot · 2020-01-21T18:22:35Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

liggitt · 2020-06-19T18:39:18Z

hack/make-rules/test-e2e-node.sh

@@ -151,6 +155,7 @@ if [ "${remote}" = true ] ; then
    --delete-instances="${delete_instances}" --test_args="${test_args}" --instance-metadata="${metadata}" \
    --image-config-file="${image_config_file}" --system-spec-name="${system_spec_name}" \
    --preemptible-instances="${preemptible_instances}" --extra-envs="${extra_envs}" --test-suite="${test_suite}" \
+    "${timeout_arg}" \


what does this do if empty? is an empty arg tolerated?

Ahh, yeah, that works fine! Running with TIMEOUT=20s, TIMEOUT="" and without TIMEOUT works.

ping @liggitt

alejandrox1 · 2020-08-04T17:46:14Z

@odinuge would you be able to continue working on this?

Some test suits use more than the default 45m, resulting in the test to crash.

odinuge · 2020-08-09T15:56:56Z

/retest

SergeyKanzhelev · 2020-08-10T16:35:07Z

/retest

alejandrox1 · 2020-10-20T18:03:02Z

Looking through owners of build and hack...
/assign @spiffxp
ptal 🙏

fejta-bot · 2021-01-18T19:01:02Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

spiffxp

/remove-lifecycle stale
/approve
/lgtm

k8s-ci-robot · 2021-01-28T22:41:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, odinuge, spiffxp

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:

~~build/OWNERS~~ [spiffxp]
~~hack/OWNERS~~ [spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2021-01-29T01:34:11Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 28, 2019

k8s-ci-robot requested review from fejta and zmerlynn September 28, 2019 09:44

odinuge commented Sep 28, 2019

View reviewed changes

k8s-ci-robot requested a review from BenTheElder September 28, 2019 09:54

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 28, 2019

k8s-ci-robot assigned dchen1107 Sep 29, 2019

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 29, 2019

k8s-ci-robot removed request for BenTheElder and fejta September 29, 2019 22:33

k8s-ci-robot requested review from derekwaynecarr and mattjmcnaughton October 22, 2019 08:54

mattjmcnaughton reviewed Oct 22, 2019

View reviewed changes

odinuge force-pushed the e2e_node_timeout branch 2 times, most recently from e5fcce1 to 14d386a Compare October 23, 2019 17:17

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2020

k8s-ci-robot assigned liggitt and derekwaynecarr Jun 19, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2020

liggitt reviewed Jun 19, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2020

alejandrox1 added this to PRs that need attention from the author in SIG Node CI/Test Board Aug 4, 2020

odinuge force-pushed the e2e_node_timeout branch from 14d386a to 08d808b Compare August 9, 2020 13:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2020

Add timeout config for node_e2e tests

6dfdf05

Some test suits use more than the default 45m, resulting in the test to crash.

odinuge force-pushed the e2e_node_timeout branch from 08d808b to 6dfdf05 Compare August 9, 2020 13:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2020

k8s-ci-robot assigned spiffxp Oct 20, 2020

alejandrox1 moved this from PRs that need attention from the author to PRs - Reviewer approved in SIG Node CI/Test Board Oct 20, 2020

SergeyKanzhelev moved this from PRs - Reviewer lgtm'd to PRs - Review in progress in SIG Node CI/Test Board Jan 4, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 18, 2021

SIG Node CI/Test Board automation moved this from PRs - Review in progress to PRs - Reviewer lgtm'd Jan 28, 2021

spiffxp approved these changes Jan 28, 2021

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

k8s-ci-robot merged commit d7cb340 into kubernetes:master Jan 29, 2021

SIG Node CI/Test Board automation moved this from PRs - Reviewer lgtm'd to Done Jan 29, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout config for node_e2e tests #83268

Add timeout config for node_e2e tests #83268

odinuge commented Sep 28, 2019 •

edited

k8s-ci-robot commented Sep 28, 2019

odinuge Sep 28, 2019

mattjmcnaughton Oct 22, 2019

odinuge commented Sep 28, 2019

fejta commented Sep 29, 2019

odinuge commented Oct 22, 2019

mattjmcnaughton left a comment

mattjmcnaughton Oct 22, 2019

odinuge commented Oct 23, 2019

fejta-bot commented Jan 21, 2020

liggitt Jun 19, 2020

odinuge Aug 9, 2020

odinuge Aug 15, 2020

alejandrox1 commented Aug 4, 2020

odinuge commented Aug 9, 2020

SergeyKanzhelev commented Aug 10, 2020

alejandrox1 commented Oct 20, 2020

fejta-bot commented Jan 18, 2021

spiffxp left a comment

k8s-ci-robot commented Jan 28, 2021

fejta-bot commented Jan 29, 2021

Add timeout config for node_e2e tests #83268

Add timeout config for node_e2e tests #83268

Conversation

odinuge commented Sep 28, 2019 • edited

k8s-ci-robot commented Sep 28, 2019

odinuge Sep 28, 2019

Choose a reason for hiding this comment

mattjmcnaughton Oct 22, 2019

Choose a reason for hiding this comment

odinuge commented Sep 28, 2019

fejta commented Sep 29, 2019

odinuge commented Oct 22, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

mattjmcnaughton Oct 22, 2019

Choose a reason for hiding this comment

odinuge commented Oct 23, 2019

fejta-bot commented Jan 21, 2020

liggitt Jun 19, 2020

Choose a reason for hiding this comment

odinuge Aug 9, 2020

Choose a reason for hiding this comment

odinuge Aug 15, 2020

Choose a reason for hiding this comment

alejandrox1 commented Aug 4, 2020

odinuge commented Aug 9, 2020

SergeyKanzhelev commented Aug 10, 2020

alejandrox1 commented Oct 20, 2020

fejta-bot commented Jan 18, 2021

spiffxp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 28, 2021

fejta-bot commented Jan 29, 2021

odinuge commented Sep 28, 2019 •

edited