Skip to content

Add {{.TempDir}} and {{.GlobalTempDir}} variables to host templates #3318

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

Merged
merged 1 commit into from
May 1, 2025

Conversation

jandubois
Copy link
Member

@jandubois jandubois commented Mar 8, 2025

Allows writing the /tmp/lima mount as

mounts:
- location: "{{.Temp}}/lima"
  mountPoint: /tmp/lima

Note that on macOS this would use $TMPDIR and not /tmp, so would be a change in behaviour. It would not affect existing instances though.

@afbjorklund
Copy link
Member

afbjorklund commented Mar 8, 2025

Do you need to model "the real" $TMPDIR, or should it be something like $LIMA_TEMP instead?

One concern was that there are many users sharing the /tmp/lima location, on a shared server.

@arixmkii
Copy link
Contributor

arixmkii commented Mar 8, 2025

This made default.yaml compatible with Windows. I managed to create and start VM w/o changes to template.

Output of the mount command:

C:\msys64\home\Arthu\gitrepos\lima\_output\bin>limactl shell default mount | find "sshfs"
time="2025-03-08T20:21:48+02:00" level=warning msg="treating lima version \"e0487475.m\" from \"C:\\\\Users\\\\Arthu\\\\.lima\\\\default\\\\lima-version\" as very latest release"
bash: line 1: cd: /mnt/c/msys64/home/Arthu/gitrepos/lima/_output/bin: No such file or directory
:/mnt/c/Users/Arthu on /mnt/c/Users/Arthu type fuse.sshfs (ro,nosuid,nodev,relatime,user_id=1000,group_id=1000,allow_other)
:/mnt/c/Users/Arthu/AppData/Local/Temp/lima on /tmp/lima type fuse.sshfs (rw,nosuid,nodev,relatime,user_id=1000,group_id=1000,allow_other)

The /mnt/ prefixes come from WSL2 Alpine tools container (wslpath instead of cygpath), which I use for running QEMU. I'm yet to figure out required fixes to run it with Git bash msys2 stack (mux modes are not supported in msys2 OpenSSH).

@arixmkii
Copy link
Contributor

arixmkii commented Mar 8, 2025

9p mounts

C:\msys64\home\Arthu\gitrepos\lima\_output\bin>limactl shell default mount | find "9p"
time="2025-03-08T20:36:53+02:00" level=warning msg="treating lima version \"e0487475.m\" from \"C:\\\\Users\\\\Arthu\\\\.lima\\\\default\\\\lima-version\" as very latest release"
bash: line 1: cd: /mnt/c/msys64/home/Arthu/gitrepos/lima/_output/bin: No such file or directory
mount0 on /mnt/c/Users/Arthu type 9p (ro,relatime,cache=8f,access=client,msize=131072,trans=virtio)
mount1 on /tmp/lima type 9p (rw,relatime,cache=5,access=client,msize=131072,trans=virtio)

@jandubois
Copy link
Member Author

I'm going to leave this PR open (and in draft mode) because I think adding the {{.Temp}} variable and normalizing mountpoints is something we should eventually merge.

But the change to templates/default.yaml seems wrong to me and should not be merged! We need further discussion to figure out how to deal with this. I'll be offline from Mar 13 to 31, so can't participate until next month.

@jandubois
Copy link
Member Author

I've rebased this PR on latest master to solve the merge conflicts. The updated mount location is now localized to just template://_default/mounts.

This PR does introduce a user-visible change, in that /tmp/lima inside the VM is now mounted to $TMPDIR/lima on a Linux or macOS host. It will not change existing instances.

I'm undecided if this is acceptable or not.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

For compatibility {{ .Temp }} should use /tmp on macOS.
Probably we should have {{ .TempEnv }} (or {{ .TempDynamic }}) for $TMPDIR

@AkihiroSuda AkihiroSuda added this to the v1.1.0 milestone Apr 30, 2025
@jandubois
Copy link
Member Author

For compatibility {{ .Temp }} should use /tmp on macOS.

I assume you meant Unix, not macOS. os.TempDir() will use TMPDIR on Linux too.

Probably we should have {{ .TempEnv }} (or {{ .TempDynamic }}) for $TMPDIR

How about we use {{ .Tmp }} for /tmp and {{ .TmpDir }} for $TMPDIR, to make it easier to remember?

On Windows both variables would be the same.

@AkihiroSuda
Copy link
Member

How about we use {{ .Tmp }} for /tmp and {{ .TmpDir }} for $TMPDIR, to make it easier to remember?

To me this sounds like the former one is a file and the latter one is a directory

@jandubois
Copy link
Member Author

jandubois commented Apr 30, 2025

How about {{ .GlobalTemp }} for /tmp and {{ .Temp }} for $TMPDIR then?

The GlobalTemp variable really only exists for the sake of the template://_default/mount template to maintain backwards compatibility. So I would rather keep Temp for the version that users should be using.

PS: I think I actually prefer {{ .GlobalTempDir }} and {{ .TempDir }}.

@AkihiroSuda
Copy link
Member

PS: I think I actually prefer {{ .GlobalTempDir }} and {{ .TempDir }}.

👍

@jandubois jandubois marked this pull request as ready for review April 30, 2025 23:52
@jandubois jandubois changed the title Add {{.Temp}} variable to host templates Add {{.TempDir}} and {{.GlobalTempDir}} variables to host templates Apr 30, 2025
Allows writing the temp mount as

mounts:
- location: "{{.Temp}}/lima"
  mountPoint: /tmp/lima

Note that on macOS this would use $TMPDIR and not /tmp, so would be a change
in behaviour. It would not affect existing instances though.

Signed-off-by: Jan Dubois <jan.dubois@suse.com>
@jandubois jandubois force-pushed the location-template branch from 0e6e56d to 67a3361 Compare May 1, 2025 00:08
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 5e4a6b0 into lima-vm:master May 1, 2025
32 checks passed
@jandubois jandubois deleted the location-template branch May 1, 2025 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants