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

guests/linux: remove set -e from virtualbox mount #7683

Closed

Conversation

chrisroberts
Copy link
Member

Addresses issues described in #7616 by removing the set -e inclusion to allow execution of subsequent commands after a failure. To reproduce the issue, used the following Vagrantfile:

Vagrant.configure('2') do |config|
  config.vm.box = 'bento/ubuntu-16.04'
  config.vm.synced_folder '.', '/vagrant'
end

Booted the VM, ssh'd in, modified the vagrant group name to vagranti. Then ran a reload:

$ vagrant reload

==> default: Attempting graceful shutdown of VM...

Bringing machine 'default' up with 'virtualbox' provider...
==> default: Checking if box 'bento/ubuntu-16.04' is up to date...
==> default: Clearing any previously set forwarded ports...
==> default: Clearing any previously set network interfaces...
==> default: Preparing network interfaces based on configuration...
    default: Adapter 1: nat
==> default: Forwarding ports...
    default: 22 (guest) => 2222 (host) (adapter 1)
==> default: Booting VM...
==> default: Waiting for machine to boot. This may take a few minutes...
    default: SSH address: 127.0.0.1:2222
    default: SSH username: vagrant
    default: SSH auth method: private key
==> default: Machine booted and ready!
==> default: Checking for guest additions in VM...
==> default: Mounting shared folders...
    default: /vagrant => /home/spox/hashicorp/vagrant-scratch
Vagrant was unable to mount VirtualBox shared folders. This is usually
because the filesystem "vboxsf" is not available. This filesystem is
made available via the VirtualBox Guest Additions and kernel module.
Please verify that these guest additions are properly installed in the
guest. This is not a bug in Vagrant and is usually caused by a faulty
Vagrant box. For context, the command attempted was:

set -e
mount -t vboxsf -o uid=`id -u vagrant`,gid=`getent group vagrant | cut -d: -f3` vagrant /vagrant
mount -t vboxsf -o uid=`id -u vagrant`,gid=`id -g vagrant` vagrant /vagrant

The error output from the command was:

No such file or directory

Removing the set -e and reloading:

$ vagrant reload

==> default: Attempting graceful shutdown of VM...

Bringing machine 'default' up with 'virtualbox' provider...
==> default: Checking if box 'bento/ubuntu-16.04' is up to date...
==> default: Clearing any previously set forwarded ports...
==> default: Clearing any previously set network interfaces...
==> default: Preparing network interfaces based on configuration...
    default: Adapter 1: nat
==> default: Forwarding ports...
    default: 22 (guest) => 2222 (host) (adapter 1)
==> default: Booting VM...
==> default: Waiting for machine to boot. This may take a few minutes...
    default: SSH address: 127.0.0.1:2222
    default: SSH username: vagrant
    default: SSH auth method: private key
==> default: Machine booted and ready!
==> default: Checking for guest additions in VM...
==> default: Mounting shared folders...
    default: /vagrant => /home/spox/hashicorp/vagrant-scratch
==> default: Machine already provisioned. Run `vagrant provision` or use the `--provision`
==> default: flag to force provisioning. Provisioners marked to run always will still run.

@mitchellh
Copy link
Contributor

Do we know why "set -e" was originally there? Just checking that you did some background checks on that.

Otherwise LGTM.

@sethvargo
Copy link
Contributor

IIRC set -e was required to make the retries happen. Can we add an || true to the commands that are permitted to fail instead?

@chrisroberts
Copy link
Member Author

That makes sense. Instead of the || true how about just joining the two mount commands with || ? Will ensure the second mount will only be executed as required and provide the desired exit code.

Removes the `set -e` and ORs the commands. This allows for graceful
command fallback while still properly supporting the retryable functionality.
@sethvargo
Copy link
Contributor

@chrisroberts there are a few issues that this fixes. Do you want to cross-link them so they close when we merge?

@chrisroberts
Copy link
Member Author

@sethvargo yeah, I will. I've also got another PR related to this that I'm getting ready to push out.

@chrisroberts
Copy link
Member Author

closing this in favor of #7720

@ghost
Copy link

ghost commented Apr 3, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@hashicorp hashicorp locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants