Fixes mount cleaning mechanism #15782

x7upLime · 2023-02-03T19:39:14Z

MASTER:

$ minikube mount -p minikube /tmp:/a1 &
$ minikube mount -p minikube /tmp:/a2 &
$ minikube mount -p minikube /tmp:/a3 &

$ ps -ef | grep "minikube mount"
andrew   1357636 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a1
andrew   1357728 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a2
andrew   1357819 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a3

$ minikube mount --kill=true

$ ps -ef | grep "minikube mount"
andrew   1357636 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a1
andrew   1357728 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a2
andrew   1357819 1241621  0 21:30 pts/5    00:00:00 minikube mount -p minikube /tmp:/a3

FIX:

$ minikube mount -p minikube /tmp:/a1 &
$ minikube mount -p minikube /tmp:/a2 &
$ minikube mount -p minikube /tmp:/a3 &

$ ps -ef | grep "minikube mount"
andrew   1360220 1241621  1 21:37 pts/5    00:00:00 minikube mount -p minikube /tmp:/a1
andrew   1360313 1241621  1 21:37 pts/5    00:00:00 minikube mount -p minikube /tmp:/a2
andrew   1360409 1241621  3 21:37 pts/5    00:00:00 minikube mount -p minikube /tmp:/a3

$ minikube mount --kill=true

$ ps -ef | grep "minikube mount"

fixes #15778

k8s-ci-robot · 2023-02-03T19:39:23Z

Hi @x7upLime. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

minikube-bot · 2023-02-03T19:41:05Z

Can one of the admins verify this patch?

medyagh

Thank you for this pr, could you please add more comments on pid and how this pr adds new mechanisms to record pids

medyagh · 2023-02-04T22:36:01Z

/ok-to-test

medyagh · 2023-02-04T22:36:17Z

Let’s also add integration test

x7upLime · 2023-02-05T17:55:17Z

Let’s also add integration test

Done.
Added a test that ensures process removal for profile-specific mount processes

x7upLime · 2023-02-05T18:29:57Z

Thank you for this pr, could you please add more comments on pid and how this pr adds new mechanisms to record pids

Sure.
The killMountProcess() mechanism from master does the following

From master

// func killMountProcess() error {
  // checks the profile we're passing
	profile := viper.GetString("profile") 

  // goes to the minikube profile path
	paths := []string{  
		localpath.MiniPath(),
		localpath.Profile(profile),
	}

 // passes the profile path to killProcess
	for _, path := range paths {
		if err := killProcess(path); err != nil {
			return err

// func killProcess(path string) error {
     // looks at a ".mount-process" file
	pidPath := filepath.Join(path, constants.MountProcessFileName)

      /// reads a single pid out of it
     	out, err := os.ReadFile(pidPath)
	pid, err := strconv.Atoi(string(out))
 // The rest tries to find it and kill it, failing if something goes wrong

That .mount-process file gets also created by pkg/minikube/node.configureMounts:

// func configureMounts(wg *sync.WaitGroup, cc config.ClusterConfig) {

  // writes a single pid/file
	if err := lock.WriteFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(strconv.Itoa(mountCmd.Process.Pid)), 0o644); err != nil {
		exit.Error(reason.HostMountPid, "Error writing mount pid", err)
	}

subsequent calls of this would overwrite the previously written pid.

What I'm proposing is...

from fix-minikube_mount

// We write the .mount-process with this new function,
// so that we can be sure that subsequent files will not overwrite
func AppendToFile(filename string, data []byte, perm os.FileMode) error {

// Which we're using here -> pkg/minikube/cluster.Mount()
// func Mount(r mountRunner, source string, target string, c *MountConfig, pid int) error {
	if err := lock.AppendToFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(fmt.Sprintf("%s ", strconv.Itoa(pid))), 0o644); err != nil {
// to append a pid with trailing space [ ! ]

That brings us to the new mechanism:

// killMountProcess() stays the same

// func killProcess(path string) error {
// we read the .mount-process file
	cnt, err := os.ReadFile(pidPath)

// we 'strings.Fields()' it, so that even if there's a single pid without a trailing space,
// written by a call to lock.WriteToFile, we can still obtain a slice with one elem
	ppp := strings.Fields(string(cnt))

// then we're initializing a slice of errors
	var errs []error

// and instead of failing at each error during pid processing:
	for _, pp := range ppp {

// we still try to kill all processes, and add the reference to that err
			errs = append(errs, errors.Wrap(err, fmt.Sprintf("error parsing pid for %s", pp)))

// then at the end of pid processing we check the gathered errs references
// if only one.. we're returning it
	if len(errs) == 1 {
		return errs[0]
// if multiple errors we're showing them
// and returning a new multiple-errors error
	} else if len(errs) != 0 {
		out.Step(style.Failure, "Ecountered multiple errors:")
		for _, e := range errs {
			out.Err("%v\n", e)
		}
		return errors.New("multiple errors encountered while closing mount processes")
	}

medyagh

overall looks like a good PR just minor changes requested, thank you for this PR @x7upLime

cmd/minikube/cmd/delete.go

pkg/util/lock/lock.go

test/integration/functional_test_mount_test.go

x7upLime · 2023-02-08T23:19:58Z

I clicked on something I definitely shouldn't have..

Adds a helper function to check that the executable name is minikube.

What happened in the failed integration tests was that the starting process was not "minikube", it was "minikube-linux-" and "minikube-darwin-" instead.

The integration test error was due to the fact that multiple pids were written to the file without spaces between them, breaking the pid-kill logic

Otherway I see no obvious way to pass this check during (say) unit testing

:)

x7upLime · 2023-04-21T22:15:46Z

the last commit is just duplicated logic from the one used in the delete command...
I'm thinking we could move it inside the pkg/util and use it from there..

cmd/minikube/cmd/mount.go

cmd/minikube/cmd/delete.go

test/integration/functional_test_mount_test.go

cmd/minikube/cmd/mount.go

minikube-pr-bot · 2023-04-25T13:15:28Z

kvm2 driver with docker runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 15782) |
+-------------------+----------+---------------------+
| minikube start    | 50.4s    | 48.6s               |
| ⚠️  enable ingress | 25.9s    | 37.6s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube start: 48.8s 52.7s 49.3s 50.3s 51.1s
Times for minikube (PR 15782) start: 50.9s 49.8s 47.9s 46.4s 48.2s

Times for minikube ingress: 25.7s 27.2s 25.2s 23.8s 27.7s
Times for minikube (PR 15782) ingress: 84.2s 29.1s 25.7s 25.3s 23.7s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15782) |
+----------------+----------+---------------------+
| minikube start | 23.4s    | 22.2s               |
| enable ingress | 32.4s    | 32.7s               |
+----------------+----------+---------------------+

Times for minikube start: 24.7s 22.4s 25.1s 22.1s 22.5s
Times for minikube (PR 15782) start: 21.2s 24.9s 21.3s 21.4s 22.3s

Times for minikube ingress: 21.0s 20.0s 49.5s 50.5s 21.0s
Times for minikube (PR 15782) ingress: 81.5s 21.5s 19.5s 20.0s 21.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 15782) |
+----------------+----------+---------------------+
| minikube start | 22.0s    | 21.3s               |
| enable ingress | 32.1s    | 32.3s               |
+----------------+----------+---------------------+

Times for minikube start: 24.0s 20.4s 19.4s 23.1s 23.1s
Times for minikube (PR 15782) start: 20.2s 20.8s 23.3s 21.7s 20.8s

Times for minikube (PR 15782) ingress: 31.5s 34.0s 31.5s 33.0s 31.5s
Times for minikube ingress: 32.0s 31.5s 32.0s 33.0s 32.0s

minikube-pr-bot · 2023-04-25T14:26:32Z

These are the flake rates of all failed tests.

Environment	Failed Tests	Flake Rate (%)
Docker_macOS	TestNoKubernetes/serial/ProfileList (gopogh)	3.73 (chart)
Docker_macOS	TestNoKubernetes/serial/Start (gopogh)	3.73 (chart)
Docker_macOS	TestNoKubernetes/serial/StartNoArgs (gopogh)	3.73 (chart)
Docker_macOS	TestNoKubernetes/serial/StartWithK8s (gopogh)	3.73 (chart)
Docker_macOS	TestNoKubernetes/serial/StartWithStopK8s (gopogh)	3.73 (chart)
Docker_macOS	TestNoKubernetes/serial/Stop (gopogh)	3.73 (chart)
Docker_macOS	TestNetworkPlugins/group/auto/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/bridge/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/calico/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/custom-flannel/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/enable-default-cni/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/false/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/flannel/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/kindnet/Start (gopogh)	3.75 (chart)
Docker_macOS	TestNetworkPlugins/group/kubenet/Start (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/AddonExistsAfterStop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/DeployApp (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/EnableAddonAfterStop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/EnableAddonWhileActive (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/FirstStart (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/SecondStart (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/Stop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/UserAppExistsAfterStop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/default-k8s-diff-port/serial/VerifyKubernetesImages (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/DeployApp (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/EnableAddonAfterStop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/EnableAddonWhileActive (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/FirstStart (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/Stop (gopogh)	3.75 (chart)
Docker_macOS	TestStartStop/group/embed-certs/serial/VerifyKubernetesImages (gopogh)	3.75 (chart)
More tests...	Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

k8s-ci-robot · 2023-04-26T19:35:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spowelljr, x7upLime

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:

~~OWNERS~~ [spowelljr]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 3, 2023

k8s-ci-robot requested review from afbjorklund and sharifelgamal February 3, 2023 19:39

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2023

medyagh reviewed Feb 4, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 4, 2023

This comment has been minimized.

Sign in to view

medyagh requested changes Feb 7, 2023

View reviewed changes

cmd/minikube/cmd/delete.go Outdated Show resolved Hide resolved

pkg/util/lock/lock.go Outdated Show resolved Hide resolved

test/integration/functional_test_mount_test.go Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

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

x7upLime force-pushed the fix-minikube_mount branch from 43e0e57 to 3605dee Compare February 8, 2023 23:17

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

x7upLime requested review from medyagh and removed request for afbjorklund and sharifelgamal February 8, 2023 23:18

x7upLime added 14 commits April 22, 2023 00:49

Ensures sigkilling only minikube started processes

ddec310

Adds a helper function to check that the executable name is minikube.

Changes pid owner conditional

d72f2fb

What happened in the failed integration tests was that the starting process was not "minikube", it was "minikube-linux-" and "minikube-darwin-" instead.

Changes the way in which pid is written to mounts file

e5b57f8

The integration test error was due to the fact that multiple pids were written to the file without spaces between them, breaking the pid-kill logic

Generates docs

9665bad

Adds the ownership check as a dependency

c55a4ca

Otherway I see no obvious way to pass this check during (say) unit testing

Adapts unit test to new dependency

82abc0d

Adapts TestTryKillOne log for failing

595c8ee

Corrects mendacious comment

8f50ee9

Regenerates docs

1398988

Corrects style issue

b09cd43

Saves us from passing func as parameter

0170981

:)

Apply suggestions from code review

b196ee4

added stuff to reflect suggestions

3d3e6a1

Adds mechanism to remove pid from pidfile if mount is sigtermd

aab0cb6

x7upLime force-pushed the fix-minikube_mount branch from 3ea179f to aab0cb6 Compare April 21, 2023 21:50

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

spowelljr requested changes Apr 21, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

refactors removePid logic

455bcb1

This comment has been minimized.

Sign in to view

spowelljr requested changes Apr 25, 2023

View reviewed changes

Applied suggestions

8931619

spowelljr approved these changes Apr 26, 2023

View reviewed changes

spowelljr merged commit 6d9a5bf into kubernetes:master Apr 26, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes mount cleaning mechanism #15782

Fixes mount cleaning mechanism #15782

x7upLime commented Feb 3, 2023

k8s-ci-robot commented Feb 3, 2023

minikube-bot commented Feb 3, 2023

medyagh left a comment

medyagh commented Feb 4, 2023

medyagh commented Feb 4, 2023

This comment has been minimized.

This comment has been minimized.

x7upLime commented Feb 5, 2023

x7upLime commented Feb 5, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

medyagh left a comment

This comment has been minimized.

This comment has been minimized.

x7upLime commented Feb 8, 2023

x7upLime commented Apr 21, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

minikube-pr-bot commented Apr 25, 2023

minikube-pr-bot commented Apr 25, 2023

k8s-ci-robot commented Apr 26, 2023

Fixes mount cleaning mechanism #15782

Fixes mount cleaning mechanism #15782

Conversation

x7upLime commented Feb 3, 2023

MASTER:

FIX:

k8s-ci-robot commented Feb 3, 2023

minikube-bot commented Feb 3, 2023

medyagh left a comment

Choose a reason for hiding this comment

medyagh commented Feb 4, 2023

medyagh commented Feb 4, 2023

This comment has been minimized.

This comment has been minimized.

x7upLime commented Feb 5, 2023

x7upLime commented Feb 5, 2023

From master

from fix-minikube_mount

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

medyagh left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

x7upLime commented Feb 8, 2023

x7upLime commented Apr 21, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

minikube-pr-bot commented Apr 25, 2023

minikube-pr-bot commented Apr 25, 2023

k8s-ci-robot commented Apr 26, 2023