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

Make toolboxes use the container name as hostname. #771

Closed
wants to merge 1 commit into from

Conversation

runfalk
Copy link

@runfalk runfalk commented May 20, 2021

This allows the user to easily see which container they are currently in instead of the generic @toolbox.

I use multiple toolboxes, tied to the projects I'm working on. When I have many open at the same time it's hard for me to know which one I'm actually in since they all have the same hostname (toolbox). This changes things so that the container re-uses the container name as host name.

An alternative solution would be to prefix the prompt like Python virtualenvs do: (fedora-toolbox-34) user@toolbox:, but that's a bit trickier to get right.

@runfalk
Copy link
Author

runfalk commented May 20, 2021

It looks something like this:

runfalk@hostname:~/dev/toolbox (main)$ ./builddir/src/toolbox create --distro fedora --release f34
Image required to create toolbox container.
Download registry.fedoraproject.org/fedora-toolbox:34 (500MB)? [y/N]: y
Created container: fedora-toolbox-34
Enter with: toolbox enter
runfalk@hostname:~/dev/toolbox (main)$ toolbox enter
runfalk@fedora-toolbox-34:~/dev/toolbox (main)$

Copy link
Member

@HarryMichal HarryMichal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a mistake in changes of examples in README.

README.md Outdated Show resolved Hide resolved
images/fedora/f32/README.md Outdated Show resolved Hide resolved
images/fedora/f33/README.md Outdated Show resolved Hide resolved
images/fedora/f34/README.md Outdated Show resolved Hide resolved
This allows the user to easily see which container they are currently in instead of the generic @toolbox.
@runfalk
Copy link
Author

runfalk commented May 24, 2021

Good spotting. Fixed!

@runfalk runfalk requested a review from HarryMichal May 24, 2021 21:14
@HarryMichal
Copy link
Member

Thank you, @runfalk, for the patch! I'm not entirely sure about this change as it is not as minor as it would seem. I'd like to know what @debarshiray thinks about this. I'm inclined to accepting this but @debarshiray may have some more insight.

@HarryMichal HarryMichal added 2. Container Configuration Configuration of a container. Mounts, environmental variables, privileges. 2. Host Realm The issue is related to what happens on the host machine where Toolbox is executed 3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage 6. Requires Toolbx Recreation This will take effect in new Toolbox containers and not in existing ones labels May 24, 2021
@runfalk
Copy link
Author

runfalk commented May 25, 2021

I don't think this requires toolbox recreation (label 6). I tested this change with a pre-change toolbox and everything I tested worked just fine. This only changes new toolboxes. Is the hostname essential to anything in toolbox itself?

EDIT: I did a quick grep and the line I changed is the only occurance of hostname in the code (aside from the readmes and the hostname package that get installed).

@HarryMichal
Copy link
Member

I used the label because the change does not affect existing containers. Those already have the hostname set. Therefore, containers have to be recreated with the version of Toolbox that has this patch.

@runfalk
Copy link
Author

runfalk commented May 26, 2021

The point I wanted to make is that they don't have to be recreated, as they work perfectly fine even with the patched toolbox.

@HarryMichal
Copy link
Member

I found myself thinking about this today and realized this may be incompatible with #784. It is a classic problem with containers: the configuration is baked in and can not be changed. In order to make this work across container renames, some other mechanism should exist.

@mattweidner
Copy link

Perhaps a compromise would be to add a toolbox create option (--hostname) that is passed through to podman to set the container hostname to an arbitrary string. If no --hostname option is provided, the default string "toolbox" could be used. The user could choose to set the hostname the same as the container name or something else, but there would be no implicit expectation for the hostname to change when the container is renamed. This would mirror the operation of podman, allow users to have a choice in their container's hostname, and potentially deconflict #784.

@dabrain34
Copy link

dabrain34 commented Dec 16, 2021

would be good to have a prefix with toolbox always because you name your container without toolbox in the name, you might be confused too such as:

"--hostname", "toolbox-" + container,

@cig0
Copy link

cig0 commented Dec 22, 2021

I'm happy to see I'm not the only one that would love to see this feature implemented :)

Today I quickly put together the following workaround:

C=(ansible aws gcp python)

for i in "${C[@]}";
    do
        toolbox create -c $i \
        && toolbox run -c $i sudo hostname $i \
        && toolbox run -c $i hostname \
        && echo -e "\n\n";
    done

Which provides some relief (!?) to me:

image

@shoop
Copy link

shoop commented Jan 15, 2022

I have the opposite request, I don't understand why the hostname needs to be set. I fully support a mechanism to show which container is active, but I have issues with the hostname set to something different than the original host:

  1. Configuration software like chezmoi expects to be able to distinguish on hostname.
  2. Network software now suddenly needs to be able to resolve a not-really-existing hostname. Even the workaround proposed to use containername.hostname is fabricating a non-existing network name, really, as the toolbox itself does not have a separate network.

So I'd like to have the containername in /run/.toolboxenv and then have bash prompts etc be updated to use that, instead of messing around with what is in my view a system property.

@buck-ross
Copy link

buck-ross commented Jan 24, 2022

@shoop I have an alternative proposition, but first, I'd like to take the time to address the concerns you've enumerated:

Configuration software like chezmoi expects to be able to distinguish on hostname

This is true, but many software products with this requirement tend to require only a distinct hostname, not necessarily a DNS resolvable one. As such, it can become problematic if multiple toolbox containers, running the same software package, are given the same hostname (I.E, the host's hostname), meanwhile it seems entirely unnecessary to place the burden on the toolbox devs to ensure that the hostname is always DNS resolvable.

Network software now suddenly needs to be able to resolve a not-really-existing hostname

What about this is sudden? All toolbox containers currently have the hostname toolbox, which is presumably never network-resolvable. What we really need is a way to give some containers the same hostname as the host (or other resolvable names), and others may have other hostnames, which may or may not be resolvable.

So I'd like to have the containername in /run/.toolboxenv

This is already provided in /run/.containerenv. No need to modify /run/.toolboxenv. I have my environment configured to extract the container's name as follows:

if test -z "$CONTAINER_NAME" && test -f /run/.containerenv; then
    cat /run/.containerenv | sed -e '/^name=/!d' -e 's/^name="//' -e 's/"$//' | export CONTAINER_NAME="$(cat)"
fi

@buck-ross
Copy link

I propose that instead of having the hostname always set to the container name, or always set to the host's hostname, we should instead give the user the option of setting the hostname to some (sane) value when invoking toolbox create with an optional flag, like --hostname. This was already proposed in #210, but was recently closed due to the author seemingly being unwilling to re-write their changes in Go.

I've already isolated the exact line which needs to be changed (create.go#L395), and have a plan to add this new flag on my own fork as soon as I get the chance. Assuming this idea is well received, I'll make my own PR in a few weeks, once I've had a chance to write the new feature & all associated test-cases.

@debarshiray
Copy link
Member

The point I wanted to make is that they don't have to be
recreated, as they work perfectly fine even with the
patched toolbox.

What do you mean? As far as I can see, this pull request only changes the podman create ... invocation for creating new Toolbx containers. So how will it automatically update the hostnames of existing containers?

@@ -401,7 +401,7 @@ func createContainer(container, image, release string, showCommandToEnter bool)
createArgs = append(createArgs, xdgRuntimeDirEnv...)

createArgs = append(createArgs, []string{
"--hostname", "toolbox",
"--hostname", container,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that the rules for a valid hostname are different from the rules for a valid OCI container name.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve the default hostname of the containers, we should implement #969

When it comes to offering better ways to visually distinguish Toolbx containers from one another, see #98 for some ideas. eg., there's metadata in /run/.containerenv that can be used by users to customize their shell prompts without requiring changes in Toolbx itself.

@debarshiray
Copy link
Member

I am going to have to close this pull request.

However, thanks for your interest in Toolbx and contributing to the discussion.

@debarshiray
Copy link
Member

I'm happy to see I'm not the only one that would love to see this feature implemented :)

Today I quickly put together the following workaround:

C=(ansible aws gcp python)

for i in "${C[@]}";
    do
        toolbox create -c $i \
        && toolbox run -c $i sudo hostname $i \
        && toolbox run -c $i hostname \
        && echo -e "\n\n";
    done

Which provides some relief (!?) to me:

image

@mildred has another way to do this via a start-up script.

@debarshiray
Copy link
Member

I am going to have to close this pull request.

However, thanks for your interest in Toolbx and contributing to the discussion.

You are also welcome to help implement #969

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. Container Configuration Configuration of a container. Mounts, environmental variables, privileges. 2. Host Realm The issue is related to what happens on the host machine where Toolbox is executed 3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage 6. Requires Toolbx Recreation This will take effect in new Toolbox containers and not in existing ones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants