Remove the restart kubelet check from the test.

[ipochi]

Upon reconsidering as to the purpose of the test i.e to test the lock contention flags
(--lock-file-contention and --lock-file), it makes sense that we test only the actual functionality
which is the kubelet should stop once there is a lock contention.

In no way it is the responsiblity of the kubelet to restart, which would be the responsiblity
of a higher system such as systemd. Hence the removal of the check for releasing the lock
and checking for whether the kubelet is healthy again or not seem out of scope from kubelet's responsiblities.

Thanks to @adisky for this perspective.

/cc @endocrimes @SergeyKanzhelev

Signed-off-by: Imran Pochi imran@kinvolk.io

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

This PR addresses the failing test.

https://k8s-testgrid.appspot.com/sig-node-kubelet#kubelet-gce-e2e-lock-contention

Which issue(s) this PR fixes:

Fixes #108348

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[ipochi]

/test pull-kubernetes-integration

[wzshiming]

kubernetes/cmd/kubelet/app/server.go

Lines 488 to 503 in d7d1219

	if s.ExitOnLockContention && s.LockFilePath == "" {
	return errors.New("cannot exit on lock file contention: no lock file specified")
	}
	done := make(chan struct{})
	if s.LockFilePath != "" {
	klog.InfoS("Acquiring file lock", "path", s.LockFilePath)
	if err := flock.Acquire(s.LockFilePath); err != nil {
	return fmt.Errorf("unable to acquire file lock on %q: %w", s.LockFilePath, err)
	}
	if s.ExitOnLockContention {
	klog.InfoS("Watching for inotify events", "path", s.LockFilePath)
	if err := watchForLockfileContention(s.LockFilePath, done); err != nil {
	return err
	}
	}
	}

kubernetes/cmd/kubelet/app/server_linux.go

Lines 26 to 48 in c3d9b10

	func watchForLockfileContention(path string, done chan struct{}) error {
	watcher, err := inotify.NewWatcher()
	if err != nil {
	klog.ErrorS(err, "Unable to create watcher for lockfile")
	return err
	}
	if err = watcher.AddWatch(path, inotify.InOpen|inotify.InDeleteSelf); err != nil {
	klog.ErrorS(err, "Unable to watch lockfile")
	watcher.Close()
	return err
	}
	go func() {
	select {
	case ev := <-watcher.Event:
	klog.InfoS("inotify event", "event", ev)
	case err = <-watcher.Error:
	klog.ErrorS(err, "inotify watcher error")
	}
	close(done)
	watcher.Close()
	}()
	return nil
	}

Looking at the logic, the old kubelet will exit after the new kubelet is started. this exited kubelet should indeed not be restarted.

[wzshiming]

kubernetes/cmd/kubelet/app/options/options.go

Lines 308 to 309 in 06e1070

	fs.StringVar(&f.LockFilePath, "lock-file", f.LockFilePath, "<Warning: Alpha feature> The path to file for kubelet to use as a lock file.")
	fs.BoolVar(&f.ExitOnLockContention, "exit-on-lock-contention", f.ExitOnLockContention, "Whether kubelet should exit upon lock-file contention.")

By the way, these two flags look very old, and there is still <Warning: Alpha feature> Is it possible to consider removing it

[ipochi]

kubernetes/cmd/kubelet/app/options/options.go

Lines 308 to 309 in 06e1070

	fs.StringVar(&f.LockFilePath, "lock-file", f.LockFilePath, "<Warning: Alpha feature> The path to file for kubelet to use as a lock file.")
	fs.BoolVar(&f.ExitOnLockContention, "exit-on-lock-contention", f.ExitOnLockContention, "Whether kubelet should exit upon lock-file contention.")

By the way, these two flags look very old, and there is still <Warning: Alpha feature> Is it possible to consider removing it

We want to graduate the flags and move to Kubelet configuration, hence this work.

One of the prerequisites of graduating is to have e2e test for the feature.

[adisky]

/lgtm

[wzshiming]

kubernetes/cmd/kubelet/app/options/options.go

Lines 308 to 309 in 06e1070

	fs.StringVar(&f.LockFilePath, "lock-file", f.LockFilePath, "<Warning: Alpha feature> The path to file for kubelet to use as a lock file.")
	fs.BoolVar(&f.ExitOnLockContention, "exit-on-lock-contention", f.ExitOnLockContention, "Whether kubelet should exit upon lock-file contention.")

By the way, these two flags look very old, and there is still <Warning: Alpha feature> Is it possible to consider removing it

We want to graduate the flags and move to Kubelet configuration, hence this work.

One of the prerequisites of graduating is to have e2e test for the feature.

> find . -name '*.go' | xargs -I {}  grep -rni "<Warning: Alpha feature>" {}
./cmd/kubelet/app/options/options.go:307:       fs.Var(&bindableNodeLabels, "node-labels", fmt.Sprintf("<Warning: Alpha feature> Labels to add when registering the node in the cluster.  Labels must be key=value pairs separated by ','. Labels in the 'kubernetes.io' namespace must begin with an allowed prefix (%s) or be in the specifically allowed set (%s)", strings.Join(kubeletapis.KubeletLabelNamespaces(), ", "), strings.Join(kubeletapis.KubeletLabels(), ", ")))
./cmd/kubelet/app/options/options.go:308:       fs.StringVar(&f.LockFilePath, "lock-file", f.LockFilePath, "<Warning: Alpha feature> The path to file for kubelet to use as a lock file.")
./cmd/kubelet/app/options/options.go:310:       fs.BoolVar(&f.SeccompDefault, "seccomp-default", f.SeccompDefault, "<Warning: Alpha feature> Enable the use of `RuntimeDefault` as the default seccomp profile for all workloads. The SeccompDefault feature gate must be enabled to allow this flag, which is disabled per default.")
./cmd/kubelet/app/options/options.go:452:       fs.DurationVar(&c.CPUManagerReconcilePeriod.Duration, "cpu-manager-reconcile-period", c.CPUManagerReconcilePeriod.Duration, "<Warning: Alpha feature> CPU Manager reconciliation period. Examples: '10s', or '1m'. If not supplied, defaults to 'NodeStatusUpdateFrequency'")
./cmd/kubelet/app/options/options.go:453:       fs.Var(cliflag.NewMapStringString(&c.QOSReserved), "qos-reserved", "<Warning: Alpha feature> A set of ResourceName=Percentage (e.g. memory=50%) pairs that describe how pod resource requests are reserved at the QoS level. Currently only memory is supported. Requires the QOSReserved feature gate to be enabled.")

It looks like there are a few more of this type, migrating to the Kubelet configuration file I feel needs KEP which involves API changes

[wzshiming]

/lgtm

[adisky]

/triage accepted
/priority important-soon

[SergeyKanzhelev]

In no way it is the responsiblity of the kubelet to restart, which would be the responsiblity
of a higher system such as systemd.

@cynepco3hahue I rememeber you were looking into this in one of the tests. Do you have any comments here?

[SergeyKanzhelev]

long term we want to merge this test back into the Serial lane (once it will be configured using config file). And the fact that kubelet wouldn't restart will become a problem. I pinged @cynepco3hahue in case there is a quick fix here. For now this is OK to merge

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [SergeyKanzhelev]

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

[adisky]

long term we want to merge this test back into the Serial lane (once it will be configured using config file). And the fact that kubelet wouldn't restart will become a problem. I pinged @cynepco3hahue in case there is a quick fix here. For now this is OK to merge

/lgtm

Thanks @SergeyKanzhelev for the insight, Now I know why we were checking restart as well. But rather making it a part of test itself we can add kubelet restart as cleanup or AfterEach`,let this pass successfully we can later iterate over to merge in serial lane

[endocrimes]

I'm a little hesitant about this change - I think it makes sense for the test in a case like this (especially in an e2e suite) to also describe expected interactions with other systems, but appreciate that could hinder testing Kubelet in stranger deployment cases with the upstream suite.

We definitely need to add a cleanup step at a minimum here though.