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

[castedo]

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.

[debarshiray]

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]

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]

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]

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]

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]

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]

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

[debarshiray]

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]

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]

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