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

Add back service to mount VirtualBox host directory into the guest. #14784

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

eiffel-fl
Copy link
Contributor

Hi.

In this PR, I fixed the problem described in #14465.
Sorry for the delay, I wanted to write a clean recipe but I think in a first time the goal is to solve the problem.
I will come back later with a proper way (i.e. compiling VBoxService from sources).

You can use this image to test it:

$ minikube start --driver=kvm --iso-path=file://$(pwd)/minikube-amd64-vbox.iso
...
$ minikube ssh                                                               
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ VBoxService 
VBoxService: error: VbglR3Init failed with rc=VERR_FILE_NOT_FOUND
$ cat /etc/systemd/system/multi-user.target.wants/vbox-mount-service.service
[Unit]
Description=VirtualBox Mount Service
ConditionVirtualization=oracle

[Service]
ExecStartPre=-/usr/sbin/modprobe vboxguest
ExecStartPre=-/usr/sbin/modprobe vboxsf

# Normally, VirtualBox only syncs every 20 minutes. This syncs on start, and
# forces an immediate sync if VM time is over 5 seconds off.
ExecStart=/usr/sbin/VBoxService -f --disable-automount --timesync-set-start --timesync-set-threshold 5000

[Install]
WantedBy=multi-user.target

Sadly, I was not able to test it as minikube does not boot using VirtualBox on my machine.

If you see any way to improve this PR, feel free to share (cc @vroetman).

Best regards.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 12, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @eiffel-fl. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 12, 2022
@eiffel-fl eiffel-fl force-pushed the francis/vbox-automount-quickfix branch from 33bc264 to 3301b6f Compare August 12, 2022 16:31
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 12, 2022
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@vroetman
Copy link

I built the iso, but it still does not auto mount the /hosthome directory.
When I run minikube start --driver=virtualbox --iso-url=https://storage.googleapis.com/minikube/iso/minikube-v1.25.2.iso it works, and I see the following in the journal from the minikube-automount.service

Aug 17 13:47:25 minikube minikube-automount[2138]: + try_mount_share hosthome
Aug 17 13:47:25 minikube minikube-automount[2138]: + dir=hosthome
Aug 17 13:47:25 minikube minikube-automount[2138]: + name=hosthome
Aug 17 13:47:25 minikube minikube-automount[2138]: + dir=/hosthome
Aug 17 13:47:25 minikube minikube-automount[2138]: + mkdir -p /hosthome
Aug 17 13:47:25 minikube minikube-automount[2138]: + mount -t vboxsf -o defaults,iocharset=utf8,uid=1000,gid=1000 hosthome /hosthome

When I run minikube start --driver=virtualbox --iso-url=file:///path/to/minikube/out/minikube-amd64.iso it says nothing about hosthome (which is in the share list in the VirtualBox VM), but the journal does say

Aug 17 13:54:56 minikube minikube-automount[457]: ++ VBoxControl --nologo sharedfolder list -automount
Aug 17 13:54:56 minikube minikube-automount[457]: /usr/sbin/minikube-automount: line 211: VBoxControl: command not found

in this iso, /usr/bin/VBoxControl is missing.

This iso has a vbox-mount-service.service file, but the v1.25.2 iso has a vboxservice.service file instead.

My guess is we are still missing the VBoxControl program. I don't know about the vboxservice.service and how important it is, but it seems it has to do with time sync and other VirtualBox specific things.

@eiffel-fl
Copy link
Contributor Author

in this iso, /usr/bin/VBoxControl is missing.

I did not add this binary as I do not know if it was needed, but it seems VBoxService calls it.
I will change the recipe and build another image, I will upload it before this evening.

This iso has a vbox-mount-service.service file, but the v1.25.2 iso has a vboxservice.service file instead.

This is intended, vbox-mount-service.service is the new name of the vboxservice.service in this PR.
I just made the name more specific to represent what it does.

@eiffel-fl eiffel-fl force-pushed the francis/vbox-automount-quickfix branch 2 times, most recently from 2f123ea to 523e2e7 Compare August 17, 2022 15:06
@eiffel-fl
Copy link
Contributor Author

I updated the recipe and built the corresponding image:

$ minikube start --driver=kvm --iso-path=file://$(pwd)/minikube-amd64-vbox.iso
...
$ minikube ssh                                                               
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ $ VBoxControl version
Oracle VM VirtualBox Guest Additions Command Line Management Interface Version 5.2.42
(C) 2008-2020 Oracle Corporation
All rights reserved.

5.2.42r137960

@vroetman
Copy link

vroetman commented Aug 18, 2022

I updated the recipe and built the corresponding [image]

I tried your image, but it would not boot for me, so I built the image again from your branch. It went further this time, but it fails with

Aug 18 16:22:05 minikube minikube-automount[315]: + try_mount_share hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + dir=hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + name=hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + dir=/hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + mkdir -p /hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + mount -t vboxsf -o defaults,iocharset=utf8,uid=1000,gid=1000 hosthome /hosthome
Aug 18 16:22:05 minikube minikube-automount[315]: + rmdir /hosthome
Aug 18 16:22:05 minikube kernel: vboxsf: Unknown parameter 'iocharset'

which is a little strange, given that the manual says iocharset should be an option. The mount statement works for me without the iocharset option. Maybe you should put mount.vboxsf back in again?

Also, the VBoxService file that your vbox-mount-service.service file calls does a lot more than mounting - it runs the time syncing from the VM to the host. In fact, you are calling it with --disable-automount so that service is not involved with mounting. I would recommend you rename it back to vboxservice.service.

@eiffel-fl
Copy link
Contributor Author

Maybe you should put mount.vboxsf back in again?

It is highly possible their mount.vboxsf handles more option than the mount which can deal with vboxsf present in the image.
I will push something tomorrow.

I would recommend you rename it back to vboxservice.service.

I do not understand it.
What does the VBoxService binary does?
It calls VBoxControl which in turns calls mount.vboxsf permitting you to access host directory from the guest, right?

@vroetman
Copy link

I do not understand it.
What does the VBoxService binary does?
It calls VBoxControl which in turns calls mount.vboxsf permitting you to access host directory from the guest, right?

I don't totally understand it, either, but if you run VBoxService --help you will see a bunch of options that will give you an idea what it does. It would be best to keep the other name with the service.

minikube-automount was calling VBoxControl apparently to get a list of shared folders that were tagged with automount. VBoxService was probably not involved with the automount.

@eiffel-fl
Copy link
Contributor Author

It would be best to keep the other name with the service.

I think you are right, it was a mistake to try to be smarter than the previous recipe.
I will add it back as a whole and once everything works again I will work on it to make it the "buildroot way".

@eiffel-fl eiffel-fl force-pushed the francis/vbox-automount-quickfix branch from 523e2e7 to acf7c48 Compare August 19, 2022 11:37
@eiffel-fl
Copy link
Contributor Author

eiffel-fl commented Aug 19, 2022

I removed the faulty option from the minikube-automount script, I would like to just stick to the mount.vboxsf which is already present.
Can you please try run it?
The new iso is available here.

@afbjorklund
Copy link
Collaborator

Looks ok

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2022
@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 56.4s    | 56.2s               |
| enable ingress | 27.1s    | 27.6s               |
+----------------+----------+---------------------+

Times for minikube start: 58.5s 54.0s 57.9s 56.0s 55.8s
Times for minikube (PR 14784) start: 57.2s 55.8s 55.2s 55.8s 56.9s

Times for minikube ingress: 25.1s 26.1s 25.6s 29.6s 29.1s
Times for minikube (PR 14784) ingress: 28.6s 29.1s 29.1s 25.6s 25.6s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 28.1s    | 28.2s               |
| enable ingress | 22.2s    | 22.9s               |
+----------------+----------+---------------------+

Times for minikube start: 27.6s 28.4s 27.3s 28.6s 28.7s
Times for minikube (PR 14784) start: 27.6s 28.5s 28.0s 29.6s 27.2s

Times for minikube ingress: 20.9s 22.5s 22.0s 23.5s 21.9s
Times for minikube (PR 14784) ingress: 21.5s 22.5s 23.5s 22.9s 24.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 23.5s    | 23.8s               |
| enable ingress | 29.5s    | 27.3s               |
+----------------+----------+---------------------+

Times for minikube start: 23.5s 23.3s 23.9s 23.4s 23.2s
Times for minikube (PR 14784) start: 23.5s 23.1s 23.8s 24.2s 24.6s

Times for minikube ingress: 27.5s 27.5s 27.4s 27.5s 37.5s
Times for minikube (PR 14784) ingress: 26.9s 27.4s 27.5s 27.0s 27.5s

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
KVM_Linux_containerd TestRunningBinaryUpgrade (gopogh) 0.63 (chart)
Docker_Windows TestAddons/Setup (gopogh) 52.29 (chart)
Docker_Windows TestCertExpiration (gopogh) 52.29 (chart)
Docker_Windows TestCertOptions (gopogh) 52.29 (chart)
Docker_Windows TestDockerFlags (gopogh) 52.29 (chart)
Docker_Windows TestErrorSpam/setup (gopogh) 52.29 (chart)
Docker_Windows TestForceSystemdEnv (gopogh) 52.29 (chart)
Docker_Windows TestForceSystemdFlag (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/CertSync (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/CpCmd (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/FileSync (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageBuild (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageListJson (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageListShort (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageListTable (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageListYaml (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageLoadDaemon (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageLoadFromFile (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageReloadDaemon (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageSaveDaemon (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageSaveToFile (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/ImageTagAndLoadDaemon (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/ImageCommands/Setup (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/NonActiveRuntimeDisabled (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/SSHCmd (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/UpdateContextCmd/no_changes (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/UpdateContextCmd/no_clusters (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/UpdateContextCmd/no_minikube_cluster (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/parallel/Version/components (gopogh) 52.29 (chart)
Docker_Windows TestFunctional/serial/CacheCmd/cache/cache_reload (gopogh) 52.29 (chart)
More tests... Continued...

Too many tests failed - See test logs for more details.

To see the flake rates of all tests by environment, click here.

@vroetman
Copy link

The new iso is available here.

I don't know why, but your iso images will not boot for me, but the ones I build work.

VBoxManage: error: Could not get the storage format of the medium '/home/vr/.minikube/machines/vboxtest/boot2docker.iso' (VERR_NOT_SUPPORTED)
VBoxManage: error: Details: code VBOX_E_IPRT_ERROR (0x80bb0005), component MediumWrap, interface IMedium, callee nsISupports
VBoxManage: error: Context: "OpenMedium(Bstr(pszFilenameOrUuid).raw(), enmDevType, enmAccessMode, fForceNewUuidOnOpen, pMedium.asOutParam())" at line 191 of file VBoxManageDisk.cpp
VBoxManage: error: Invalid UUID or filename "/home/vr/.minikube/machines/vboxtest/boot2docker.iso"

@afbjorklund , did @eiffel-fl's image work for you?

Anyway, I built it again, and it works, and I think dropping the iocharset option should be fine as the manual says UTF-8 is the default.

However, I still think you should change the name of vbox-mount-service.service back to vboxservice.service as that service is not doing anything related to mounting—the command line is /usr/sbin/VBoxService -f --disable-automount --timesync-set-start --timesync-set-threshold 5000. Then I will build and test it one more time and I think we are good to go.

@eiffel-fl eiffel-fl force-pushed the francis/vbox-automount-quickfix branch from acf7c48 to 82665a5 Compare August 22, 2022 17:28
@eiffel-fl
Copy link
Contributor Author

eiffel-fl commented Aug 22, 2022

I rename the service to its previous name:

$ minikube ssh
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ cat /etc/systemd/system/multi-user.target.wants/vboxservice.service       
[Unit]
Description=VirtualBox Mount Service
ConditionVirtualization=oracle

[Service]
ExecStartPre=-/usr/sbin/modprobe vboxguest
ExecStartPre=-/usr/sbin/modprobe vboxsf

# Normally, VirtualBox only syncs every 20 minutes. This syncs on start, and
# forces an immediate sync if VM time is over 5 seconds off.
ExecStart=/usr/sbin/VBoxService -f --disable-automount --timesync-set-start --timesync-set-threshold 5000

[Install]
WantedBy=multi-user.target

I don't know why, but your iso images will not boot for me, but the ones I build work.

Sadly, I do not why too...
I nonetheless uploaded the new ISO here.

Fixes: 770d41f ("Use kernel 5.10 for minikube.iso.")
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
@eiffel-fl eiffel-fl force-pushed the francis/vbox-automount-quickfix branch from 82665a5 to aa63b4f Compare August 23, 2022 13:55
@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 55.7s    | 56.1s               |
| enable ingress | 29.0s    | 28.7s               |
+----------------+----------+---------------------+

Times for minikube start: 56.6s 54.6s 55.6s 56.2s 55.5s
Times for minikube (PR 14784) start: 56.8s 57.1s 56.4s 56.8s 53.5s

Times for minikube ingress: 29.2s 28.7s 29.7s 26.2s 31.2s
Times for minikube (PR 14784) ingress: 30.2s 27.7s 28.3s 28.2s 29.1s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 28.9s    | 29.3s               |
| enable ingress | 23.5s    | 22.4s               |
+----------------+----------+---------------------+

Times for minikube start: 28.4s 28.6s 29.8s 28.8s 28.7s
Times for minikube (PR 14784) start: 29.8s 28.6s 28.9s 29.4s 29.8s

Times for minikube ingress: 22.5s 24.0s 22.0s 26.0s 23.0s
Times for minikube (PR 14784) ingress: 22.0s 21.5s 23.0s 23.5s 22.0s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 14784) |
+----------------+----------+---------------------+
| minikube start | 24.7s    | 24.9s               |
| enable ingress | 27.0s    | 27.0s               |
+----------------+----------+---------------------+

Times for minikube start: 25.0s 24.3s 24.3s 25.0s 24.8s
Times for minikube (PR 14784) start: 25.2s 25.2s 24.7s 24.6s 24.8s

Times for minikube ingress: 27.0s 27.0s 27.0s 27.0s 27.0s
Times for minikube (PR 14784) ingress: 27.0s 27.0s 27.0s 27.0s 27.0s

@minikube-pr-bot
Copy link

These are the flake rates of all failed tests.

Environment Failed Tests Flake Rate (%)
Hyper-V_Windows TestNoKubernetes/serial/StartWithK8s (gopogh) 35.00 (chart)
Docker_Windows TestNetworkPlugins/group/enable-default-cni/DNS (gopogh) 37.14 (chart)
KVM_Linux TestMultiNode/serial/RestartMultiNode (gopogh) 52.47 (chart)
Docker_Linux_containerd TestNetworkPlugins/group/calico/Start (gopogh) 75.42 (chart)
Docker_Linux TestNetworkPlugins/group/calico/Start (gopogh) 75.50 (chart)
Docker_Linux_containerd TestNetworkPlugins/group/enable-default-cni/DNS (gopogh) 75.59 (chart)
Docker_Linux TestNetworkPlugins/group/bridge/DNS (gopogh) 78.81 (chart)
Docker_Linux_containerd TestNetworkPlugins/group/bridge/DNS (gopogh) 79.39 (chart)
Docker_Linux TestNetworkPlugins/group/enable-default-cni/DNS (gopogh) 79.47 (chart)
Docker_Linux TestNetworkPlugins/group/kubenet/DNS (gopogh) 82.78 (chart)
Docker_Linux TestNetworkPlugins/group/false/DNS (gopogh) 84.11 (chart)
Docker_Windows TestNetworkPlugins/group/bridge/DNS (gopogh) 87.14 (chart)
Docker_Windows TestNetworkPlugins/group/calico/Start (gopogh) 96.08 (chart)
Docker_Linux_containerd TestKubernetesUpgrade (gopogh) 100.00 (chart)
Docker_Windows TestFunctional/parallel/ServiceCmd (gopogh) 100.00 (chart)
Docker_Windows TestNetworkPlugins/group/cilium/Start (gopogh) 100.00 (chart)
Docker_Windows TestNetworkPlugins/group/kubenet/HairPin (gopogh) 100.00 (chart)
Hyper-V_Windows TestMultiNode/serial/PingHostFrom2Pods (gopogh) 100.00 (chart)
Hyper-V_Windows TestMultiNode/serial/RestartKeepsNodes (gopogh) 100.00 (chart)
Hyper-V_Windows TestNetworkPlugins/group/kubenet/HairPin (gopogh) 100.00 (chart)
KVM_Linux_containerd TestStoppedBinaryUpgrade/Upgrade (gopogh) 100.00 (chart)
KVM_Linux TestNetworkPlugins/group/kubenet/HairPin (gopogh) 100.00 (chart)

To see the flake rates of all tests by environment, click here.

@vroetman
Copy link

Thank you @eiffel-fl, this one (aa63b4f) seems to be working great for me, and I think it's good to go. I really appreciate the time you spent on it.

Is there anyone else who has tested it, especially on a mac? @adam-olema? @nwbt?

@eiffel-fl
Copy link
Contributor Author

I really appreciate the time you spent on it.

I broke the thing, so sort of normal I try to fix it!

Is there anyone else who has tested it

If anyone wants to test, you can use this image.
It does not reflect the latest changes (i.e. the change of service name) but it should have all the features of the last commit.

@vroetman
Copy link

What do we need to get this reviewed and merged in?

@eiffel-fl
Copy link
Contributor Author

Let's try something:
/ok-to-build-iso
If that works, people will have an ISO available, so virtual box users can use it while waiting for this PR to be merged.

@eiffel-fl
Copy link
Contributor Author

Pouin pouin pouin pouin 😅😅😅.
It seems only maintainers can run command like this.

@nickaguilarh
Copy link

Hi,
I tested it on macOS 12.5.1 with the latest image provided.

minikube v1.26.1 on Darwin 12.5.1

00:00:00.179597 SharedFolders host service: Adding host mapping
00:00:00.179611     Host path '/Volumes/RAM', map name 'RAM', writable, automount=true, automntpnt=/RAM, create_symlinks=false, missing=false
00:00:00.179809 SharedFolders host service: Adding host mapping
00:00:00.179820     Host path '/Users', map name 'Users', writable, automount=true, automntpnt=, create_symlinks=true, missing=false
~ > minikube ssh 'ls -la /'
total 8
drwxr-xr-x  21 root   root    540 Sep  7 00:03 .
drwxr-xr-x  21 root   root    540 Sep  7 00:03 ..
drwxrwxr-x   1 docker docker  204 Sep  6 22:44 RAM
drwxr-xr-x   1 docker docker  160 Sep  3 04:49 Users
~ > minikube ssh 'ls -la /Users'       
total 0
drwxr-xr-x  1 docker docker  160 Sep  3 04:49 .
drwxr-xr-x 21 root   root    540 Sep  7 00:03 ..
-rw-r--r--  1 docker docker    0 Aug 11 06:44 .localized
drwxrwxrwx  1 docker docker   96 Aug 11 06:44 Shared
drwxr-x---  1 docker docker 1440 Sep  7 00:08 nick

It seems to be fully working.

@vroetman
Copy link

vroetman commented Sep 8, 2022

@eiffel-fl, I was looking at the bot response at the beginning of the PR, and it says

Once this PR has been reviewed and has the lgtm label, please assign afbjorklund for approval by writing /assign @afbjorklund in a comment

But I think you need to be a member of the Kubernetes organization to use /lgtm to add the label, or perhaps one of the Reviewers.

There is also the Testing and Merge Workflow documentation.

@eiffel-fl
Copy link
Contributor Author

But I think you need to be a member of the Kubernetes organization to use /lgtm to add the label.

This is my understanding too that only members of this organization can use /slash-command.
But this PR will finish by being merged, let's just wait a bit and meanwhile we can use this image.

@vroetman
Copy link

vroetman commented Sep 8, 2022

But this PR will finish by being merged, let's just wait a bit

I am not experienced with PRs in this project, but if I am reading it right, perhaps it won't get reviewed or merged without the lgtm and the assign to @afbjorklund ?

@mm0
Copy link

mm0 commented Sep 24, 2022

Maybe you should put mount.vboxsf back in again?

It is highly possible their mount.vboxsf handles more option than the mount which can deal with vboxsf present in the image. I will push something tomorrow.

I would recommend you rename it back to vboxservice.service.

I do not understand it. What does the VBoxService binary does? It calls VBoxControl which in turns calls mount.vboxsf permitting you to access host directory from the guest, right?

FYI. What it does is provide the ability to sync VM clock with the host which is much needed for certificate validation considering systemd-timesyncd isn't enabled. Here is a better explanation. VirtualBox is largely unusable due to the previous changes.

@eiffel-fl
Copy link
Contributor Author

FYI. What it does is provide the ability to sync VM clock with the host which is much needed for certificate validation considering systemd-timesyncd isn't enabled. Here is a better explanation. VirtualBox is largely unusable due to the previous changes.

Thank you a lot for this links.
Nonetheless, even though I understand not having this service leads to not be able to sync minikube VM time with host, I am not really sure I understand why it is a problem in practice (even though I understand minikube with VBox is no more usable).
Indeed, with qemu I can have a difference in time, and there is no big troubles:

francis@pwmachine:~/Codes/kinvolk$ date
lun. 03 oct. 2022 11:01:04 CEST
francis@pwmachine:~/Codes/kinvolk$ minikube ssh -- date
Mon Oct  3 09:01:10 UTC 2022

@mm0
Copy link

mm0 commented Oct 3, 2022

FYI. What it does is provide the ability to sync VM clock with the host which is much needed for certificate validation considering systemd-timesyncd isn't enabled. Here is a better explanation. VirtualBox is largely unusable due to the previous changes.

Thank you a lot for this links. Nonetheless, even though I understand not having this service leads to not be able to sync minikube VM time with host, I am not really sure I understand why it is a problem in practice (even though I understand minikube with VBox is no more usable). Indeed, with qemu I can have a difference in time, and there is no big troubles:

francis@pwmachine:~/Codes/kinvolk$ date
lun. 03 oct. 2022 11:01:04 CEST
francis@pwmachine:~/Codes/kinvolk$ minikube ssh -- date
Mon Oct  3 09:01:10 UTC 2022

I believe the main issue is that Minikube no longer has the virtualbox-guest-utils package installed that provide the utils to manage timesync + automount.

The problem that timesync solves is that if the clock in the guest VM drifts beyond a certain number of seconds, it will then synchronize the clock with the host. Alternatively, something like NTP can be used in the guest VM to not have to rely on the host to synchronize the clock. Without either solution in place, your guest VM clock may eventually end up being minutes, hours or days behind causing SSL verification to fail among other possible issues.

I have verified that the vboxguest drivers are loaded fine, but I believe the lack of the utils is preventing Minikube from being setup in a way to use these vboxguest features.

$ modinfo vboxguest
filename:       /lib/modules/5.10.57/kernel/drivers/virt/vboxguest/vboxguest.ko
license:        GPL
description:    Oracle VM VirtualBox Guest Additions for Linux Module
author:         Oracle Corporation
alias:          pci:v000080EEd0000CAFEsv00000000sd00000000bc*sc*i*
depends:        
retpoline:      Y
intree:         Y
name:           vboxguest
vermagic:       5.10.57 SMP mod_unload modversions 

@eiffel-fl
Copy link
Contributor Author

hours or days behind causing SSL verification to fail among other possible issues.

It makes totally sense, thank you for the precision.

I have verified that the vboxguest drivers are loaded fine, but I believe the lack of the utils is preventing Minikube from being setup in a way to use these vboxguest features.

Normally, the driver is OK because it is now in upstream kernel, this is why I removed everything about vbox in my previous PR.
Sadly, I also removed the surrounding tools which are really needed 😅.

@medyagh
Copy link
Member

medyagh commented Oct 12, 2022

/ok-to-build-iso

@@ -179,7 +179,7 @@ mkdir -p /var/lib/boot2docker/etc/
# VirtualBox Host Mounting
# - this will bail quickly and gracefully if we're not in VBox
if modprobe vboxguest &> /dev/null && modprobe vboxsf &> /dev/null; then
mountOptions='defaults,iocharset=utf8'
Copy link
Member

Choose a reason for hiding this comment

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

@eiffel-fl
I am curious why we remove iocharset=utf8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this because it not recognized by the kernel:

$ minikube start --driver=kvm --iso-url=file://$(pwd)/minikube-v1.27.0-1665598505-14784-amd64.iso
😄  minikube v1.26.1 sur Ubuntu 20.04
...
$ minikube ssh
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ modprobe vboxguest
$ sudo mkdir /bar
$ sudo mount -t vboxsf -o defaults,iocharset=utf8 /mnt /bar
mount: /bar: wrong fs type, bad option, bad superblock on /mnt, missing codepage or helper program, or other error.
$ sudo dmesg | tail
[ 1009.804852] docker0: port 1(veth2bfee67) entered blocking state
[ 1009.804854] docker0: port 1(veth2bfee67) entered disabled state
[ 1009.804912] device veth2bfee67 entered promiscuous mode
[ 1010.449087] eth0: renamed from vethcce140b
[ 1010.454695] docker0: port 1(veth2bfee67) entered blocking state
[ 1010.454697] docker0: port 1(veth2bfee67) entered forwarding state
[ 1016.802729] kauditd_printk_skb: 30 callbacks suppressed
[ 1016.802730] audit: type=1325 audit(1665651863.996:191): table=filter family=2 entries=50 op=xt_replace pid=10811 subj=kernel comm="iptables-restor"
[ 1016.804055] audit: type=1325 audit(1665651863.997:192): table=nat family=2 entries=70 op=xt_replace pid=10811 subj=kernel comm="iptables-restor"
[ 1114.343757] vboxsf: Unknown parameter 'iocharset'

Also, I suspect utf8 is the default regarding the kernel sources:

$ pwd                                                                                                    
.../linux/fs/vboxsf
$ git grep 'utf8'                                                                                        
dir.c:122:                      end = &info->name.string.utf8[info->name.size];
dir.c:129:              end = &info->name.string.utf8[info->name.size];
dir.c:150:                                          info->name.string.utf8,
dir.c:162:              return dir_emit(ctx, info->name.string.utf8, info->name.length,
dir.c:398:      memcpy(ssymname->string.utf8, symname, symname_size);
shfl_hostintf.h:71:             u8 utf8[2];
super.c:146:    /* Load nls if not utf8 */
super.c:148:    if (strcmp(nls_name, "utf8") != 0) {
super.c:182:    strlcpy(folder_name->string.utf8, fc->source, size);
super.c:193:    root_path.string.utf8[0] = '/';
super.c:194:    root_path.string.utf8[1] = 0;
super.c:361:    err = vboxsf_set_utf8();
super.c:363:            vbg_err("vboxsf_setutf8 error %d\n", err);
utils.c:372:            out = shfl_path->string.utf8;
utils.c:385:                    nb = utf32_to_utf8(uni, out, out_len);
utils.c:395:            shfl_path->length = out - shfl_path->string.utf8;
utils.c:408:            memmove(shfl_path->string.utf8, path, path_len);
utils.c:409:            shfl_path->string.utf8[path_len] = 0;
utils.c:418:              const unsigned char *utf8_name, size_t utf8_len)
utils.c:426:    in = utf8_name;
utils.c:427:    in_bound_len = utf8_len;
utils.c:438:            nb = utf8_to_utf32(in, in_bound_len, &uni);
vboxsf_wrappers.c:363:int vboxsf_set_utf8(void)
vfsmod.h:99:              const unsigned char *utf8_name, size_t utf8_len);
vfsmod.h:134:int vboxsf_set_utf8(void);

@@ -49,8 +49,6 @@ BR2_PACKAGE_ACPID=y

# Minikube

config BR2_PACKAGE_VBOX_GUEST=y
Copy link
Member

Choose a reason for hiding this comment

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

@eiffel-fl
why do we need to remove this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a leftover of my previous contribution where I, too quickly and violently, removed it.
With this PR, I added back what I deleted but with another name which I think is more descriptive.

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

@eiffel-fl thank you for your patience and I left some comments but overall the PR looks good. (I just invoked the ISO build) so we can see the test results with the new ISO

@minikube-bot
Copy link
Collaborator

Hi @eiffel-fl, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further.

@eiffel-fl
Copy link
Contributor Author

eiffel-fl commented Oct 14, 2022

Hi.

I was finally able to use minikube with virtualbox driver, with the latest image, my home is automatically mounted under /hosthome:

$ minikube start --driver=virtualbox --iso-url=file://$(pwd)/minikube-v1.27.0-1665598505-14784-amd64.iso
😄  minikube v1.26.1 sur Ubuntu 20.04
✨  Utilisation du pilote virtualbox basé sur la configuration de l'utilisateur
👍  Démarrage du noeud de plan de contrôle minikube dans le cluster minikube
🔥  Création de VM virtualbox (CPUs=2, Mémoire=6000MB, Disque=20000MB)...
🐳  Préparation de Kubernetes v1.24.3 sur Docker 20.10.18...
...
$ minikube ssh                                                                                          
                         _             _            
            _         _ ( )           ( )           
  ___ ___  (_)  ___  (_)| |/')  _   _ | |_      __  
/' _ ` _ `\| |/' _ `\| || , <  ( ) ( )| '_`\  /'__`\
| ( ) ( ) || || ( ) || || |\`\ | (_) || |_) )(  ___/
(_) (_) (_)(_)(_) (_)(_)(_) (_)`\___/'(_,__/'`\____)

$ ls -l /hosthome/francis
...
drwxr-xr-x 1 docker docker     4096 May 14  2021 'Mod'$'\303\250''les'
drwxr-xr-x 1 docker docker     4096 May 14  2021  Musique
drwxr-xr-x 1 docker docker     4096 May 14  2021  Public
drwxrwxr-x 1 docker docker     4096 Oct 14 22:10 'T'$'\303\251''l'$'\303\251''chargements'
drwxr-xr-x 1 docker docker     4096 Sep 15 09:06 'Vid'$'\303\251''os'
...
# Use tab to complete M to Modèles
$ cd /hosthome/francis/Modèles
$ pwd
/hosthome/francis/Modèles

There are problems showing some characters (e.g. é, è, and other French accented letters), nonetheless I do not think these problems are related to this PR or virtualbox, as I can reproduce with kvm.

As a conclusion, I think the built image fixed the problem I added previously (still sorry for the disturbance).

Best regards.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eiffel-fl, medyagh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2022
@medyagh medyagh merged commit a5a5dce into kubernetes:master Oct 17, 2022
@eiffel-fl eiffel-fl deleted the francis/vbox-automount-quickfix branch October 18, 2022 07:56
@vroetman
Copy link

Fixes #14465

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants