Implement disable default mounts via command line #23254
Conversation
|
I don't think the |
|
@baude PTAL |
|
|
|
That's mostly my concern - feels like poor user interface |
|
I'm open to reimplement it, when the final decision is agreed within the team. |
|
@Luap99 @ashley-cui @l0rd Thoughts on this one? I'm pretty firmly in favor of a separate flag but could be convinced otherwise |
|
Since I can find no precedence for this, I guess I would go with new option. But why not just edit containers.conf and remove the volumes? |
|
I'm on team edit containers.conf, but okay with a new flag. |
|
@arixmkii can you please provide more details on the use case? What's the use case that requires to disable the defaults volumes? And why editing the conf file doesn't work in this case? |
|
Editing config would do the trick. I just expected there to be API to achieve the same effect, but there was no. Right now there is no sure way to init machine with mounts disabled w/o checking config in advanced. Example - running e2e machine tests would always use mounts in all tests with the default configuration and there is no way to force this behavior in tests only. This actually could be considered as niche or minor issue. I created original issue about this and as this was not strictly rejected I submitted this follow up. If this is considered as not needed both this PR and the issue can be closed with "won't implement." In this case I can resubmit only a change for checking if mounts are requested before checking virtiofsd dependency. @l0rd my scenario is the system, where I reinstall (often with prune everything uninstall or sometimes restoring that system from snapshot) Podman releases rebuilt from main head often and I can't have mounts operational. It's very specific and can be mitigated with some sort of automation on my side. |
|
I am fine with |
pkg/machine/e2e/config_test.go
Outdated
| if len(cmdArgs) >= 2 && cmdArgs[0] == "machine" && cmdArgs[1] == "init" && !slices.Contains(cmdArgs, "-v") { | ||
| tailArgs := append([]string{"-v", ""}, cmdArgs[2:]...) | ||
| cmdArgs = append([]string{"machine", "init"}, tailArgs...) | ||
| } |
There was a problem hiding this comment.
This does not look right. First of all this argument manipulation happens in the wrong place. It would need to happen for buildCmd() in the machineInit type.
Second I really really think tests must use the default volumes, it is critical that these mounts work. Most tests may not use them but it is still important that our code works and does not contain flakes etc.. (#23118) And yes this one test there actually depend on the default mounts.
There was a problem hiding this comment.
This part is indeed dirty and hacky. I need to get more familiar with the testing framework setup to improve this.
I noticed numerous tests, which were designed specifically for testing volume mounts, so, I didn't expect there will be some test, which implicitly test default ones. Probably the behavior from that test could be adjusted by loading the config as an app would do and manually converting it into chain of -v "/some:/some" -v "/other:/other", but I'm not sure if it worth doing or if it is better to keep the things as is for testing as they have proved to be working fine already.
Alternatively I could keep everything as is and try to add dedicated tests for the new API option checking that it will create no mounts. And if at later point the adjustment of default testing setting would be needed it can be done in a separate PR. WDYT?
There was a problem hiding this comment.
I find it important that we test defaults. Adding explicit machine mounts means we are no longer testing how most users use it.
There was a problem hiding this comment.
Removed this workaround. Also, added a dedicated test for this command line.
Thank you @arixmkii. Given the rarity of the use case we should avoid a new option. But this PR fixes the issue with an empty list of volume ( I have tested it on macOS with libkrun and it worked well 👍 |
|
macOS machine test failed is legitimate. It seems that macOS mounts some additional paths for system purposes (I have no macOS hardware to check). I will disable new test on macOS. |
Signed-off-by: Arthur Sengileyev <arthur.sengileyev@gmail.com>
|
Disabled test for both applehv and libkrun. Would appreciate if someone could share output of |
|
@arixmkii Default macos mounts are defined here https://github.com/containers/common/blob/main/pkg/config/default_darwin.go#L16 |
|
Ah I think the thing is that we mount rosetta payload via virtiofs on applehv thus you see the extra mount there. |
|
@Luap99 Thank you! But, unfortunately, I don't need default ones, I need actual. I'm guessing that Rossetta binfmt is being mount as virtiofs from the host, but I can't prove it w/o testing. In this case there will be a virtiofs mount on macOS even, when no user mounts are set for VM.My current believe is that this is what lead to a test failure on macOS. Update: I wrote this message before page refreshed. So, it is duplicate of the comment above. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arixmkii, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Fixes #22364
This PR adds specific handling of the command line arguments
-v ""- special handling for empty string.-vbeing present suppresses defaults from the config, so, it is sufficient to implement it as no-op to achieve empty mounts. This happens as early as possible (before any machine specific logic would be called).Additional change is to allow using QEMU w/o virtiofsd binary if there are no mounts needed and the binary will not be used to run additional daemon.
And the final change is to disable default mounts for e2e tests, which are not supposed to have any mounts. Implementation is a bit hacky it checks if the command is
machine initand then traverses args to see if there are mounts requested, if no mounts in the list it will inject empty mount arguments and continue.Does this PR introduce a user-facing change?