Adding restart kubelet flag on e2e test #97028

knabben · 2020-12-03T01:21:00Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adding a --restart-kubelet flag on Node E2E suite, the flag can be used to control the restart/not of Kubelet when it is killed.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Kubelet --exit-on-lock-contention will exit Kubelet when a new lock file is created, this flag is used in the suite to not bring this up via systemctl.

Does this PR introduce a user-facing change?:

Adding `--restart-kubelet` flag on E2E Node test suite

knabben · 2020-12-03T13:24:05Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

neolit123 · 2020-12-03T21:18:37Z

test/e2e/framework/test_context.go

@@ -221,6 +221,8 @@ type NodeTestContextType struct {
 	// the node e2e test. If empty, the default one (system.DefaultSpec) is
 	// used. The system specs are in test/e2e_node/system/specs/.
 	SystemSpecName string
+	// RestartKubelet restarts Kubelet unit when the process is killed.
+	RestartKubelet bool


i don't think we should make the framework test context aware of such an option.
in fact a lot of options should be trimmed out of it if we eventually want to treat it as an API.

/cc @oomichi @alejandrox1

@neolit123 As you stated, this still can be applied to other options, only used on Node E2E scope, inside the NodeTestContextType and even TestContextType directly. I don't see any other immediate place (used in the Node E2E suite) to set up and share the variable without reading the flag on init(), any suggestion?

given a number of flags already use the framework context for storage, this one can go there as well.

kubernetes/test/e2e_node/e2e_node_suite_test.go

Lines 81 to 87 in 00da68d

flags.BoolVar(&framework.TestContext.NodeConformance, "conformance", false, "If true, the test suite will not start kubelet, and fetch system log (kernel, docker, kubelet log etc.) to the report directory.")

flags.BoolVar(&framework.TestContext.PrepullImages, "prepull-images", true, "If true, prepull images so image pull failures do not cause test failures.")

flags.BoolVar(&framework.TestContext.RestartKubelet, "restart-kubelet", true, "If true, restart Kubelet unit when the process is killed.")

flags.StringVar(&framework.TestContext.ImageDescription, "image-description", "", "The description of the image which the test will be running on.")

flags.StringVar(&framework.TestContext.SystemSpecName, "system-spec-name", "", "The name of the system spec (e.g., gke) that's used in the node e2e test. The system specs are in test/e2e_node/system/specs/. This is used by the test framework to determine which tests to run for validating the system requirements.")

flags.Var(cliflag.NewMapStringString(&framework.TestContext.ExtraEnvs), "extra-envs", "The extra environment variables needed for node e2e tests. Format: a list of key=value pairs, e.g., env1=val1,env2=val2")

flags.StringVar(&framework.TestContext.SriovdpConfigMapFile, "sriovdp-configmap-file", "", "The name of the SRIOV device plugin Config Map to load.")

but long term this should be decoupled.
ideally the PR should pass review by SIG Node / e2e_node owners.

According to the ln#49-76,

TestContextType contains test settings and global state. It is due to historical reasons that it is a mixture of items managed by the test framework itself, cloud providers and individual tests, the end goal is to move anything not required by the framework into the code which uses the settings.

The recommendation for those settings is:

They are stored in their own context structure or local variables. (In test/e2e_node/e2e_node_suite_test.go as suggested)

The standard flag package is used to register them.

The flag name should follow the pattern <part1>.<part2>....<partn> where the prefix is unlikely to conflict with other tests or standard packages and each part is in lower camel case. (eg: node.kubelet.restartOnExit ?)

WDYT?

@SergeyKanzhelev this is an open discussion, do reusing the TestContext for node tests a topic to be reviewed? Should we start on this PR?

I think @alejandrox1 started a document for this refactoring.

I like the suggested refactoring, but I agree it is outside the scope of this PR. I think @neolit123 agreed with this. @ike-ma any concerns with making refactoring separately?

@SergeyKanzhelev this is an open discussion, do reusing the TestContext for node tests to be reviewed, starting on this PR?

I think @alejandrox1 started a document for this refactoring.

@alejandrox1 @knabben Do we have an issue for the refactoring?

@SergeyKanzhelev @cynepco3hahue #101986

neolit123 · 2020-12-03T21:22:08Z

test/e2e_node/e2e_node_suite_test.go

@@ -80,6 +80,7 @@ func registerNodeFlags(flags *flag.FlagSet) {
 	// It is hard and unnecessary to deal with the complexity inside the test suite.
 	flags.BoolVar(&framework.TestContext.NodeConformance, "conformance", false, "If true, the test suite will not start kubelet, and fetch system log (kernel, docker, kubelet log etc.) to the report directory.")
 	flags.BoolVar(&framework.TestContext.PrepullImages, "prepull-images", true, "If true, prepull images so image pull failures do not cause test failures.")
+	flags.BoolVar(&framework.TestContext.RestartKubelet, "restart-kubelet", true, "If true, restart Kubelet unit when the process is killed.")


this seems fine as long as sig node are OK with the change.
@kubernetes/sig-node-pr-reviews

Adding --restart-kubelet flag on E2E Node test suite

please explain what the flag does in the release note too.

looks like the flag was already approved here:
#96775 (review)

neolit123 · 2020-12-03T21:22:37Z

test/e2e_node/services/kubelet.go

@@ -302,6 +302,7 @@ func (e *E2EServices) startKubelet() (*server, error) {
 	}

 	cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)
+	restartOnExit := framework.TestContext.RestartKubelet


can another variable be used to store the flag state instead of framework.TestContext.RestartKubelet ?

Reading the flag from the file init is one option, I'm not sure if is a pattern we want to propagate here:

func init() { flag.Var(&kubeletArgs, "kubelet-flags", "Kubelet flags passed to kubelet, this will override default kubelet flags in the test. Flags specified in multiple kubelet-flags will be concatenate.") flag.BoolVar(&genKubeletConfigFile, "generate-kubelet-config-file", true, "The test runner will generate a Kubelet config file containing test defaults instead of passing default flags to the Kubelet.") flag.BoolVar(&restartKubelet, "restart-kubelet", true, "If true, restart Kubelet unit when the process is killed.") }

It seems to be the recommended way to do init() here and use a local variable for the restart-kubelet flag based on test/e2e/framework/test_context.go ln#49-76?

The point is to decouple specific node flags from the main e2e framework context. But adding these flags on the init() here without further context it's only augmenting the mess.

@neolit123 we opened an issue to move this TestContext.RestartKubelet to a proper place, for this specific PR, we are keeping in the global one, wdyt?

knabben · 2020-12-21T21:30:23Z

/retest

knabben · 2021-03-08T17:16:35Z

/reopen

SergeyKanzhelev · 2021-04-07T18:19:09Z

/assign @ike-ma

k8s-ci-robot · 2021-04-07T18:19:10Z

@SergeyKanzhelev: GitHub didn't allow me to assign the following users: ike-ma.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @ike-ma

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.

rata · 2021-04-20T14:50:48Z

Friendly ping? :)

ike-ma · 2021-04-20T20:21:06Z

Ack. Review in progress.

SergeyKanzhelev · 2021-05-12T17:07:46Z

/test pull-kubernetes-node-e2e-containerd

What's the current status of this PR?

SergeyKanzhelev · 2021-05-12T17:08:22Z

/assign @cynepco3hahue

to help @ike-ma to review

cynepco3hahue · 2021-05-13T09:53:29Z

I currently do not see a lot of issues adding the flag under the TestContextType because the flag affects the test infrastructure behavior and does not affect the kubelet configuration. But it can be nice to add TODO and issue number that will point to the fact that we want to have a separate context for e2e node variables.

cynepco3hahue · 2021-05-18T07:16:22Z

/lgtm

oomichi · 2021-06-22T21:41:14Z

Sorry for my late response.
This PR doesn't change the test behavior by default and adds a flag for restarting.
That seems straightforward for me.

/lgtm
/approve

k8s-ci-robot · 2021-06-22T21:41:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knabben, oomichi

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/framework/OWNERS~~ [oomichi]
~~test/e2e_node/OWNERS~~ [oomichi]

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

knabben · 2021-06-23T00:59:48Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

k8s-ci-robot · 2021-06-23T04:00:10Z

@knabben: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-ubuntu-containerd	`00da68d`	link	`/test pull-kubernetes-e2e-gce-ubuntu-containerd`

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.

This flag was added to the test suite in kubernetes/kubernetes#97028 However as it turns out, in order to test the exit on lock contention, it is not needed, hence removing the flag from the test job. Signed-off-by: Imran Pochi <imran@kinvolk.io>

Adding restart kubelet flag on e2e test

00da68d

k8s-ci-robot requested review from fabriziopandini and sjenning December 3, 2020 01:21

knabben mentioned this pull request Dec 3, 2020

Kubelet exit on lock contention E2E test #96775

Closed

neolit123 reviewed Dec 3, 2020

View reviewed changes

k8s-ci-robot requested review from alejandrox1 and oomichi December 3, 2020 21:18

neolit123 reviewed Dec 3, 2020

View reviewed changes

knabben closed this Feb 20, 2021

k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 17, 2021

ike-ma mentioned this pull request Apr 21, 2021

Adding Kubelet E2E Lock Contention job kubernetes/test-infra#20106

Closed

k8s-ci-robot assigned cynepco3hahue May 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2021

k8s-ci-robot assigned oomichi Jun 22, 2021

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

k8s-ci-robot merged commit af60beb into kubernetes:master Jun 23, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 23, 2021

This was referenced Jun 28, 2021

Tracker for migrating lock contention flags to config in k8s upstream kinvolk/lokomotive#1514

Open

Add e2e lock contention job kubernetes/test-infra#22758

Merged

ipochi mentioned this pull request Jul 9, 2021

E2E test for kubelet exit-on-lock-contention #103608

Merged

ipochi mentioned this pull request Jul 9, 2021

Remove restart kubelet flag from e2e lock contention kubernetes/test-infra#22836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding restart kubelet flag on e2e test #97028

Adding restart kubelet flag on e2e test #97028

knabben commented Dec 3, 2020 •

edited

Loading

knabben commented Dec 3, 2020

neolit123 Dec 3, 2020

knabben Dec 21, 2020 •

edited

Loading

neolit123 Dec 21, 2020

ike-ma Apr 20, 2021

knabben May 12, 2021 •

edited

Loading

SergeyKanzhelev May 13, 2021

cynepco3hahue May 13, 2021

knabben May 13, 2021

neolit123 Dec 3, 2020

neolit123 Dec 3, 2020

neolit123 Dec 3, 2020

knabben Dec 21, 2020

ike-ma Apr 21, 2021

knabben May 12, 2021

knabben May 13, 2021

neolit123 May 13, 2021

knabben commented Dec 21, 2020

knabben commented Mar 8, 2021

SergeyKanzhelev commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

rata commented Apr 20, 2021

ike-ma commented Apr 20, 2021

SergeyKanzhelev commented May 12, 2021

SergeyKanzhelev commented May 12, 2021

cynepco3hahue commented May 13, 2021

cynepco3hahue commented May 18, 2021

oomichi commented Jun 22, 2021

k8s-ci-robot commented Jun 22, 2021

knabben commented Jun 23, 2021

k8s-ci-robot commented Jun 23, 2021

	flags.BoolVar(&framework.TestContext.NodeConformance, "conformance", false, "If true, the test suite will not start kubelet, and fetch system log (kernel, docker, kubelet log etc.) to the report directory.")
	flags.BoolVar(&framework.TestContext.PrepullImages, "prepull-images", true, "If true, prepull images so image pull failures do not cause test failures.")
	flags.BoolVar(&framework.TestContext.RestartKubelet, "restart-kubelet", true, "If true, restart Kubelet unit when the process is killed.")
	flags.StringVar(&framework.TestContext.ImageDescription, "image-description", "", "The description of the image which the test will be running on.")
	flags.StringVar(&framework.TestContext.SystemSpecName, "system-spec-name", "", "The name of the system spec (e.g., gke) that's used in the node e2e test. The system specs are in test/e2e_node/system/specs/. This is used by the test framework to determine which tests to run for validating the system requirements.")
	flags.Var(cliflag.NewMapStringString(&framework.TestContext.ExtraEnvs), "extra-envs", "The extra environment variables needed for node e2e tests. Format: a list of key=value pairs, e.g., env1=val1,env2=val2")
	flags.StringVar(&framework.TestContext.SriovdpConfigMapFile, "sriovdp-configmap-file", "", "The name of the SRIOV device plugin Config Map to load.")

Adding restart kubelet flag on e2e test #97028

Adding restart kubelet flag on e2e test #97028

Conversation

knabben commented Dec 3, 2020 • edited Loading

knabben commented Dec 3, 2020

Choose a reason for hiding this comment

knabben Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knabben May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knabben commented Dec 21, 2020

knabben commented Mar 8, 2021

SergeyKanzhelev commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

rata commented Apr 20, 2021

ike-ma commented Apr 20, 2021

SergeyKanzhelev commented May 12, 2021

SergeyKanzhelev commented May 12, 2021

cynepco3hahue commented May 13, 2021

cynepco3hahue commented May 18, 2021

oomichi commented Jun 22, 2021

k8s-ci-robot commented Jun 22, 2021

knabben commented Jun 23, 2021

k8s-ci-robot commented Jun 23, 2021

knabben commented Dec 3, 2020 •

edited

Loading

knabben Dec 21, 2020 •

edited

Loading

knabben May 12, 2021 •

edited

Loading