Revert #104308 to bring back LockContention tests #104334

ipochi · 2021-08-12T15:46:48Z

This reverts commit 9d09c9d

This E2E test was reverted because the test was failing continuously.
More on the issue here #104307

What type of PR is this?

/kind feature
/sig node

What this PR does / why we need it:

This PR re-reverts and brings back the LockContention test, with
the addition of [Serial] tag to the test.

Which issue(s) this PR fixes:

This E2E test was reverted because the test was failing continuously.
More on the issue here #104307

SergeyKanzhelev · 2021-08-12T20:29:57Z

/approve cancel

it is failing on serial tab. Can you please check why?

ipochi · 2021-08-13T08:24:39Z

/approve cancel

it is failing on serial tab. Can you please check why?

@SergeyKanzhelev

Lock contention flags are not added to the the tests which are failing. Created a PR to update the job.
kubernetes/test-infra#23243

Question: If I want to re-run the failing test but this time I want the test to use the job yaml from the PR and not the master branch. How can I do that ?

/cc @ehashman

endocrimes · 2021-09-07T16:24:03Z

@ipochi Unfortunately the easiest way to do that is by running them from your machine, which requires having a Google Cloud account configured, with a default application profile and project that won't interfere with any production environments you care about (gcloud auth application-default login)

and then e.g:

make test-e2e-node REMOTE=true FOCUS="LockContention" SKIP="" PARALLELISM=1 IMAGE_CONFIG_FILE="path/to/image-config.yaml" ZONE=us-central1-a

endocrimes · 2021-09-07T16:24:35Z

/triage accepted
/priority important-longterm

ehashman · 2021-09-09T15:40:46Z

test/e2e_node/lock_contention_test.go

+
+const contentionLockFile = "/var/run/kubelet.lock"
+
+var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Serial] [NodeFeature:LockContention]", func() {


Suggested change

var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Serial] [NodeFeature:LockContention]", func() {

var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Feature:LockContention]", func() {

We'll need to update the test-infra manifest. This should avoid this getting picked up by the suites that launch the kubelet without the right command line flags.

@SergeyKanzhelev do we want to consider marking this [NodeSpecialFeature] and skip that in the regular feature suite?

I think this might have to stay serial? - (given that it messes with kubelet health)

@ehashman I've made it NodeSpecialFeature:Contention, I think that works better than Feature:LockContention.

What do you think ?

SergeyKanzhelev · 2021-09-09T15:57:00Z

/assign

…608-imran/e2e-lock-contention" This reverts commit 9d09c9d This E2E test was reverted becuase the test was failing continously. More on the issue here kubernetes#104307 This commit re-reverts and brings back the LockContention test, with the addition of [Serial] tag to the test.

ipochi · 2022-01-07T06:06:55Z

@SergeyKanzhelev friendly ping :)

SergeyKanzhelev · 2022-01-12T00:52:59Z

test/e2e_node/lock_contention_test.go

+var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Serial] [NodeFeature:LockContention]", func() {
+
+	ginkgo.It("Kubelet should stop when the test acquires the lock on lock file and restart once the lock is released", func() {
+


it may be useful to check the kubelet was started with the proper flag and skip if not. Or at lease add a comment here explaining that this is needed for test to run

SergeyKanzhelev

lgtm, beyond file rename and adding a comment or skipper

This commit is to be squashed and merged with the first commit. Signed-off-by: Imran Pochi <imran@kinvolk.io>

ipochi · 2022-01-20T06:19:20Z

@SergeyKanzhelev Thank you for reviewing the PR. I think I accidentally forced pushed an older version of the commit which resulted in the name changed (_linux) not appearing and couple of other review comments which I addressed such as the setting the permalinks.

I've ensured this time around by pushing in the latest commit. Please review once again (I've created a secondary commit containing the changes). Sorry for the mess.

ipochi · 2022-01-20T06:21:55Z

@SergeyKanzhelev @ehashman

The job for the lock contention test was removed in the test-infra repo.

What needs to be done on that repo, to get the job back ?

k8s-ci-robot · 2022-01-20T06:47:34Z

@ipochi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-node-kubelet-serial-containerd	e45fdc7a74a81a4c933e01dae00e26530f9d6787	link	`/test pull-kubernetes-node-kubelet-serial-containerd`
pull-kubernetes-node-kubelet-serial	e45fdc7a74a81a4c933e01dae00e26530f9d6787	link	`/test pull-kubernetes-node-kubelet-serial`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

ipochi · 2022-01-20T08:47:56Z

/retest-required

ipochi · 2022-02-10T10:45:35Z

@SergeyKanzhelev @ehashman

The job for the lock contention test was removed in the test-infra repo.

What needs to be done on that repo, to get the job back ?

@SergeyKanzhelev @ehashman friendly ping :)

endocrimes · 2022-02-15T17:15:25Z

@ipochi We'll need a new PR in test-infra that adds a new version of the job. I'd basically copy the containerd-serial job with a new name and updated filters.

to the test suite. This test is created due to the need of `--lock-file` and `--exit-on-lock-contention` be moved as flags and into the Kubelet configuration file rather than be dropped. Adds a new tab in TestGrid named `kubelet-gce-e2e-lock-contention` running tests focused on `NodeSpecialFeature:LockContention`. Corresponding e2e test is at kubernetes/kubernetes#104334 Signed-off-by: Imran Pochi <imranpochi@microsoft.com>

ipochi · 2022-02-16T11:43:31Z

@ipochi We'll need a new PR in test-infra that adds a new version of the job. I'd basically copy the containerd-serial job with a new name and updated filters.

kubernetes/test-infra#23243

ehashman

/lgtm
/approve

this looks fine, we can revert if it's not

k8s-ci-robot · 2022-02-17T23:42:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, ipochi

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:

~~test/e2e_node/OWNERS~~ [ehashman]

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

This commit moves the `--exit-on-lock-contention` and `--lock-file` kubelet flags to Kubelet Configuration. This PR is built on the following PRs: Corresponding E2E test PRs : kubernetes#103608, kubernetes#104334, kubernetes#108563 Corresponding Job in test-infra : https://github.com/kubernetes/test-infra/blob/e684255cc8701ef97b6832e3daadb6841c00cc65/config/jobs/kubernetes/sig-node/containerd.yaml#L1315-#L1343 Signed-off-by: Imran Pochi <imran@kinvolk.io>

k8s-ci-robot requested review from dchen1107 and mrunalp August 12, 2021 15:48

SergeyKanzhelev approved these changes Aug 12, 2021

View reviewed changes

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

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Aug 12, 2021

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2021

SergeyKanzhelev moved this from Triage to PRs Waiting on Author in SIG Node CI/Test Board Aug 12, 2021

ehashman added this to Triage in SIG Node PR Triage Aug 12, 2021

ipochi mentioned this pull request Aug 13, 2021

Add E2E test for lock contention flags kubernetes/test-infra#23243

Merged

endocrimes moved this from Triage to Waiting on Author in SIG Node PR Triage Sep 7, 2021

SIG Node CI/Test Board automation moved this from PRs Waiting on Author to PRs - Needs Reviewer Sep 9, 2021

ehashman suggested changes Sep 9, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 5, 2021

ipochi force-pushed the imran/re-revert-lock-contention-tests branch from 7b3eb73 to a6a558e Compare December 2, 2021 06:18

ipochi requested a review from SergeyKanzhelev December 3, 2021 01:04

ipochi force-pushed the imran/re-revert-lock-contention-tests branch 2 times, most recently from 435d353 to 61c4732 Compare January 4, 2022 02:03

SergeyKanzhelev reviewed Jan 12, 2022

View reviewed changes

Addressing review comments

6071f6e

This commit is to be squashed and merged with the first commit. Signed-off-by: Imran Pochi <imran@kinvolk.io>

ehashman reviewed Feb 17, 2022

View reviewed changes

k8s-ci-robot assigned ehashman Feb 17, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2022

k8s-ci-robot merged commit 23ccbaf into kubernetes:master Feb 18, 2022

SIG Node CI/Test Board automation moved this from PRs Waiting on Author to Done Feb 18, 2022

SIG Node PR Triage automation moved this from Waiting on Author to Done Feb 18, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 18, 2022

ipochi mentioned this pull request Mar 15, 2022

kubelet: Move Lock Contention flags to configuration #104302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert #104308 to bring back LockContention tests #104334

Revert #104308 to bring back LockContention tests #104334

ipochi commented Aug 12, 2021

SergeyKanzhelev commented Aug 12, 2021

ipochi commented Aug 13, 2021 •

edited

endocrimes commented Sep 7, 2021

endocrimes commented Sep 7, 2021

ehashman Sep 9, 2021

endocrimes Sep 9, 2021

ipochi Oct 4, 2021

SergeyKanzhelev commented Sep 9, 2021

ipochi commented Jan 7, 2022

SergeyKanzhelev Jan 12, 2022

SergeyKanzhelev left a comment

ipochi commented Jan 20, 2022

ipochi commented Jan 20, 2022 •

edited

k8s-ci-robot commented Jan 20, 2022 •

edited

ipochi commented Jan 20, 2022

ipochi commented Feb 10, 2022

endocrimes commented Feb 15, 2022

ipochi commented Feb 16, 2022

ehashman left a comment

k8s-ci-robot commented Feb 17, 2022


		const contentionLockFile = "/var/run/kubelet.lock"

		var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Serial] [NodeFeature:LockContention]", func() {

		var _ = SIGDescribe("Lock contention [Slow] [Disruptive] [Serial] [NodeFeature:LockContention]", func() {

		ginkgo.It("Kubelet should stop when the test acquires the lock on lock file and restart once the lock is released", func() {

Revert #104308 to bring back LockContention tests #104334

Revert #104308 to bring back LockContention tests #104334

Conversation

ipochi commented Aug 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

SergeyKanzhelev commented Aug 12, 2021

ipochi commented Aug 13, 2021 • edited

endocrimes commented Sep 7, 2021

endocrimes commented Sep 7, 2021

ehashman Sep 9, 2021

Choose a reason for hiding this comment

endocrimes Sep 9, 2021

Choose a reason for hiding this comment

ipochi Oct 4, 2021

Choose a reason for hiding this comment

SergeyKanzhelev commented Sep 9, 2021

ipochi commented Jan 7, 2022

SergeyKanzhelev Jan 12, 2022

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

ipochi commented Jan 20, 2022

ipochi commented Jan 20, 2022 • edited

k8s-ci-robot commented Jan 20, 2022 • edited

ipochi commented Jan 20, 2022

ipochi commented Feb 10, 2022

endocrimes commented Feb 15, 2022

ipochi commented Feb 16, 2022

ehashman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2022

ipochi commented Aug 13, 2021 •

edited

ipochi commented Jan 20, 2022 •

edited

k8s-ci-robot commented Jan 20, 2022 •

edited