Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix minikube mount --kill #15778

Closed
medyagh opened this issue Feb 2, 2023 · 2 comments · Fixed by #15782
Closed

fix minikube mount --kill #15778

medyagh opened this issue Feb 2, 2023 · 2 comments · Fixed by #15782
Assignees
Labels
area/mount help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@medyagh
Copy link
Member

medyagh commented Feb 2, 2023

What Happened?

minikube mount --kill should kill all already mount processes spwan by minikube

to reproduce
try to mount 3 different folders
minikube mount -p minikube /tmp:/a1
minikube mount -p minikube /tmp:/a2
minikube mount -p minikube /tmp:/a3

and then
minikube mount --kill=true

and expect there be no process

ps aux | grep "minikube mount"

Attach the log file

n/a

Operating System

None

Driver

None

@medyagh medyagh added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Feb 2, 2023
@x7upLime
Copy link
Contributor

x7upLime commented Feb 3, 2023

The issue comes from the fact that the kill flag relies on a mechanism(k8s.io/minikube/cmd/minikube/cmd.killMountProcess)
that expects the mounts to be mounted by k8s.io/minikube/pkg/minikube/node.configureMounts(),
which places a file containing the pid of a process in the localpath.Profile(profile).

Instead, $ minikube mount doesn't place anything,
and the code for the kill flag returns err==nil if the file is not present.


The code for $minikube mount just places a listening server and blocks until a ^C is issued,
or until kill request is received.. but there is currently no mechanism provided for the actual kill:
If one calls a mount multiple times putting the process in background:

$ ps -ef | grep "minikube mount"
andrew   1215678 1210216  0 16:31 pts/2    00:00:00 minikube mount -p minikube /tmp:/a1
andrew   1215771 1210216  0 16:31 pts/2    00:00:00 minikube mount -p minikube /tmp:/a2
andrew   1215865 1210216  0 16:31 pts/2    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232237 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232329 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232425 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232518 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232611 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3
andrew   1232703 1228957  0 17:18 pts/6    00:00:00 minikube mount -p minikube /tmp:/a3

@x7upLime
Copy link
Contributor

x7upLime commented Feb 3, 2023

// cmd/minikube/cmd/mount.go -- 	Run: func(cmd *cobra.Command, args []string)
 		var wg sync.WaitGroup
+		pid := make(chan int)
 		if cfg.Type == nineP {
 			wg.Add(1)
-			go func() {
+			go func(pid chan int) {
+				pid <- os.Getpid()
 				out.Styled(style.Fileserver, "Userspace file server: ")
 				ufs.StartServer(net.JoinHostPort(bindIP, strconv.Itoa(port)), debugVal, hostPath)
 				out.Step(style.Stopped, "Userspace file server is shutdown")
 				wg.Done()
-			}()
+			}(pid)
 		}

// pkg/minikube/cluster/mount.go -- func Mount()
 		return &MountError{ErrorType: MountErrorUnknown, UnderlyingError: errors.Wrapf(err, "mount with cmd %s ", rr.Command())}
 	}
 
+	profile := viper.GetString("profile")
+	if err := lock.WriteFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(strconv.Itoa(pid)), 0o644); err != nil {
+		exit.Error(reason.HostMountPid, "Error writing mount pid", err)
+	}
+
 	klog.Infof("mount successful: %q", rr.Output())
 	return nil
 }

also relying on lock.Writefile as the configureMounts() implementation would not be enough:
we would overwrite the file each time,
so multiple calls to $minikube mount would makes us lose track of the previous active processes...
and $ minikube mount --kill=true would result in killing only the last one.

$ minikube mount -p minikube /tmp:/a1 &
$ minikube mount -p minikube /tmp:/a2 &
$ ps -ef | grep "minikube mount"
andrew   1268190 1268040  1 19:01 pts/6    00:00:00 minikube mount -p minikube /tmp:/a1
andrew   1268288 1268040  5 19:01 pts/6    00:00:00 minikube mount -p minikube /tmp:/a2

$ minikube mount --kill=true
$ ps -ef | grep "minikube mount"
andrew   1268190 1268040  0 19:01 pts/6    00:00:00 minikube mount -p minikube /tmp:/a1

Perhaps we could use a lock.AppendToFile()?

x7upLime added a commit to x7upLime/minikube that referenced this issue Feb 3, 2023
The delete mechanism called by the --kill flag logic
inside the minikube mount command, relies on a .mount-process file
inside the .minikube dotfolder in home directory.

For this to work, the mount should've been instantiated
by some mechanism that also creates the file; like
k8s.io/minikube/pkg/minikube/node.configureMounts()

To just add the local.WriteFile() logic from configureMounts()
is not enough for the usecase in kubernetes#15778:
user's consequent "minikube mount" calls would break the cleaning
logic, since the file's content gets overwritten at each new call.
So on subsequent minikube mounts, a call to "minikube mount
--kill=true" would remove just the last pid, leaving the other
mounts unaltered, and no trace of them in any place...(leak?)

For the same mechanism to work here,
we should rely on some local.AppendFile() function, to add
separated pids consequently.
This way only "minikube mount" would use this new append logic,
leaving unaltered the other functions.

We now have a new behaviour for the .mount-process file,
in order to address this, we should modify the delete logic from
cmd/minikube/cmd/delete.go -- killProcess(),
so that we can both kill a single pid, as well as multiple
space-separated pids in the same way.

...

Ah yes..
we're slightly modifying the "minikube mount" RunE anon function,
in order to write the pid into the .mount-process file;
as well as modifying cluster.Mount()'s signature to accept the pid.
That should be more than safe, since cluster.Mount() is used only here
x7upLime added a commit to x7upLime/minikube that referenced this issue Feb 8, 2023
The delete mechanism called by the --kill flag logic
inside the minikube mount command, relies on a .mount-process file
inside the .minikube dotfolder in home directory.

For this to work, the mount should've been instantiated
by some mechanism that also creates the file; like
k8s.io/minikube/pkg/minikube/node.configureMounts()

To just add the local.WriteFile() logic from configureMounts()
is not enough for the usecase in kubernetes#15778:
user's consequent "minikube mount" calls would break the cleaning
logic, since the file's content gets overwritten at each new call.
So on subsequent minikube mounts, a call to "minikube mount
--kill=true" would remove just the last pid, leaving the other
mounts unaltered, and no trace of them in any place...(leak?)

For the same mechanism to work here,
we should rely on some local.AppendFile() function, to add
separated pids consequently.
This way only "minikube mount" would use this new append logic,
leaving unaltered the other functions.

We now have a new behaviour for the .mount-process file,
in order to address this, we should modify the delete logic from
cmd/minikube/cmd/delete.go -- killProcess(),
so that we can both kill a single pid, as well as multiple
space-separated pids in the same way.

...

Ah yes..
we're slightly modifying the "minikube mount" RunE anon function,
in order to write the pid into the .mount-process file;
as well as modifying cluster.Mount()'s signature to accept the pid.
That should be more than safe, since cluster.Mount() is used only here
@spowelljr spowelljr added area/mount priority/backlog Higher priority than priority/awaiting-more-evidence. labels Feb 18, 2023
x7upLime added a commit to x7upLime/minikube that referenced this issue Apr 21, 2023
The delete mechanism called by the --kill flag logic
inside the minikube mount command, relies on a .mount-process file
inside the .minikube dotfolder in home directory.

For this to work, the mount should've been instantiated
by some mechanism that also creates the file; like
k8s.io/minikube/pkg/minikube/node.configureMounts()

To just add the local.WriteFile() logic from configureMounts()
is not enough for the usecase in kubernetes#15778:
user's consequent "minikube mount" calls would break the cleaning
logic, since the file's content gets overwritten at each new call.
So on subsequent minikube mounts, a call to "minikube mount
--kill=true" would remove just the last pid, leaving the other
mounts unaltered, and no trace of them in any place...(leak?)

For the same mechanism to work here,
we should rely on some local.AppendFile() function, to add
separated pids consequently.
This way only "minikube mount" would use this new append logic,
leaving unaltered the other functions.

We now have a new behaviour for the .mount-process file,
in order to address this, we should modify the delete logic from
cmd/minikube/cmd/delete.go -- killProcess(),
so that we can both kill a single pid, as well as multiple
space-separated pids in the same way.

...

Ah yes..
we're slightly modifying the "minikube mount" RunE anon function,
in order to write the pid into the .mount-process file;
as well as modifying cluster.Mount()'s signature to accept the pid.
That should be more than safe, since cluster.Mount() is used only here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/mount help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants