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

Don't preserve current working directory unless it's a bind mount from the host #988

Open
castedo opened this issue Jan 19, 2022 · 10 comments · May be fixed by #1205
Open

Don't preserve current working directory unless it's a bind mount from the host #988

castedo opened this issue Jan 19, 2022 · 10 comments · May be fixed by #1205
Labels
1. Feature request A request for a new feature

Comments

@castedo
Copy link

castedo commented Jan 19, 2022

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

  1. cd /var
  2. ls -id .; ls
  3. toolbox enter
  4. ls -id .; ls

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+)

[castedo@nasa ~]$ toolbox --version
toolbox version 0.0.99.2

Toolbox package info (rpm -q toolbox)

[castedo@nasa ~]$ rpm -q toolbox
toolbox-0.0.99.3-0.4.module+el8.5.0+12682+a4eeb084.x86_64

Info about your OS

[castedo@nasa ~]$ cat /etc/os-release 
NAME="Red Hat Enterprise Linux"
VERSION="8.5 (Ootpa)"
ID="rhel"
...

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.

@castedo castedo added the 1. Bug Something isn't working label Jan 19, 2022
@castedo castedo changed the title current working directory silently stays the same when container image does not have same directory current working directory path silently the same when container image does not have same directory Jan 19, 2022
@debarshiray debarshiray changed the title current working directory path silently the same when container image does not have same directory Don't preserve current working directory unless it's a bind mount from the host Nov 28, 2022
@debarshiray
Copy link
Member

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).

Interesting. I don't think I have seen this mentioned before.

@debarshiray
Copy link
Member

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 see. Interesting. Your approach of spawning a separate helper executable called cnest-entry inside the container is very much like my idea of a toolbox shell command that came up in a different context.

I see that you later changed it to invoke a separate podman exec from the host to check the device and inode numbers. However, this might not be a good idea because it can make things slow. We made some efforts to cut down on the number of such extra podman exec invocations in Toolbx to optimize the enter and run commands.

@castedo
Copy link
Author

castedo commented Nov 28, 2022

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 cnest script almost has no requirements on the container image. If the container image has certain enhancements, then cnest script takes advantage of them, but otherwise cnest script tries to not require the container image to do things that the cnest script can do itself.

@debarshiray
Copy link
Member

The change is a trade-off between having fewer requirements
placed on the container image vs delay in entering the persistent
container.

Umm... it need not be. You can bind mount the cnest executable when creating the container with podman create --volume /path/to/cnest:/usr/bin/cnest:ro .... That's been the case for Toolbx containers for quite a while now.

@castedo
Copy link
Author

castedo commented Nov 28, 2022

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 cnest script only has two trivial requirements for a container (https://cnest.readthedocs.io/en/latest/cnest-tools.html).

I've tried to have loose coupling between the three scripts that do:

  1. run/enter container (cnest) vs
  2. create container (create-nest) vs
  3. create container image (cnestify).

One can use all three, only two or just one (as long as simple requirements are met).

@debarshiray
Copy link
Member

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. :)

@halfline
Copy link

halfline commented Dec 5, 2022

maybe toolbox should just go to /run/host/$PWD ?

@debarshiray
Copy link
Member

debarshiray commented Dec 7, 2022

maybe toolbox should just go to /run/host/$PWD ?

Yeah, possibly. :)

We will have to be a bit careful. eg., if we just went to /run/host/$HOME then it will mess up the prompts in Bash and other shells because it won't show up as ~. However, that's easily done, because it can be entirely handled on the host and won't need poking inside the container.

I do wonder how useful it would really be to always end up inside /run/host. In the vast majority of cases, users will be somewhere inside $HOME which works just fine; and many of the other interesting locations are already bind mounts. So, it seems like we should only go there if the location isn't a bind mount to limit exposing casual users to the weird /run/host prefix.

Answering that is it a bind mount question will require inserting our own toolbox <some-command> invocation between podman exec and the actual shell during toolbox enter. This is also relevant for letting people have different default shells in different containers, and we already have a work-in-progress pull request for it.

The main hurdle from my perspective is figuring out a name for toolbox <some-command>. We could use some of your naming superpowers, @halfline !

@halfline
Copy link

halfline commented Dec 17, 2022

hmm i'm probably missing something, but why do we need a new command? Shouldn't the heuristic just be:

  1. If the path exists in the container and its the same as on the host, switch to it
  2. If the path doesn't exist in the container or its not the same as on the hose go to /run/host/$path

?

e.g.,

something like
(note not tested, I don't know go so it probably doesn't even compile, etc etc, just showing for illustrative purposes)

--- 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
        }

@halfline
Copy link

halfline commented Dec 19, 2022

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 :-)

halfline added a commit to halfline/toolbox that referenced this issue Dec 19, 2022
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
@halfline halfline linked a pull request Dec 19, 2022 that will close this issue
halfline added a commit to halfline/toolbox that referenced this issue Dec 19, 2022
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
halfline added a commit to halfline/toolbox that referenced this issue Dec 19, 2022
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
halfline added a commit to halfline/toolbox that referenced this issue Dec 19, 2022
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. Feature request A request for a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants