Enhance and fix provider handling #190
Conversation
apatard
added
bug
| required: False | ||
| default: virtualbox | ||
| default: None |
There was a problem hiding this comment.
IMHO this looks very good, in order to sync with Vagrant default behavior.
There was a problem hiding this comment.
great. We're making progress :)
| # Keep historic behaviour and use virtualbox as "default". | ||
| # It's important in order to be able to set vm memory/cpus. | ||
| if self.provider_name is None: | ||
| self.provider_name = "virtualbox" |
There was a problem hiding this comment.
But this assumption is not too ideal? Because if we really pass in withself.provider_name = None, it will never passing as undefined to Vagrant?
There was a problem hiding this comment.
The "trick" is in up():
def up(self):
...
if self._module.params["provider_name"] is None:
self._vagrant.up(provision=provision)
else:
self._vagrant.up(provider=self.provider_name, provision=provision)
Here, the test relies on self._module.params["provider_name"] and not self.provider_name to know if --provider is to be used. As mentioned in the comment, this test is only to be able to set a "default" provider and keep current behavior. Moreover, I wanted to keep the changes as small as possible to make review easier and to limit unforeseen breakages.
|
fwiw, I'll have to update this PR soon, as the commit "Fix create/destroy playbooks ansible-lint issues" will probably have to be killed. At the same time, I'll move this patch from draft to ready for review in order to try getting it merged. |
apatard
force-pushed
the
ensure_provider2
branch
from
a360f6b
to
4b325ee
Compare
|
I've pushed yesterday a new version with only the "unwanted" lint-fix commit. Still waiting to be able to properly run the CI to ensure this PR doesn't break it / regress. |
|
Hi @apatard, could you please rebase this? |
…tions
In most test cases, there's only one provider to handle (being vbox, libvirt, ...)
but in some cases, one may want to run the CI against several providers
like in our CI with libvirt (my local testing, zuul) and gha (vbox). In these
cases, either create different scenario per provider or try to use
"clever" things like what's done in our CI with the network scenario.
A better solution is to allow to define per-providers options, as done in a
lot of Vagrantfiles like this:
Vagrant.configure("2") do |config|
...
config.vm.provider :libvirt do |prov|
...
end
config.vm.provider :virtualbox do |prov|
...
end
...
end
This will make things cleaner and avoid such "hacks" as in the one in the CI
scenario.
This new setting (providers_options) is meant to extend current provider_option
setting and not to replace it.
This means:
- current behaviour is unchanged,
- the provider_* settings are applied to all providers configured with
providers_options.
Moreover, the memory and cpus settings will be applied to all providers specified
in providers_options.
Signed-off-by: Arnaud Patard <[email protected]>
- now that there's a providers_options to handle provider specific options, use it to set qemu_use_session, - kill provider name setting on scenario where the provider doesn't matter for the test, - set providers_options in hostname scenario to ensure memory settings are always used, - fix provider_config_options scenario. With current configuration, nic_model_type is not set for virtualbox, and vbox is using e1000 by default, leading to missed not working test case. The issue is that the option is called default_nic_type in virtualbox provider so the test case should have failed. Fix that by setting the provider to virtualbox and use the correct option. In order to get the test working with libvirt+kvm, modify test_func.py to patch on the fly the molecule.yml file. The provider to use is detected with "vagrant provider" run on the tools/Vagrantfile file. - add "auto_config: true" to network settings in multi-node. Not sure why it's needed. Signed-off-by: Arnaud Patard <[email protected]>
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
force-pushed
the
ensure_provider2
branch
from
14cb0ce
to
67779ab
Compare
|
hm. I'll have to dig a little bit more about the hostname test failure. The vmware one is probably related to schema not handing the new parameter. |
|
Closing due to migration to https://github.com/ansible-community/molecule-plugins |
Current CI scenario are not handling nicely providers (or is using clever tricks) and current behaviour of vagrant choosing by itself the provider to use lead to troubles. This PR tries to address this. This could have been several PR but it made sens to me that it should be done in one PR.
I've made sure that after each commit the testsuite was still fine with vbox and libvirt (tested with py39 tox env).