-
Notifications
You must be signed in to change notification settings - Fork 219
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
Don't preserve current working directory unless it's a bind mount from the host #988
Comments
Interesting. I don't think I have seen this mentioned before. |
I see. Interesting. Your approach of spawning a separate helper executable called I see that you later changed it to invoke a separate |
The change is a trade-off between having fewer requirements placed on the container image vs delay in entering the persistent container. I can honestly say that I never notice the extra delay. In my daily use I usually enter one of these persistent containers a few times a week and then I just stay inside them (in a terminal window) for days. I remember timing it and deciding the extra delay was negligible. But there was a measurable extra delay. Given that entering never seems slow, I like that |
Umm... it need not be. You can bind mount the |
True. I should say a trade-off between delay and having fewer requirements on how the container is created and what container image is used. The I've tried to have loose coupling between the three scripts that do:
One can use all three, only two or just one (as long as simple requirements are met). |
Yes, agreed. Toolbx is pretty self-contained or tightly coupled. Supporting random containers is a non-goal for Toolbx. :) Anyway, cnest looks like a cool project. :) |
maybe toolbox should just go to |
Yeah, possibly. :) We will have to be a bit careful. eg., if we just went to I do wonder how useful it would really be to always end up inside Answering that is it a bind mount question will require inserting our own The main hurdle from my perspective is figuring out a name for |
hmm i'm probably missing something, but why do we need a new command? Shouldn't the heuristic just be:
? e.g., something like --- a/src/cmd/run.go
+++ b/src/cmd/run.go
@@ -295,12 +295,13 @@ func runCommandWithFallbacks(container string, command []string, emitEscapeSeque
envOptions := utils.GetEnvOptionsForPreservedVariables()
runFallbackCommandsIndex := 0
runFallbackWorkDirsIndex := 0
workDir := workingDirectory
+ runFallbackWorkDirs := append(runFallbackWorkDirs, "/run/host" + workDir)
for {
execArgs := constructExecArgs(container, command, detachKeysSupported, envOptions, workDir)
if emitEscapeSequence {
fmt.Printf("\033]777;container;push;%s;toolbox;%s\033\\", container, currentUser.Uid)
@@ -519,13 +520,13 @@ func isPathPresent(container, path string) (bool, error) {
logLevelString := podman.LogLevel.String()
args := []string{
"--log-level", logLevelString,
"exec",
"--user", currentUser.Username,
container,
- "sh", "-c", "test -d \"$1\"", "sh", path,
+ "sh", "-c", "test -d \"$1\" -a \"$1\" -ef \"/run/host/$1\"", "sh", path,
}
if err := shell.Run("podman", nil, nil, nil, args...); err != nil {
return false, err
} |
So I decided to just try the above patch this morning and as expected it didn't work at first, but it actually did do what I was expecting after a small change (prepending /run/host + workDir instead of appending it to the fallback list). I think this is better behavior, so I'll submit it as a pull request and we can discuss whether or not I'm missing something there, I guess :-) |
As a convenience to users, when entering a container toolbx tries to maintain the working directory from the host. This works great if the user is inside their home directory, for instance, since the home directory is shared between host and container. It can be confusing in other cases, though. The issue is that the directory in the container may have the same path as the directory in the host, but have completely different contents. The old contents may actually be in /run/host/$PWD instead. Switching to /run/host/$PWD unconditionally has its own downsides. For one, it's ugly, and also, in common cases, like subdirectories of the home directory, it's unnecessary. This commit tries to find the balance, by making toolbx check first if the directory is shared between host and container, and if not, only then falling back to trying /run/host/$PWD. Closes containers#988
As a convenience to users, when entering a container toolbx tries to maintain the working directory from the host. This works great if the user is inside their home directory, for instance, since the home directory is shared between host and container. It can be confusing in other cases, though. The issue is that the directory in the container may have the same path as the directory in the host, but have completely different contents. The old contents may actually be in /run/host/$PWD instead. Switching to /run/host/$PWD unconditionally has its own downsides. For one, it's ugly, and also, in common cases, like subdirectories of the home directory, it's unnecessary. This commit tries to find the balance, by making toolbx check first if the directory is shared between host and container, and if not, only then falling back to trying /run/host/$PWD. Closes containers#988
As a convenience to users, when entering a container toolbx tries to maintain the working directory from the host. This works great if the user is inside their home directory, for instance, since the home directory is shared between host and container. It can be confusing in other cases, though. The issue is that the directory in the container may have the same path as the directory in the host, but have completely different contents. The old contents may actually be in /run/host/$PWD instead. Switching to /run/host/$PWD unconditionally has its own downsides. For one, it's ugly, and also, in common cases, like subdirectories of the home directory, it's unnecessary. This commit tries to find the balance, by making toolbx check first if the directory is shared between host and container, and if not, only then falling back to trying /run/host/$PWD. Closes containers#988
As a convenience to users, when entering a container toolbx tries to maintain the working directory from the host. This works great if the user is inside their home directory, for instance, since the home directory is shared between host and container. It can be confusing in other cases, though. The issue is that the directory in the container may have the same path as the directory in the host, but have completely different contents. The old contents may actually be in /run/host/$PWD instead. Switching to /run/host/$PWD unconditionally has its own downsides. For one, it's ugly, and also, in common cases, like subdirectories of the home directory, it's unnecessary. This commit tries to find the balance, by making toolbx check first if the directory is shared between host and container, and if not, only then falling back to trying /run/host/$PWD. Closes containers#988 Signed-off-by: Ray Strode <rstrode@redhat.com>
Describe the bug
toolbox enter
changes to the home directory only when the current working directory path does not exist in the container. However many paths in the container are to a different directory that in the host (different in the sense of having different contents and inode).It's confusing enough that container and host filesystems are different but with some directories shared/mounted. It probably makes confusion worse acting like nothing unusual has happened when directory contents and inode silently change even though they have the same path.
Steps how to reproduce the behaviour
Expected behaviour
A message and/or a change to home directory in the container.
Actual behaviour
Nothing unusual, as though the user is still in /var of the host.
Output of
toolbox --version
(v0.0.90+)Toolbox package info (
rpm -q toolbox
)Info about your OS
Additional context
FWIW, I've coded https://github.com/castedo/cnest to change to the home directory if PWD is a different device or inode in the container, even if the path exists. Cnest prints out a message to the user in any scenario where the directory is not exactly the same. Here's the line of code sending inode info into the container:
https://github.com/castedo/cnest/blob/d3a333fac1c92356ee406ece8cbc05818fefa20b/bin/cnest#L47
and the code inside the container making use of it:
https://github.com/castedo/cnest/blob/main/cnest/data/cnest-entry
Note it is the combination of inode plus device number that uniquely identifies the true directory identity.
I'm not 100% convinced cnest does the desirable behaviour. But it does seem like it's better to not behave exactly the same in the case of shared vs different directories.
The text was updated successfully, but these errors were encountered: