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

[BUG] API CloudClient.profile() / vm_overrides not effectively merged into VM config? #64610

Closed
jmozd opened this issue Jul 7, 2023 · 1 comment · Fixed by #64701
Closed
Labels
Bug broken, incorrect, or confusing behavior Salt-Cloud

Comments

@jmozd
Copy link
Contributor

jmozd commented Jul 7, 2023

Description
It appears as if the processing of the contents of CloudClient.profile()'s vm_overrides parameter does not work as documented.

Trying to start a VM from profile, with individual overrides, does not result in merging of overrides values with the profile.

Additionally, the current implementation seems to replace nested override parameters in full, rather than merging in at all levels. The expected behavior with regards to this appears not to be documented/defined.

Setup

While extending a Python3-based automation software with a feature to start VMs (based on a common profile, but extended by individually determined parameters), we noticed that the resulting VMs were instantiated using just the profile data, without effective overrides from the separate list of per-VM data.

The setup is using a vCenter-based hypervisor setup, but the problem appears to be at the non-provider-specific part of salt-cloud.

profile exerpt (/etc/salt/cloud.profiles.d/generic.conf):

generic:
   provider: some-vcenter
   clonefrom: aCommonVmSetup

  devices:
    network:
      Network adapter 1:
        ip: 1.2.3.4
        name: 1.2.3.0
[...]

code exerpt:

our_vm_overrides = { 'somevm.company.com': { 'devices': { 'network': { 'Network adapter 1': { 'ip': '1.2.3.99'}}}} }

result = saltcloudcon.profile( profile='generic', names=[ 'somevm.company.com'], vm_overrides=our_vm_overrides)

The effective configuration of the VM has the IP address from the profile (1.2.3.4) instead of the IP from the overrides (1.2.3.99).

Debugging https://github.com/saltstack/salt/blob/master/salt/cloud/__init__.py shows that SaltCloud.profile() hands the parameters to Cloud.run_profile(), which in turn uses Cloud.vm_config() per requested VM name to merge the various sources of config information into an individual VM config (https://github.com/saltstack/salt/blob/master/salt/cloud/__init__.py#L1303)

Per documentation in CloudClient.profile(), vm_overrides is a dict with the individual VM's name as the key, a sample is given in the code.

CloudClient.profile() (IMO correctly) hands the full vm_overrides dict to Cloud.run_profile, which in turn loops over the names of VMs to start and calls Cloud.vm_config() with the individual VM name and the full vm_overrides dict. OTOH, vm_config() merges that dict into the VM configuration - resulting a VM config dict where configs from provider and profile are effectively merged, but the (intended per-VM) overrides are merged in, in full:

[...]
 provider: some-vcenter
 clonefrom: aCommonVmSetup

devices:
  network:
    Network adapter 1:
      ip: 1.2.3.4
      name: 1.2.3.0
[...]
  somevm.company.com:
    devices:
      network:
        Network adapter 1:
          ip: 1.2.3.99

That added subelement is not evaluated by the provider-specific "create()" method (at least not in the VMware implementation), therefore the overrides are not in effect.

When changing the code in vm_config() to actually merge in the VM-specific part of the vm_overrides dict, another problem surfaces:

        vm = main.copy()
        vm = salt.utils.dictupdate.update(vm, provider)
        vm = salt.utils.dictupdate.update(vm, profile)
        # vm.update(overrides)
        vm.update( overrides.get( name, {})
        vm["name"] = name

This way, any structured content in vm_overrides will replace the whole element from i.e. the profile, resulting in i.e.

[...]
   provider: some-vcenter
   clonefrom: aCommonVmSetup

  devices:
    network:
      Network adapter 1:
        ip: 1.2.3.99
[...]

(please note that "Network adapter 1" now only has the "ip", no longer the "name" element.)

To allow for more complex parameter overrides, a full merge seems more suitable:

        vm = main.copy()
        vm = salt.utils.dictupdate.update(vm, provider)
        vm = salt.utils.dictupdate.update(vm, profile)
        # vm.update(overrides)
        vm = salt.utils.dictupdate.update( vm, overrides.get( name, {}))
        vm["name"] = name

Steps to Reproduce the behavior

Use the SaltCloud.profile() API with per-VM override values.

Expected behavior
Calling SaltCloud.profile() with per-VM overrides as documented in the source code of the method, should result in calls to create the resulting VM(s) with profile data incorporating the override values in a way that the overrides are effective for the created VM(s).

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
$ salt --versions-report
Salt Version:
          Salt: 3006.1

Python Version:
        Python: 3.6.8 (default, May 31 2023, 10:28:59)

Dependency Versions:
          cffi: 1.15.1
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.0.3
       libgit2: Not Installed
  looseversion: 1.2.0
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.5
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 21.3
     pycparser: 2.21
      pycrypto: 3.18.0
  pycryptodome: 3.18.0
        pygit2: Not Installed
  python-gnupg: Not Installed
        PyYAML: 6.0
         PyZMQ: 25.0.2
        relenv: Not Installed
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist: rhel 8.8 Ootpa
        locale: UTF-8
       machine: x86_64
       release: 4.18.0-477.15.1.el8_8.x86_64
        system: Linux
       version: Red Hat Enterprise Linux 8.8 Ootpa
@jmozd jmozd added Bug broken, incorrect, or confusing behavior needs-triage labels Jul 7, 2023
@welcome
Copy link

welcome bot commented Jul 7, 2023

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Salt-Cloud
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants