Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

driver.provider.name Overrided by VAGRANT_DEFAULT_PROVIDER #174

Closed

Conversation

hswong3i
Copy link
Collaborator

@hswong3i hswong3i commented May 5, 2022

In case environment variable VAGRANT_DEFAULT_PROVIDER is defined (see
https://www.vagrantup.com/docs/other/environmental-variables#vagrant_default_provider),
our driver.provider.name will have no effect and always start with
above default provider as specifiied.

With python-vagrant up() (see
https://github.com/pycontribs/python-vagrant/blob/main/src/vagrant/__init__.py#L310-L318)
we could specify the target provider with provider option, where our
current wrapper only provide the provision option.

Signed-off-by: Wong Hoi Sing Edison [email protected]

hswong3i added a commit to alvistack/vagrant-gitlab-runner that referenced this pull request May 5, 2022
@apatard
Copy link
Member

apatard commented May 5, 2022

Several comments:

  • this will have to wait for the CI to be working again (blocked by a bug in mocule)
  • a small test case would be nice
  • I already thought a similar change several times in the past but it needs more modifications as it'll break the molecule-vagrant CI. Forcing the provider like this will now produce an error in cases where the provider is not found and vagrant using a different provider. I agree it may be bad idea to not error out if vagrant is not using the asked provider but in a bunch of cases, things will just work not matter which provider is used. And I don't like breaking people (working) CI...

[ btw, if we really adopt this change, I guess it'll meant doing a major release of molecule-vagrant to respect semver. I'll have to check that. ]

@hswong3i
Copy link
Collaborator Author

hswong3i commented May 5, 2022

Currently one of the failed test case is:

  1. In molecule.yml specify drive.provider.name: libvirt
  2. But the testing machine is running under OSX, which DON'T have vagrant-libvirt plugin installed
  3. So why previous test passed? Because we gracefully incorrectly running it with virtualbox...

For semver, I guess we could provide a default value for this, e.g. virtualbox as vagrant default provider. Therefore existing user without specifying drive.provider.name will fallback as default as vagrant. Most likely don't break semver?

@hswong3i
Copy link
Collaborator Author

hswong3i commented May 5, 2022

Some example why specify drive.provider.name is important: because most provider_raw_config_args are not portable. Yes, ideally we don't need to have such indeed customization because vagrant already abstract for us; but at least for my case, such virtualization-platform-specific customization is important.

e.g. my molecule.yml for libvirt:

driver:
  name: vagrant
  provider:
    name: libvirt
  provision: true
platforms:
  - name: ${MOLECULE_INSTANCE_NAME}-1
    box: alvistack/ubuntu-22.04
    cpu: 2
    memory: 8192
    provider_raw_config_args:
      - "cpu_mode = 'host-passthrough'"
      - "disk_bus = 'virtio'"
      - "nic_model_type = 'virtio'"
      - "video_type = 'virtio'"
      - "disk_driver :cache => 'writeback'"
      - "storage :file, bus: 'virtio', cache: 'writeback'"

In case of virtualbox:

driver:
  name: vagrant
  provider:
    name: virtualbox
  provision: true
platforms:
  - name: ${MOLECULE_INSTANCE_NAME}-1
    box: alvistack/ubuntu-22.04
    cpu: 2
    memory: 8192
    provider_raw_config_args:
      - "customize ['modifyvm', :id, '--cpu-profile', 'host']"
      - "customize ['modifyvm', :id, '--nested-hw-virt', 'on']"

@apatard
Copy link
Member

apatard commented May 5, 2022

Currently one of the failed test case is:

1. In molecule.yml specify `drive.provider.name: libvirt`

2. But the testing machine is running under OSX, which DON'T have vagrant-libvirt plugin installed

3. So why previous test passed? Because we ~gracefully~ incorrectly running it with virtualbox...

The test have been written are basic so they should work with libvirt and virtualbox. The provider used in reality doesn't matter
[ iirc there's only one test passing with vbox by "luck" and this test is fine with libvirt ]. If you apply your change, the testsuite will now fail badly.
Despite the fact that for unknown reason zuul tests are not run, the same test suite has to run with virtualbox (gha) and libvirt (zuul). The testsuite will need more hacks in order to switch the provider "on the fly" while now it's kind of just working. Duplicating scenarios would be a bad idea.

For semver, I guess we could provide a default value for this, e.g. virtualbox as vagrant default provider. Therefore existing user without specifying drive.provider.name will fallback as default as vagrant. Most likely don't break semver?

it's not a matter of default value since the module is already setting the default provider to virtualbox. The problem is that people doing things like what's done in the testsuite (and are happy with that) will now get failures. So this new behaviour is not compatible with previous one. Moreover, I'm not sure that there's a reliable way to know if vagrant using a different provider is fine or not for a given scenario. Think also about this very simple molecule.yml case:

---
dependency:
  name: galaxy
driver:
  name: vagrant
platforms:
  - name: instance
provisioner:
  name: ansible

With your change, it'll fail on systems with a different provider than virtualbox. Without it, things will just work.

I guess that a better solution may be to add a new option (for instance "force_provider") to the vagrant module and set it according to a molecule_yml.driver.provider.force option. If it's set, pass the provider to vagrant the same way you did. And in this case, it'll even remain compatible with current behaviour.

At last but not least, please, provide a test case if possible (I guess it should be enought to test something like getting an error if the provider in molecule_yml.driver.provider.name set to provider not installed is correctly giving an error).

apatard added a commit to apatard/molecule-vagrant that referenced this pull request Sep 2, 2022
Currently, vagrant is still choosing the provider to use on its own
liking. While it's fine for cases like our CI or people who are fine
with default VM configurations, it's not always the case.

See:
- ansible-community#174
- https://github.com/ansible-community/molecule-vagrant/issues/181

So, add a new configuration setting to call vagrant up with
--provider, in order to force the provider to use and lead to an
error if it's not usable. The default value is to keep current
behaviour to not break things.

Signed-off-by: Arnaud Patard <[email protected]>
apatard added a commit to apatard/molecule-vagrant that referenced this pull request Sep 6, 2022
Currently, vagrant is still choosing the provider to use on its own
liking. While it's fine for cases like our CI or people who are fine
with default VM configurations, it's not always the case.

See:
- ansible-community#174
- https://github.com/ansible-community/molecule-vagrant/issues/181

So, add a new configuration setting to call vagrant up with
--provider, in order to force the provider to use and lead to an
error if it's not usable. The default value is to keep current
behaviour to not break things.

Signed-off-by: Arnaud Patard <[email protected]>
@apatard
Copy link
Member

apatard commented Sep 6, 2022

I've implemented what I suggested in #187. Would it solve your issue ?

@hswong3i
Copy link
Collaborator Author

hswong3i commented Sep 7, 2022

I've implemented what I suggested in #187. Would it solve your issue ?

It could solve the issue technically, but I couldn't agree from KISS design principle...

apatard added a commit to apatard/molecule-vagrant that referenced this pull request Sep 23, 2022
Currently, vagrant is still choosing the provider to use on its own
liking. This may cause troubles like VM memory/cpus or provider
options ignored if the provider use is not the one specified.

See:

- ansible-community#174
- https://github.com/ansible-community/molecule-vagrant/issues/181

So, pass --provider when the provider is set in the molecule.yml.
If the test doesn't care about the provider, it should remove the
provider name setting in the molecule.yml.

Note: While this may break some setup, I believe this will prevent
new reports/issues like the one previously noted. In case one
molecule.yml file for multiple providers is wanted, use the new
providers_options, as done in our CI scenario.

Signed-off-by: Arnaud Patard <[email protected]>
apatard added a commit to apatard/molecule-vagrant that referenced this pull request Oct 18, 2022
Currently, vagrant is still choosing the provider to use on its own
liking. This may cause troubles like VM memory/cpus or provider
options ignored if the provider use is not the one specified.

See:

- ansible-community#174
- https://github.com/ansible-community/molecule-vagrant/issues/181

So, pass --provider when the provider is set in the molecule.yml.
If the test doesn't care about the provider, it should remove the
provider name setting in the molecule.yml.

Note: While this may break some setup, I believe this will prevent
new reports/issues like the one previously noted. In case one
molecule.yml file for multiple providers is wanted, use the new
providers_options, as done in our CI scenario.

Signed-off-by: Arnaud Patard <[email protected]>
@konstruktoid
Copy link
Contributor

Hi @hswong3i, could you please rebase?

In case environment variable `VAGRANT_DEFAULT_PROVIDER` is defined (see
<https://www.vagrantup.com/docs/other/environmental-variables#vagrant_default_provider>),
our `driver.provider.name` will have no effect and always start with
above default provider as specifiied.

With python-vagrant `up()` (see
<https://github.com/pycontribs/python-vagrant/blob/main/src/vagrant/__init__.py#L310-L318>)
we could specify the target provider with `provider` option, where our
current wrapper only provide the `provision` option.

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
apatard added a commit to apatard/molecule-vagrant that referenced this pull request Jan 6, 2023
Currently, vagrant is still choosing the provider to use on its own
liking. This may cause troubles like VM memory/cpus or provider
options ignored if the provider use is not the one specified.

See:

- ansible-community#174
- https://github.com/ansible-community/molecule-vagrant/issues/181

So, pass --provider when the provider is set in the molecule.yml.
If the test doesn't care about the provider, it should remove the
provider name setting in the molecule.yml.

Note: While this may break some setup, I believe this will prevent
new reports/issues like the one previously noted. In case one
molecule.yml file for multiple providers is wanted, use the new
providers_options, as done in our CI scenario.

Signed-off-by: Arnaud Patard <[email protected]>
@ssbarnea
Copy link
Member

ssbarnea commented Jan 6, 2023

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.

4 participants