cirruslabs / tart

macOS and Linux VMs on Apple Silicon to use in CI and other automations
https://tart.run
Other
3.67k stars 103 forks source link

Add noauto option to --dir flag, for explicitly mounted directories #730

Closed torarnv closed 5 months ago

torarnv commented 5 months ago

The directories have to be named, and will be available in the guest for mounting via mount_virtiofs name mountpoint on macOS or mount -t virtiofs name mountpoint on Linux.

torarnv commented 5 months ago

You can unmount the directory upon VM launch

Yepp, I'm aware, but got tired of unmounting and re-mounting, that's why I added this noauto option 😄

Maybe we should add it here? 🤔

Makes sense

fkorotkov commented 5 months ago

As you could already noticed we are very conservative about adding new options and flags if it's already can be achieved with a little automation. There are already so many arguments! 😅 If the reason of noauto is to remove couple lines from your scripts then I propose just to update inline docs to mention unmounting option.

This default behaviour is used in GitLabs plugin and a couple other places. Will be a shame if an accident noauto will break it.

torarnv commented 5 months ago

This default behaviour is used in GitLabs plugin and a couple other places. Will be a shame if an accident noauto will break it.

Not sure I follow. The option is optional, just like ro. How would you accidentally add noauto? 😅

As you could already noticed we are very conservative about adding new options and flags if it's already can be achieved with a little automation. There are already so many arguments! 😅 If the reason of noauto is to remove couple lines from your scripts then I propose just to update inline docs to mention unmounting option.

Auto-mounting (or explicit mounting) is a feature of Virtualization.framework. It makes sense for Tart, as a convenient wrapper around the framework, to expose the features that are available, instead of forcing people to do provisioning workarounds.

As Tart grows, and has more users, it's natural that more use-cases are uncovered. As long as the options are clear, with clear semantics, I don't think it's a problem to have many options (docker run --help). The semantics of noauto are well known, and the options format of --dir matches that of Docker's --volume argument..

fkorotkov commented 5 months ago

With noauto one can pass one directory with noauto and other without noauto. In this case we won't be able to satisfy such config.

What do you think of #731 as an alternative to noauto? You'll be able to specify a tag that won't be auto mounted.

torarnv commented 5 months ago

With noauto one can pass one directory with noauto and other without noauto. In this case we won't be able to satisfy such config.

I'm not sure why you're saying that is not satisfiable. It works fine with the proposed patch.

❯ ./scripts/run-signed.sh run --dir build:~/build --dir dev:~/dev:noauto sonoma

Results in the content of ~/build mounted into /Volumes/My Shared Files, just like if it was the only --dir option. While the content of ~/dev must me manually mounted with mount_virtiofs.

What do you think of #731 as an alternative to noauto? You'll be able to specify a tag that won't be auto mounted.

I think that's a worse alternative. It adds a new global run option, compared to this, which adds a --dir specific option. If the worry is that we're adding too many options, then surely one that is specific to a sub-command's sub-flag is better than one on the same level as other commands options? :)

Also the approach in https://github.com/cirruslabs/tart/pull/731 doesn't allow you to have multiple tags, it forces you to use a single tag.

Also, it disables auto-mounting. You can not combine auto-mounting with non-auto-mounting.

fkorotkov commented 5 months ago

My bad, I missed that name is used as a tag when I initially looked at the code on Saturday.

I though your main problem was:

got tired of unmounting and re-mounting, that's why I added this noauto option

For this use case, #731 introducing a hidden flag IMO a better option. Your proposal is indeed more advanced and allows to have multiple mount points but code is too complex for this niche functionality.

torarnv commented 5 months ago

My bad, I missed that name is used as a tag when I initially looked at the code on Saturday.

I though your main problem was:

got tired of unmounting and re-mounting, that's why I added this noauto option

That was my initial use case, but as a feature it doesn't hurt to support more advanced use-cases as well.

For this use case, #731 introducing a hidden flag IMO a better option. Your proposal is indeed more advanced and allows to have multiple mount points but code is too complex for this niche functionality. Plus, the noauto option is a common mount option, and as mentioned before, Docker already supports multiple options, not just ro, so there's precedence for this way of passing mount options.

The proposal in #731 has the exact problem you mentioned above. It can not do auto-mount and non-auto mount together. It also adds exactly what you were arguing against: more options at a level where they don't need to be or belong. Should we add a --dir-mount-read-only option as well?

To be honest, I'm surprised at the push back for this patch (and the audio patch). Our organization is in the process of adopting Tart for our own CI, with a support contract, and that strategy was made based on my understanding that Tart was an open source project that could be improved for future use-cases.

I guess we can live with or work around things for our cases (possibly by patching Tart) for now, but that doesn't seem like a sustainable long term plan.

fkorotkov commented 5 months ago

I'm sorry about the pushback but Tart is used by hundreds of organizations and we can't just add any kind of features right away. This route will make it all unmaintainable at some point. I agree that it would be more productive to initially discuss your use case in details rather than figuring it out piece by piece from the PR and discussions here.

We started by having a problem of having to unmount a directory in your automation scripts. Which doesn't seem like a big problem since you are not doing it manually and it's still automated with scripts. Now we end up with a feature request to have customizable tags for shared directories so you can mount things separately. Do I understand your new requirements correctly?

Going forward I propose to open up a discussion first for big changes like this one.

torarnv commented 5 months ago

I'm sorry, I should have started a discussion first. I honestly didn't consider this such a big feature 😅

My initial use-case was that I didn't want the directories auto mounted, because I wanted them mounted somewhere outside of /Volumes. I also wanted an easy way to unmount and re-mount a directory, since virtio has issues picking up changes of files on the host side (known issue), and remounting is one way to "fix" that.

I understand Tart is used by many different organizations, with different use-cases. That's why, when I implemented this feature, it made it:

  1. A generic and flexible option, instead of a very specific global auto-mount override like in https://github.com/cirruslabs/tart/pull/731. A generic and flexible option means that we don't need to add yet another specific global flag for the next related use-case, and so on, which would complicate the command line flags of Tart. A flexible option allows different use-cases under a single option.

  2. A sub-option of `--dir instead of a global run option, so as to not pollute the option namespace of the run option, so that only those with use-cases related to mount options will need to care about the option.

Both of these choices were made exactly because Tart is used by many clients.

Now we end up with a feature request to have customizable tags for shared directories so you can mount things separately. Do I understand your new requirements correctly?

Share tags in Virtualization.frameworks are (sadly) not independent of auto-mounting. So either a share is automounted, or it has a custom tag (and need explicit mounting). We already allow users to pass a name for their shares, so we already have something to use for the tag identifier. What we then need is a way for users to choose if that name is used as a tag, disabling automount, or as the name of the folder within the automounted location in the guest. That's where an noauto option comes in.

fkorotkov commented 5 months ago

I was not able to find noauto mentions in Docker page you refered but I started wondering about option to pass a tag to the current [name:]path[:ro] specification. Something like [name:]path[:ro][@virtiofs-tag] or [name:]path[:ro][#virtiofs-tag]. This way something like:

tart run --dir="build:~/src/build@src" --dir="project:~/src/project@src" --dir="~/src/cache@cache" myvm

Will work and have multiple tags and named directories within a tag. What do you think of such feature and # vs @?

torarnv commented 5 months ago

https://docs.docker.com/storage/volumes/ describes (emphasis mine):

-v or --volume: Consists of three fields, separated by colon characters (:). The fields must be in the correct order, and the meaning of each field isn't immediately obvious. In the case of named volumes, the first field is the name of the volume, and is unique on a given host machine. For anonymous volumes, the first field is omitted. The second field is the path where the file or directory are mounted in the container. The third field is optional, and is a comma-separated list of options, such as ro. These options are discussed below.

So the choice of extending the last part of Tarts's --dir [name:]path[:ro] to --dir [name:]path[:comma,separated,list,of,options] came from that.

noauto came from https://en.wikipedia.org/wiki/Fstab#Options_common_to_all_filesystems, which should be familiar to users of Tart. Fstab also uses a common separated list of options, so this feature in Tart aligns with that.

I started wondering about option to pass a tag to the current [name:]path[:ro] specification. Something like [name:]path[:ro][@virtiofs-tag] or [name:]path[:ro][#virtiofs-tag]. This way something like:

tart run --dir="build:~/src/build@src" --dir="project:~/src/project@src" --dir="~/src/cache@cache" myvm

If I understand the proposal, the goal is to be able to mount_virtiofs src ~/src and have both build and project mounted? If so, that's an even more advanced use-case, ie. disabling auto-mounting, but still be able to group and mount them all under one tag (compared to this patch, which forces the user to name each tag, with one directory per tag).

I think that might be a bit confusing. The user is already passing a name, which is very similar to the tag. It would also mean that for a single non-auto share you'd need --dir="~/hello@foo", which seems a bit magical compared to --dir="foo:~/hello:noauto". The placement of @ or # after the host directory path is also a bit strange, considering the tag affects the guest side of things.

If we don't want an explicit noauto option, and instead want the mount behavior to be implicit based on specifying a tag or not, perhaps we should still use the options field (the last part), e.g --dir="foo:~/hello:tag=something", similar to Fstab's gid=5,mode=620?

fkorotkov commented 5 months ago

tag=something LGTM. What do you think of #733. I added some tests and grouping of by the tag looks a bit more concise to me.

torarnv commented 5 months ago

Thanks! Yes, https://github.com/cirruslabs/tart/pull/733 looks like it covers both the simple and the advanced use-cases, and with a non-intrusive option that doesn't pollute the main run-option namespace. Nice cleanup of the docs as well!

Thank you very much! 🥳

fkorotkov commented 5 months ago

Great! Glad we together came up with a solution better than our initial versions on their own.